Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22857005:
Android: uses taskset when starting adb. (Closed)
|
Created:
7 years, 4 months ago by bulach Modified:
7 years, 4 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAndroid: uses taskset when starting adb.
Workaround for some adb issues.
While at it, print the step name before doing any action.
BUG=268450
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217959
Patch Set 1 #
Total comments: 4
Patch Set 2 : android_commands #
Total comments: 2
Patch Set 3 : wait for adb command #
Total comments: 4
Patch Set 4 : bb_utils.TESTING #Messages
Total messages: 23 (0 generated)
ptal
https://codereview.chromium.org/22857005/diff/1/build/android/buildbot/bb_dev... File build/android/buildbot/bb_device_steps.py (right): https://codereview.chromium.org/22857005/diff/1/build/android/buildbot/bb_dev... build/android/buildbot/bb_device_steps.py:245: RunCmd(['sleep', '1']) All interactions with adb should go through android_commands.py. Let's use RestartAdbServer() tehre.
https://codereview.chromium.org/22857005/diff/1/build/android/buildbot/bb_dev... File build/android/buildbot/bb_device_steps.py (right): https://codereview.chromium.org/22857005/diff/1/build/android/buildbot/bb_dev... build/android/buildbot/bb_device_steps.py:245: RunCmd(['sleep', '1']) On 2013/08/14 17:03:01, frankf wrote: > All interactions with adb should go through android_commands.py. Let's use > RestartAdbServer() tehre. hmm... I think this just emits the commands, it doesn't really run them? ilevy? I mean, we could potentially have a build/android/restart_adb_server.py that then depends on pylib/android_commands.py if you prefer? (also, please note this patch just adds the taskset, it doesn't add a new dependency to adb..)
https://codereview.chromium.org/22857005/diff/1/build/android/buildbot/bb_dev... File build/android/buildbot/bb_device_steps.py (right): https://codereview.chromium.org/22857005/diff/1/build/android/buildbot/bb_dev... build/android/buildbot/bb_device_steps.py:245: RunCmd(['sleep', '1']) I'm not following. RunCmd here just runs that exact command. It's good to centralize all knowledge of adb in android_commands.py and fix the issue there (Otherwise someone might use RestartAdbServer which doesn't have CPU affinity) There are existing calls to android_commands.py in this file (this is issue with existing code not your changes) On 2013/08/14 17:56:24, bulach wrote: > On 2013/08/14 17:03:01, frankf wrote: > > All interactions with adb should go through android_commands.py. Let's use > > RestartAdbServer() tehre. > > hmm... I think this just emits the commands, it doesn't really run them? > ilevy? > I mean, we could potentially have a build/android/restart_adb_server.py that > then depends on pylib/android_commands.py if you prefer? > > (also, please note this patch just adds the taskset, it doesn't add a new > dependency to adb..)
https://codereview.chromium.org/22857005/diff/1/build/android/buildbot/bb_dev... File build/android/buildbot/bb_device_steps.py (right): https://codereview.chromium.org/22857005/diff/1/build/android/buildbot/bb_dev... build/android/buildbot/bb_device_steps.py:245: RunCmd(['sleep', '1']) On 2013/08/14 18:05:44, frankf wrote: > I'm not following. RunCmd here just runs that exact command. It's good to > centralize all knowledge of adb in android_commands.py and fix the issue there > (Otherwise someone might use RestartAdbServer which doesn't have CPU affinity) > There are existing calls to android_commands.py in this file (this is issue with > existing code not your changes) > > On 2013/08/14 17:56:24, bulach wrote: > > On 2013/08/14 17:03:01, frankf wrote: > > > All interactions with adb should go through android_commands.py. Let's use > > > RestartAdbServer() tehre. > > > > hmm... I think this just emits the commands, it doesn't really run them? > > ilevy? > > I mean, we could potentially have a build/android/restart_adb_server.py that > > then depends on pylib/android_commands.py if you prefer? > > > > (also, please note this patch just adds the taskset, it doesn't add a new > > dependency to adb..) > ahn, you're right! my confusion, I thought this would just "generate" the commands rather than actually executing it.. new patch in a sec.
ptal :)
lgtm w/ a concern https://codereview.chromium.org/22857005/diff/11001/build/android/pylib/andro... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/22857005/diff/11001/build/android/pylib/andro... build/android/pylib/android_commands.py:463: self.StartAdbServer() Looks like we ignore the return code and silently fail?
lgtm
https://codereview.chromium.org/22857005/diff/11001/build/android/pylib/andro... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/22857005/diff/11001/build/android/pylib/andro... build/android/pylib/android_commands.py:463: self.StartAdbServer() On 2013/08/14 18:34:11, frankf wrote: > Looks like we ignore the return code and silently fail? ha, good catch! it's worse than that.. :) I tried to just check the ret code and raise an exception, but the presubmit at build/android/buildbot/tests/bb_run_bot_test.py presubmit failed... what happens is that kill-server and start-server are async, so they may return "0", and yet not finish, so a subsequent pair comes along and fails... so the "right" fix is to actually block and wait on KillAdbServer / StartAdbServer, so that this can safely throw an error..
https://codereview.chromium.org/22857005/diff/21001/build/android/buildbot/bb... File build/android/buildbot/bb_device_steps.py (right): https://codereview.chromium.org/22857005/diff/21001/build/android/buildbot/bb... build/android/buildbot/bb_device_steps.py:243: adb = android_commands.AndroidCommands() these should be guarded to not run in testing mode.
lgtm, Thanks Marcus! https://codereview.chromium.org/22857005/diff/21001/build/android/pylib/andro... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/22857005/diff/21001/build/android/pylib/andro... build/android/pylib/android_commands.py:492: # pgrep fond adb, start-server succeeded. typo
thanks everyone! comments addressed, isaac, ptal. https://codereview.chromium.org/22857005/diff/21001/build/android/buildbot/bb... File build/android/buildbot/bb_device_steps.py (right): https://codereview.chromium.org/22857005/diff/21001/build/android/buildbot/bb... build/android/buildbot/bb_device_steps.py:243: adb = android_commands.AndroidCommands() On 2013/08/14 19:53:29, Isaac wrote: > these should be guarded to not run in testing mode. Done. https://codereview.chromium.org/22857005/diff/21001/build/android/pylib/andro... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/22857005/diff/21001/build/android/pylib/andro... build/android/pylib/android_commands.py:492: # pgrep fond adb, start-server succeeded. On 2013/08/14 21:15:19, frankf wrote: > typo Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/22857005/29001
Retried try job too often on mac_rel for step(s) telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/22857005/29001
Retried try job too often on mac_rel for step(s) telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/22857005/29001
Retried try job too often on mac_rel for step(s) telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/22857005/29001
Retried try job too often on mac_rel for step(s) telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/22857005/29001
Message was sent while issue was closed.
Change committed as 217959 |
