Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12733012:
Provision Android devices after the 'device_status_check' step. (Closed)
|
Created:
7 years, 9 months ago by Siva Chandra Modified:
7 years, 8 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, frankf+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionProvision Android devices after the 'device_status_check' step.
BUG=169338
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191315
Patch Set 1 #
Total comments: 1
Patch Set 2 : Probable fix for subprocess command format #Patch Set 3 : Temporary commit so that the CL can be tried #Patch Set 4 : use absolute path to adb_reboot #Patch Set 5 : Fix args to provision_devices #Patch Set 6 : Add print statements which log progress #Patch Set 7 : #
Total comments: 4
Patch Set 8 : Address ilevy's comments #
Total comments: 1
Patch Set 9 : Adding host heartbeat #
Total comments: 19
Patch Set 10 : Run 'host_heartbeat' as a detached process #Patch Set 11 : Use 'aux' with ps and kill all previous host heartbeats #Patch Set 12 : Address all of ilevy's comments #Patch Set 13 : Avoid a write-write conflict #Patch Set 14 : Remove process detachment #
Total comments: 4
Patch Set 15 : Fix navabi@'s nit #Patch Set 16 : Keep the heart beat running bypassing all exceptions #
Total comments: 29
Patch Set 17 : Use an option 'auto-reconnect' to bb_device_steps #Patch Set 18 : Address ilevy's comments #
Total comments: 10
Patch Set 19 : Provision the devices with root enabled. #Patch Set 20 : Include time.h #
Messages
Total messages: 33 (0 generated)
I have tested this on my system. Works. However, the try bot is unhappy: http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered... Any ideas why?
On 2013/03/15 01:57:50, Siva Chandra wrote: > I have tested this on my system. Works. However, the try bot is unhappy: > http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered... > > Any ideas why? Is it because, when we patch on the try server, the new file loses its executable mode?
https://chromiumcodereview.appspot.com/12733012/diff/1/build/android/buildbot... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/12733012/diff/1/build/android/buildbot... build/android/buildbot/bb_device_steps.py:269: if buildername == 'Android GN (stats)': This if is a temporary. It will go away after we test this feature on that builder.
Look at patch set 7. The provisioning script is supposed to run on a builder with a particular name. However, to test on the try server, I have commented the 'if' which brings in the restriction and run on it the try server. Patch set 5 was tried and the 'provision devices' step went through fine. Patch set 6, which also has the if commented out, is now being tried. The provisioning script by itself has been tested on my machine.
I'm confused -- does provision_devices.py actually provision the devices? I don't see a call the to the provision script in build_internal. https://chromiumcodereview.appspot.com/12733012/diff/30001/build/android/buil... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/12733012/diff/30001/build/android/buil... build/android/buildbot/bb_device_steps.py:269: if buildername == 'Android GN (stats)': I'd prefer to filter by waterfall instead of by builder. Also -- provision_devices reboots each phone, so we should not also run RebootDevices (line 260/261) if we are provisioning. https://chromiumcodereview.appspot.com/12733012/diff/30001/build/android/buil... build/android/buildbot/bb_device_steps.py:274: RunCmd(cmd, flunk_on_failure=False) Let's not set 'flunk_on_failure=False'. I'd prefer to avoid this paradigm unless we specifically find we need it.
I have not responded to code changes. We will get the basics of this CL sorted out first. I have one comment inline in the code. About not using the provisioning script in build_internal, few reasons: 1. The build_internal provisioning script is not stable to the level of comfort that it can run continuously on bots. 2. We really do not want all the functionality right away on the bots that we should fix the internal provisioning script now. We can get started with this script, which can later call the internal provisioning script if required. The two things the script in this CL is doing have not had any issues so far, and seem to be reliable enough to run on bots safely. 3. The script in this CL depends on the the adb_reboot binary built from source in the chromium tree. 4. Last, and the least important of the reasons (actually, something I would like to know), can we invoke scripts from build_internal from public code? https://chromiumcodereview.appspot.com/12733012/diff/30001/build/android/buil... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/12733012/diff/30001/build/android/buil... build/android/buildbot/bb_device_steps.py:269: if buildername == 'Android GN (stats)': On 2013/03/19 00:05:22, Isaac wrote: > I'd prefer to filter by waterfall instead of by builder. Also -- > provision_devices reboots each phone, so we should not also run RebootDevices > (line 260/261) if we are provisioning. This particular provisioning script does not reboot the phones.
https://chromiumcodereview.appspot.com/12733012/diff/30001/build/android/buil... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/12733012/diff/30001/build/android/buil... build/android/buildbot/bb_device_steps.py:269: if buildername == 'Android GN (stats)': On 2013/03/19 00:45:05, Siva Chandra wrote: > On 2013/03/19 00:05:22, Isaac wrote: > > I'd prefer to filter by waterfall instead of by builder. Also -- > > provision_devices reboots each phone, so we should not also run RebootDevices > > (line 260/261) if we are provisioning. > > This particular provisioning script does not reboot the phones. How about we use options.experimental for this, which will enable it on the FYI waterfall? The issues with line 269 are: 1) We really, really try to avoid looking at builder name. 2) Logic for test options should be in bb_run_bot.py, not here.
PTAL at patch set 8. https://chromiumcodereview.appspot.com/12733012/diff/37001/build/android/buil... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/12733012/diff/37001/build/android/buil... build/android/buildbot/bb_device_steps.py:271: '/out/%s/adb_reboot' % options.factory_properties['target']) Presubmit fails with because of missing key 'target' in factory_properties. I will fix it if the rest of the CL is OK.
PT a L The latest patchset adds a host heartbeat as part of provisioning. This step launches a heartbeat on the host. If a heartbeat from the previous run is running, it will be killed before a new one is launched.
PING
This is failing trybots?
On 2013/03/25 21:07:14, Isaac wrote: > This is failing trybots? I do not know why they failed. I am retrying now. It works as expected on my system. Are the basics of this patch OK? For example, look at how I launch the host heartbeat after provisioning the devices.
I have some comments, I think the general idea is solid though. https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/buil... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/buil... build/android/buildbot/bb_device_steps.py:271: '/out/%s/adb_reboot' % options.factory_properties['target']) default to 'Debug'' if factory_properties is missing target. https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/buil... build/android/buildbot/bb_device_steps.py:272: cmd = ['build/android/provision_devices.py', '-a', '%s' % adb_reboot] just use adb_reboot here. https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/buil... build/android/buildbot/bb_device_steps.py:273: RunCmd(cmd) Just call the command directly instead of making a temp variable. https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/prov... File build/android/provision_devices.py (right): https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/prov... build/android/provision_devices.py:30: if 'adb_reboot' in dev_proc: I'd prefer a more structured parsing of ps. https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/prov... build/android/provision_devices.py:35: if android_cmd.FileExistsOnDevice('/data/local/adb_reboot'): why delete? Just run pushifneeded. https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/prov... build/android/provision_devices.py:43: p1 = subprocess.Popen(['echo', 'cd /data/local; ./adb_reboot; exit'], don't need to echo and pipe. You can use subprocess.popen and then send input with communicate(<input str>) https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/prov... build/android/provision_devices.py:55: pid = re.findall('(\d+)', line)[0] You should use a regex in line 52 and get the group from the match object.. https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/prov... build/android/provision_devices.py:56: subprocess.call(['kill', '%s' % pid]) prefer str(pid) https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/prov... build/android/provision_devices.py:61: subprocess.Popen([cmd]) Are you expecting this to not die when the parent dies? I think it will.
PTaL at patch set 12. https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/buil... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/buil... build/android/buildbot/bb_device_steps.py:271: '/out/%s/adb_reboot' % options.factory_properties['target']) On 2013/03/25 23:50:03, Isaac wrote: > default to 'Debug'' if factory_properties is missing target. Done. https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/buil... build/android/buildbot/bb_device_steps.py:272: cmd = ['build/android/provision_devices.py', '-a', '%s' % adb_reboot] On 2013/03/25 23:50:03, Isaac wrote: > just use adb_reboot here. Done. https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/buil... build/android/buildbot/bb_device_steps.py:273: RunCmd(cmd) On 2013/03/25 23:50:03, Isaac wrote: > Just call the command directly instead of making a temp variable. Done. https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/prov... File build/android/provision_devices.py (right): https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/prov... build/android/provision_devices.py:30: if 'adb_reboot' in dev_proc: On 2013/03/25 23:50:03, Isaac wrote: > I'd prefer a more structured parsing of ps. Like? https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/prov... build/android/provision_devices.py:35: if android_cmd.FileExistsOnDevice('/data/local/adb_reboot'): On 2013/03/25 23:50:03, Isaac wrote: > why delete? Just run pushifneeded. Done. https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/prov... build/android/provision_devices.py:43: p1 = subprocess.Popen(['echo', 'cd /data/local; ./adb_reboot; exit'], On 2013/03/25 23:50:03, Isaac wrote: > don't need to echo and pipe. You can use subprocess.popen and then send input > with communicate(<input str>) This does not work: p = subprocess.Popen(['adb', '-s', device, 'shell'], universal_newlines=True) p.communicate('cd /data/local; ./adb_reboot; exit') https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/prov... build/android/provision_devices.py:55: pid = re.findall('(\d+)', line)[0] On 2013/03/25 23:50:03, Isaac wrote: > You should use a regex in line 52 and get the group from the match object.. Done. https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/prov... build/android/provision_devices.py:56: subprocess.call(['kill', '%s' % pid]) On 2013/03/25 23:50:03, Isaac wrote: > prefer str(pid) Done. https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/prov... build/android/provision_devices.py:61: subprocess.Popen([cmd]) On 2013/03/25 23:50:03, Isaac wrote: > Are you expecting this to not die when the parent dies? I think it will. On my system, the process stays alive, dont know why. But in the latest patch set, I am creating a detached process. And, I think this is also the reason why the try jobs are failing.
Look at the latest patch set. A patch set before that was tried on the tryserver and even though it failed it looks good to me. I say this because, that particular bot has been red for a few builds preceding my try for the exact same test which is failing in my try. https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/prov... File build/android/provision_devices.py (right): https://chromiumcodereview.appspot.com/12733012/diff/41001/build/android/prov... build/android/provision_devices.py:61: subprocess.Popen([cmd]) On 2013/03/26 02:27:51, Siva Chandra wrote: > On 2013/03/25 23:50:03, Isaac wrote: > > Are you expecting this to not die when the parent dies? I think it will. > > On my system, the process stays alive, dont know why. But in the latest patch > set, I am creating a detached process. And, I think this is also the reason why > the try jobs are failing. I do not think we need to have a detached process. Just spawning with subprocess.Popen seems to be working. I have tested it.
I missed explaining the change in the new patch sets in previous mail. From patch set 13, we do the host-device hand shake without potentially involving a write-write conflict. Earlier, we had the device 'touch' a file, and the host remove it. Since host removes at a much higher rate than the device creates, there is a possibility of a write-write conflict. Starting with patch set 13, we are eliminating this by making the host 'touch' (which is a write) a file, while the device only 'reads' this files last modified time. There can never be a write-write conflict.
We can run it again on the android_fyi_dbg_triggered_tests bot as it stopped failing the test that was always failing. http://build.chromium.org/p/tryserver.chromium/builders/android_fyi_dbg_trigg... https://chromiumcodereview.appspot.com/12733012/diff/8002/build/android/host_... File build/android/host_heartbeat.py (right): https://chromiumcodereview.appspot.com/12733012/diff/8002/build/android/host_... build/android/host_heartbeat.py:17: PULSE_PERIOD = 20 I thought the heartbeat was every 1 min, and the check on the device was every 2 minutes. Any reason to change the heartbeat to every 20 secs? It looks like the daemon is still checking ever 2 mins.
On 2013/03/27 19:53:31, navabi wrote: > We can run it again on the android_fyi_dbg_triggered_tests bot as it stopped > failing the test that was always failing. I just triggered the try job. Let's see if it goes through.
Thanks for triggering a try job Armand. https://chromiumcodereview.appspot.com/12733012/diff/8002/build/android/host_... File build/android/host_heartbeat.py (right): https://chromiumcodereview.appspot.com/12733012/diff/8002/build/android/host_... build/android/host_heartbeat.py:17: PULSE_PERIOD = 20 On 2013/03/27 19:53:32, navabi wrote: > I thought the heartbeat was every 1 min, and the check on the device was every 2 > minutes. Any reason to change the heartbeat to every 20 secs? It looks like the > daemon is still checking ever 2 mins. I think the host should say 'I can see you' at a much faster rate than the device checks. For example, if the host says at only twice the frequency the device is checking, there could be a transient USB issue due which the host pulse could miss reaching the device. This will make the device think that the it lost connection with the host.
It has succeeded bow! So, PTaL with no nervousness :)
lgtm with nit, but best to let ilevy take a look since he has reviewed the previous patches in closer detail. https://chromiumcodereview.appspot.com/12733012/diff/8002/tools/android/adb_r... File tools/android/adb_reboot/adb_reboot.c (right): https://chromiumcodereview.appspot.com/12733012/diff/8002/tools/android/adb_r... tools/android/adb_reboot/adb_reboot.c:31: time_t ct; Declare these at the top of the method instead of inside of the while loop.
https://chromiumcodereview.appspot.com/12733012/diff/8002/tools/android/adb_r... File tools/android/adb_reboot/adb_reboot.c (right): https://chromiumcodereview.appspot.com/12733012/diff/8002/tools/android/adb_r... tools/android/adb_reboot/adb_reboot.c:31: time_t ct; On 2013/03/27 22:05:29, navabi wrote: > Declare these at the top of the method instead of inside of the while loop. Done.
I have some more comments, I'll look over the new patchset when it's ready. Thanks! https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/host... File build/android/host_heartbeat.py (right): https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/host... build/android/host_heartbeat.py:25: android_cmd.EnableAdbRoot() I don't think we should be called this here as it can disconnect adb. Instead use 'su' as part of the command to run on phone as root. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... File build/android/provision_devices.py (right): https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:24: def PushAndLaunchAdbReboot(device, adb_reboot): it's unclear what type of data these ares are, could you add a pydoc section with definitions? https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:28: dev_procs = android_cmd.RunShellCommand('ps') Let's use android_commands.KillAllBlocking instead of this logic. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:36: android_cmd.PushIfNeeded('%s' % adb_reboot, '/data/local/') adb_reboot is already a string. Just PushIfNeeded(adb_reboot, '/data/local'). https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:41: subprocess.call(['adb', '-s', device, 'shell'], stdin=p.stdout) Try: p = subprocess.Popen(['adb', '-s', device, 'shell'], stdin=subproccess.PIPE) p.communicate('/data/local/adb_reboot; exit\n') https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:45: ps = subprocess.Popen(['ps', 'aux'], stdout = subprocess.PIPE) Use killall or pkill to kill by name. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:53: cmd = '%s/build/android/host_heartbeat.py' % constants.CHROME_DIR os.path.join(constants.CHROME_DIR, 'build/......heartbeat.py') https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:54: print 'Launching %s ...' % cmd Not sure we need full path here, maybe just print 'Spawning host_heartbeat' which will let you get rid of 'cmd' variable. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:73: parser.add_option('-d', '--device', dest='device', default=None, None is the default, you can remove that argument https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:75: parser.add_option('-a', '--adb_reboot', dest='adb_reboot', default=None, use hyphens, not underscores for long arguments: '--adb-reboot', which will automatically save in options.adb_reboot (dest is not needed) https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:77: options, _ = parser.parse_args(argv) Let's check the args list for non-parsed arguments and throw error. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:81: return -1 negative error codes have undefined behavior in bash. Let's use 1.
PTAL https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/host... File build/android/host_heartbeat.py (right): https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/host... build/android/host_heartbeat.py:25: android_cmd.EnableAdbRoot() On 2013/03/28 22:37:44, Isaac wrote: > I don't think we should be called this here as it can disconnect adb. > > Instead use 'su' as part of the command to run on phone as root. If I add the 'su ' prefix, it says "permission denied". I removed EnableAdbRoot() as the device should be on root anyway. The first part of the provisioning script ensures this. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... File build/android/provision_devices.py (right): https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:24: def PushAndLaunchAdbReboot(device, adb_reboot): On 2013/03/28 22:37:44, Isaac wrote: > it's unclear what type of data these ares are, could you add a pydoc section > with definitions? Done. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:28: dev_procs = android_cmd.RunShellCommand('ps') On 2013/03/28 22:37:44, Isaac wrote: > Let's use android_commands.KillAllBlocking instead of this logic. Done. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:36: android_cmd.PushIfNeeded('%s' % adb_reboot, '/data/local/') On 2013/03/28 22:37:44, Isaac wrote: > adb_reboot is already a string. Just PushIfNeeded(adb_reboot, '/data/local'). Done. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:41: subprocess.call(['adb', '-s', device, 'shell'], stdin=p.stdout) On 2013/03/28 22:37:44, Isaac wrote: > Try: > p = subprocess.Popen(['adb', '-s', device, 'shell'], stdin=subproccess.PIPE) > p.communicate('/data/local/adb_reboot; exit\n') > Done. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:45: ps = subprocess.Popen(['ps', 'aux'], stdout = subprocess.PIPE) On 2013/03/28 22:37:44, Isaac wrote: > Use killall or pkill to kill by name. The name for the process we to want kill happens to be python. At least, I tried pkill ".*host_heartbeat.*' and killall -r ".*host_heartbeat.*" and they don't kill host_heartbeat. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:53: cmd = '%s/build/android/host_heartbeat.py' % constants.CHROME_DIR On 2013/03/28 22:37:44, Isaac wrote: > os.path.join(constants.CHROME_DIR, 'build/......heartbeat.py') Done. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:54: print 'Launching %s ...' % cmd On 2013/03/28 22:37:44, Isaac wrote: > Not sure we need full path here, maybe just > > print 'Spawning host_heartbeat' > > which will let you get rid of 'cmd' variable. Done. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:73: parser.add_option('-d', '--device', dest='device', default=None, On 2013/03/28 22:37:44, Isaac wrote: > None is the default, you can remove that argument Done. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:75: parser.add_option('-a', '--adb_reboot', dest='adb_reboot', default=None, On 2013/03/28 22:37:44, Isaac wrote: > use hyphens, not underscores for long arguments: > > '--adb-reboot', which will automatically save in options.adb_reboot (dest is not > needed) Done. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:77: options, _ = parser.parse_args(argv) On 2013/03/28 22:37:44, Isaac wrote: > Let's check the args list for non-parsed arguments and throw error. Done. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:81: return -1 On 2013/03/28 22:37:44, Isaac wrote: > negative error codes have undefined behavior in bash. Let's use 1. Done.
https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/host... File build/android/host_heartbeat.py (right): https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/host... build/android/host_heartbeat.py:25: android_cmd.EnableAdbRoot() On 2013/03/28 23:52:47, Siva Chandra wrote: > On 2013/03/28 22:37:44, Isaac wrote: > > I don't think we should be called this here as it can disconnect adb. > > > > Instead use 'su' as part of the command to run on phone as root. > > If I add the 'su ' prefix, it says "permission denied". I removed > EnableAdbRoot() as the device should be on root anyway. The first part of the > provisioning script ensures this. You might not be calling with su right. I think you need su -c and then putting them command in another layer of quotes. We should get this right because not all phones run as root. I don't think the perf ones do. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... File build/android/provision_devices.py (right): https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:45: ps = subprocess.Popen(['ps', 'aux'], stdout = subprocess.PIPE) On 2013/03/28 23:52:47, Siva Chandra wrote: > On 2013/03/28 22:37:44, Isaac wrote: > > Use killall or pkill to kill by name. > > The name for the process we to want kill happens to be python. At least, I tried > pkill ".*host_heartbeat.*' and killall -r ".*host_heartbeat.*" and they don't > kill host_heartbeat. I think there is a way to do this with pkill. I don't know the syntax well enough to point you to it but there are man pages and stackoverflow. https://chromiumcodereview.appspot.com/12733012/diff/94001/build/android/buil... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/12733012/diff/94001/build/android/buil... build/android/buildbot/bb_device_steps.py:345: help='Push auto reconnect script on to the device.') 'Push script to device which restarts adbd on disconnections' https://chromiumcodereview.appspot.com/12733012/diff/94001/build/android/prov... File build/android/provision_devices.py (right): https://chromiumcodereview.appspot.com/12733012/diff/94001/build/android/prov... build/android/provision_devices.py:52: subprocess.call(['kill', '%s' % str(pid)]) '%s' % <string> is a no-op. Just use <string> or in this case str(pid). https://chromiumcodereview.appspot.com/12733012/diff/94001/build/android/prov... build/android/provision_devices.py:66: android_cmd.EnableAdbRoot() This will convert phone to root until the phone is restarted and the automated restart is happening before this script is called. So this will enable root for the remainder of the build. https://chromiumcodereview.appspot.com/12733012/diff/94001/build/android/prov... build/android/provision_devices.py:74: parser.add_option('-d', '--device', dest='device', remove dest param, unneeded https://chromiumcodereview.appspot.com/12733012/diff/94001/build/android/prov... build/android/provision_devices.py:76: parser.add_option('-a', '--adb-reboot', dest='adb_reboot', dest param unneded. Also I'd rather this script take the target (release vs. debug) as a param rather than the full path and used constants to figure out the full path to adb-reboot. That way if we move adb_reboot, we can change this script and let the buildbot scripts directory stay stable.
Look at the latest patch set. Though it says "with root enabled", it should have been "without root enabled." I addressed all comments except the one about killall and pkill. For that as well, I have an inline comment. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/host... File build/android/host_heartbeat.py (right): https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/host... build/android/host_heartbeat.py:25: android_cmd.EnableAdbRoot() On 2013/03/29 00:18:32, Isaac wrote: > On 2013/03/28 23:52:47, Siva Chandra wrote: > > On 2013/03/28 22:37:44, Isaac wrote: > > > I don't think we should be called this here as it can disconnect adb. > > > > > > Instead use 'su' as part of the command to run on phone as root. > > > > If I add the 'su ' prefix, it says "permission denied". I removed > > EnableAdbRoot() as the device should be on root anyway. The first part of the > > provisioning script ensures this. > > You might not be calling with su right. I think you need su -c and then putting > them command in another layer of quotes. > > We should get this right because not all phones run as root. I don't think the > perf ones do. This is what I tried and tested to be working: I dont put the phones on root. Just reboot them and run the provisioning script. The adb_reboot binary is pushed and launched correctly. The host_heartbeat is spawned and is touching the heartbeat file correctly. What I have changed is the C code for the adb_reboot binary. I start and stop adbd with su -c. https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... File build/android/provision_devices.py (right): https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:45: ps = subprocess.Popen(['ps', 'aux'], stdout = subprocess.PIPE) On 2013/03/29 00:18:32, Isaac wrote: > On 2013/03/28 23:52:47, Siva Chandra wrote: > > On 2013/03/28 22:37:44, Isaac wrote: > > > Use killall or pkill to kill by name. > > > > The name for the process we to want kill happens to be python. At least, I > tried > > pkill ".*host_heartbeat.*' and killall -r ".*host_heartbeat.*" and they don't > > kill host_heartbeat. > > I think there is a way to do this with pkill. I don't know the syntax well > enough to point you to it but there are man pages and stackoverflow. Stackoverflow gives this: http://stackoverflow.com/questions/3510673/find-and-kill-a-process-in-one-lin... https://chromiumcodereview.appspot.com/12733012/diff/94001/build/android/buil... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/12733012/diff/94001/build/android/buil... build/android/buildbot/bb_device_steps.py:345: help='Push auto reconnect script on to the device.') On 2013/03/29 00:18:33, Isaac wrote: > 'Push script to device which restarts adbd on disconnections' Done. https://chromiumcodereview.appspot.com/12733012/diff/94001/build/android/prov... File build/android/provision_devices.py (right): https://chromiumcodereview.appspot.com/12733012/diff/94001/build/android/prov... build/android/provision_devices.py:52: subprocess.call(['kill', '%s' % str(pid)]) On 2013/03/29 00:18:33, Isaac wrote: > '%s' % <string> is a no-op. Just use <string> or in this case str(pid). Done. https://chromiumcodereview.appspot.com/12733012/diff/94001/build/android/prov... build/android/provision_devices.py:66: android_cmd.EnableAdbRoot() On 2013/03/29 00:18:33, Isaac wrote: > This will convert phone to root until the phone is restarted and the automated > restart is happening before this script is called. So this will enable root for > the remainder of the build. Removed enabling root. https://chromiumcodereview.appspot.com/12733012/diff/94001/build/android/prov... build/android/provision_devices.py:74: parser.add_option('-d', '--device', dest='device', On 2013/03/29 00:18:33, Isaac wrote: > remove dest param, unneeded Done. https://chromiumcodereview.appspot.com/12733012/diff/94001/build/android/prov... build/android/provision_devices.py:76: parser.add_option('-a', '--adb-reboot', dest='adb_reboot', On 2013/03/29 00:18:33, Isaac wrote: > dest param unneded. Also I'd rather this script take the target (release vs. > debug) as a param rather than the full path and used constants to figure out the > full path to adb-reboot. That way if we move adb_reboot, we can change this > script and let the buildbot scripts directory stay stable. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/12733012/103001
https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... File build/android/provision_devices.py (right): https://chromiumcodereview.appspot.com/12733012/diff/86001/build/android/prov... build/android/provision_devices.py:45: ps = subprocess.Popen(['ps', 'aux'], stdout = subprocess.PIPE) On 2013/03/29 01:47:06, Siva Chandra wrote: > On 2013/03/29 00:18:32, Isaac wrote: > > On 2013/03/28 23:52:47, Siva Chandra wrote: > > > On 2013/03/28 22:37:44, Isaac wrote: > > > > Use killall or pkill to kill by name. > > > > > > The name for the process we to want kill happens to be python. At least, I > > tried > > > pkill ".*host_heartbeat.*' and killall -r ".*host_heartbeat.*" and they > don't > > > kill host_heartbeat. > > > > I think there is a way to do this with pkill. I don't know the syntax well > > enough to point you to it but there are man pages and stackoverflow. > > Stackoverflow gives this: > http://stackoverflow.com/questions/3510673/find-and-kill-a-process-in-one-lin... Maybe I'm missing something but I don't see where they say this can't be done with 1-2 commands. I'm not saying that this python code won't work, just that it is unnecessary because linux already has commands specifically for this.
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/12733012/124001
On 2013/03/29 02:27:43, I haz the power (commit-bot) wrote: > Sorry for I got bad news for ya. > Compile failed with a clobber build on android_clang_dbg. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... > Your code is likely broken or HEAD is junk. Please ensure your > code is not broken then alert the build sheriffs. > Look at the try server FAQ for more details. I tested compiling the adb_reboot.c file on my system with ndk-build and don't see any errors. But the clang builder complains. I have hopefully fixed it now by including time.h explicitly in patch set 20. On the commit queue now.
Message was sent while issue was closed.
Change committed as 191315 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
