Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2334223003:
Android GN: don't reboot before running (Closed)
DescriptionAndroid GN: don't reboot before running
We're already rebooting after running, so we should already be ready to go when the task starts.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2334223003
Committed: https://skia.googlesource.com/skia/+/ea8b2b076c8aa0da7bf7e84a42cd2af2164c1477
Patch Set 1 #Patch Set 2 : sleep(30) #
Total comments: 3
Patch Set 3 : other way #
Total comments: 7
Patch Set 4 : no wait #Patch Set 5 : rebase #
Messages
Total messages: 17 (7 generated)
Description was changed from ========== Android GN: wait for sys.boot_completed Seems necessary for Galaxy S7. BUG=skia: ========== to ========== Android GN: wait for sys.boot_completed Seems necessary for Galaxy S7. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2334223003 ==========
Description was changed from ========== Android GN: wait for sys.boot_completed Seems necessary for Galaxy S7. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2334223003 ========== to ========== Android GN: wait for sys.boot_completed Seems necessary for Galaxy S7. The flakiness from PS 1 lead to the hyper-conservative sleep(30) in PS 2. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2334223003 ==========
mtklein@chromium.org changed reviewers: + borenet@google.com
https://codereview.chromium.org/2334223003/diff/20001/infra/bots/recipe_modul... File infra/bots/recipe_modules/flavor/gn_android_flavor.py (right): https://codereview.chromium.org/2334223003/diff/20001/infra/bots/recipe_modul... infra/bots/recipe_modules/flavor/gn_android_flavor.py:64: self._adb('reboot', 'reboot') I recommend sleeping for at least 10s after issuing the reboot command, or the device won't have actually gone down for reboot before wait-for-device declares that it's back up again. https://codereview.chromium.org/2334223003/diff/20001/infra/bots/recipe_modul... infra/bots/recipe_modules/flavor/gn_android_flavor.py:69: import time I recommend folding the "adb reboot", "sleep 10", "adb wait-for-usb-device", and getprop stuff all into this inline script. https://codereview.chromium.org/2334223003/diff/20001/infra/bots/recipe_modul... infra/bots/recipe_modules/flavor/gn_android_flavor.py:73: 'getprop', 'sys.boot_completed']) Might want to try/catch this in case weird things happen to the USB connection while the device is booting.
Have a look at PS3? It takes things the complete opposite way: if we're having trouble waiting on the install() reboot, don't do the install() reboot at all. Instead, just get going with the tests, leaving the cleanup_steps() reboot as our slate cleaner.
https://codereview.chromium.org/2334223003/diff/40001/infra/bots/recipe_modul... File infra/bots/recipe_modules/flavor/gn_android_flavor.py (right): https://codereview.chromium.org/2334223003/diff/40001/infra/bots/recipe_modul... infra/bots/recipe_modules/flavor/gn_android_flavor.py:64: self._adb('wait for device', 'wait-for-usb-device') If we think we have to wait here, we might as well wait for boot-completed, just in case. https://codereview.chromium.org/2334223003/diff/40001/infra/bots/recipe_modul... infra/bots/recipe_modules/flavor/gn_android_flavor.py:70: self._adb('reboot', 'reboot') To be clear, we're rebooting and not waiting for the device to come back?
mtklein@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/2334223003/diff/40001/infra/bots/recipe_modul... File infra/bots/recipe_modules/flavor/gn_android_flavor.py (right): https://codereview.chromium.org/2334223003/diff/40001/infra/bots/recipe_modul... infra/bots/recipe_modules/flavor/gn_android_flavor.py:64: self._adb('wait for device', 'wait-for-usb-device') On 2016/09/13 18:04:52, borenet wrote: > If we think we have to wait here, we might as well wait for boot-completed, just > in case. Good point. Let's try no wait at all (PS 4), and then if that's no good I'll restore a full _wait_for_device(): sleep 10 wait-for-usb-device wait for sys.boot_completed https://codereview.chromium.org/2334223003/diff/40001/infra/bots/recipe_modul... infra/bots/recipe_modules/flavor/gn_android_flavor.py:70: self._adb('reboot', 'reboot') On 2016/09/13 18:04:52, borenet wrote: > To be clear, we're rebooting and not waiting for the device to come back? Right, seems to work okay. If it doesn't, I'll put self._wait_for_device() here too.
https://codereview.chromium.org/2334223003/diff/40001/infra/bots/recipe_modul... File infra/bots/recipe_modules/flavor/gn_android_flavor.py (right): https://codereview.chromium.org/2334223003/diff/40001/infra/bots/recipe_modul... infra/bots/recipe_modules/flavor/gn_android_flavor.py:64: self._adb('wait for device', 'wait-for-usb-device') On 2016/09/13 18:20:54, mtklein wrote: > On 2016/09/13 18:04:52, borenet wrote: > > If we think we have to wait here, we might as well wait for boot-completed, > just > > in case. > > Good point. Let's try no wait at all (PS 4), and then if that's no good I'll > restore a full _wait_for_device(): > > sleep 10 > wait-for-usb-device > wait for sys.boot_completed err, PS 5, which is PS 4 rebased.
LGTM but now the CL needs a new title. https://codereview.chromium.org/2334223003/diff/40001/infra/bots/recipe_modul... File infra/bots/recipe_modules/flavor/gn_android_flavor.py (right): https://codereview.chromium.org/2334223003/diff/40001/infra/bots/recipe_modul... infra/bots/recipe_modules/flavor/gn_android_flavor.py:64: self._adb('wait for device', 'wait-for-usb-device') On 2016/09/13 18:23:26, mtklein wrote: > On 2016/09/13 18:20:54, mtklein wrote: > > On 2016/09/13 18:04:52, borenet wrote: > > > If we think we have to wait here, we might as well wait for boot-completed, > > just > > > in case. > > > > Good point. Let's try no wait at all (PS 4), and then if that's no good I'll > > restore a full _wait_for_device(): > > > > sleep 10 > > wait-for-usb-device > > wait for sys.boot_completed > > err, PS 5, which is PS 4 rebased. SGTM https://codereview.chromium.org/2334223003/diff/40001/infra/bots/recipe_modul... infra/bots/recipe_modules/flavor/gn_android_flavor.py:70: self._adb('reboot', 'reboot') On 2016/09/13 18:20:54, mtklein wrote: > On 2016/09/13 18:04:52, borenet wrote: > > To be clear, we're rebooting and not waiting for the device to come back? > > Right, seems to work okay. If it doesn't, I'll put self._wait_for_device() here > too. SGTM. I think it'll be fine, my only concern would be bots getting quarantined because the device hasn't finished rebooting yet, and that would only be an issue if we were getting alerts about it.
Description was changed from ========== Android GN: wait for sys.boot_completed Seems necessary for Galaxy S7. The flakiness from PS 1 lead to the hyper-conservative sleep(30) in PS 2. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2334223003 ========== to ========== Android GN: don't reboot before running We're already rebooting after running, so we should already be ready to go when the task starts. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2334223003 ==========
> LGTM but now the CL needs a new title. Done. Just wanted to keep your review thread all together. > SGTM. I think it'll be fine, my only concern would be bots getting quarantined > because the device hasn't finished rebooting yet, and that would only be an > issue if we were getting alerts about it. Right, that it hasn't happened so far seems promising.
The CQ bit was checked by mtklein@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Android GN: don't reboot before running We're already rebooting after running, so we should already be ready to go when the task starts. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2334223003 ========== to ========== Android GN: don't reboot before running We're already rebooting after running, so we should already be ready to go when the task starts. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2334223003 Committed: https://skia.googlesource.com/skia/+/ea8b2b076c8aa0da7bf7e84a42cd2af2164c1477 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/ea8b2b076c8aa0da7bf7e84a42cd2af2164c1477 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
