Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1412623009:
[Android] Add CLI for flashing devices. (Closed)
|
Created:
5 years, 1 month ago by rnephew (Reviews Here) Modified:
5 years ago CC:
chromium-reviews, klundberg+watch_chromium.org, mikecase+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Add CLI for flashing devices.
BUG=543257
Committed: https://crrev.com/838d3efd56b79320c49a4d226ba2c540e86d6f99
Cr-Commit-Position: refs/heads/master@{#362581}
Patch Set 1 #
Total comments: 11
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : move exit codes #
Total comments: 10
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Messages
Total messages: 31 (6 generated)
rnephew@google.com changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org, perezju@chromium.org, rnephew@google.com
looks good overall, just a nit and a couple of comments https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... File build/android/devil/android/tools/flash_device.py (right): https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... build/android/devil/android/tools/flash_device.py:6: sys.path.append(os.path.join('..', '..', '..')) nit: '..' -> os.pardir https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... build/android/devil/android/tools/flash_device.py:14: """Returns a list of devices. Not to fix in this issue, but I find it mildly annoying that we have to re-implement something like this every time we create a new command line tool, each time with a potentially slightly different behavior (I've done this too). So I wrote a small rant about it: crbug.com/553946 https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... build/android/devil/android/tools/flash_device.py:43: parser.add_argument('-d', '--device', help='Device to flash.') should this also support a device blacklist? and how should it interact with it? https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... build/android/devil/android/tools/flash_device.py:59: logging.critical('Device %s failed to flash.', str(device)) Use logging.exception, otherwise we swallow the reason that caused the problem and make it harder to diagnose when things go wrong.
https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... File build/android/devil/android/tools/flash_device.py (right): https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... build/android/devil/android/tools/flash_device.py:6: sys.path.append(os.path.join('..', '..', '..')) On 2015/11/10 11:59:25, perezju wrote: > nit: '..' -> os.pardir Done. https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... build/android/devil/android/tools/flash_device.py:14: """Returns a list of devices. On 2015/11/10 11:59:25, perezju wrote: > Not to fix in this issue, but I find it mildly annoying that we have to > re-implement something like this every time we create a new command line tool, > each time with a potentially slightly different behavior (I've done this too). > So I wrote a small rant about it: crbug.com/553946 Acknowledged. https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... build/android/devil/android/tools/flash_device.py:43: parser.add_argument('-d', '--device', help='Device to flash.') On 2015/11/10 11:59:25, perezju wrote: > should this also support a device blacklist? and how should it interact with it? Thats tricky. Should we only run if no device is black listed? If we run when devices are black listed we will be in a state where some devices are running different versions of android. https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... build/android/devil/android/tools/flash_device.py:59: logging.critical('Device %s failed to flash.', str(device)) On 2015/11/10 11:59:25, perezju wrote: > Use logging.exception, otherwise we swallow the reason that caused the problem > and make it harder to diagnose when things go wrong. Done.
On 2015/11/10 16:01:42, rnephew1 wrote: > https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... > File build/android/devil/android/tools/flash_device.py (right): > > https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... > build/android/devil/android/tools/flash_device.py:6: > sys.path.append(os.path.join('..', '..', '..')) > On 2015/11/10 11:59:25, perezju wrote: > > nit: '..' -> os.pardir > > Done. > > https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... > build/android/devil/android/tools/flash_device.py:14: """Returns a list of > devices. > On 2015/11/10 11:59:25, perezju wrote: > > Not to fix in this issue, but I find it mildly annoying that we have to > > re-implement something like this every time we create a new command line tool, > > each time with a potentially slightly different behavior (I've done this too). > > So I wrote a small rant about it: crbug.com/553946 > > Acknowledged. > > https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... > build/android/devil/android/tools/flash_device.py:43: parser.add_argument('-d', > '--device', help='Device to flash.') > On 2015/11/10 11:59:25, perezju wrote: > > should this also support a device blacklist? and how should it interact with > it? > > Thats tricky. Should we only run if no device is black listed? If we run when > devices are black listed we will be in a state where some devices are running > different versions of android. > > https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... > build/android/devil/android/tools/flash_device.py:59: logging.critical('Device > %s failed to flash.', str(device)) > On 2015/11/10 11:59:25, perezju wrote: > > Use logging.exception, otherwise we swallow the reason that caused the problem > > and make it harder to diagnose when things go wrong. > > Done. ping
https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... File build/android/devil/android/tools/__init__.py (right): https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... build/android/devil/android/tools/__init__.py:5: # This package is intended for tools that are very tightly coupled to ...? https://codereview.chromium.org/1412623009/diff/20001/build/android/devil/and... File build/android/devil/android/tools/__init__.py (right): https://codereview.chromium.org/1412623009/diff/20001/build/android/devil/and... build/android/devil/android/tools/__init__.py:6: # modules in build/android/devil/. What do you mean by this...? Also, I'd prefer if we didn't reference the build/android/ part, as that's liable to change in the (hopefully) near future. https://codereview.chromium.org/1412623009/diff/20001/build/android/devil/and... File build/android/devil/android/tools/flash_device.py (right): https://codereview.chromium.org/1412623009/diff/20001/build/android/devil/and... build/android/devil/android/tools/flash_device.py:58: for device in GetDeviceList(device=args.device): I would rework this to run in parallel across the devices using pMap from the parallelizer. Serially flashing everything would take a while...
https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... File build/android/devil/android/tools/__init__.py (right): https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... build/android/devil/android/tools/__init__.py:5: # This package is intended for tools that are very tightly coupled to On 2015/11/23 22:46:00, jbudorick wrote: > ...? er, leftover comment, refer to the other one
https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... File build/android/devil/android/tools/flash_device.py (right): https://codereview.chromium.org/1412623009/diff/1/build/android/devil/android... build/android/devil/android/tools/flash_device.py:43: parser.add_argument('-d', '--device', help='Device to flash.') On 2015/11/10 16:01:42, rnephew1 wrote: > On 2015/11/10 11:59:25, perezju wrote: > > should this also support a device blacklist? and how should it interact with > it? > > Thats tricky. Should we only run if no device is black listed? If we run when > devices are black listed we will be in a state where some devices are running > different versions of android. That sounds actually reasonable. Refuse to run if a non-empty blacklist is given.
https://codereview.chromium.org/1412623009/diff/20001/build/android/devil/and... File build/android/devil/android/tools/__init__.py (right): https://codereview.chromium.org/1412623009/diff/20001/build/android/devil/and... build/android/devil/android/tools/__init__.py:6: # modules in build/android/devil/. On 2015/11/23 22:46:00, jbudorick wrote: > What do you mean by this...? > > Also, I'd prefer if we didn't reference the build/android/ part, as that's > liable to change in the (hopefully) near future. Thinking more about it, I dont think this is needed since its stand alone tools that use things in devil, and not things that will be imported anywhere so it doesn't need an __init__.py https://codereview.chromium.org/1412623009/diff/20001/build/android/devil/and... File build/android/devil/android/tools/flash_device.py (right): https://codereview.chromium.org/1412623009/diff/20001/build/android/devil/and... build/android/devil/android/tools/flash_device.py:58: for device in GetDeviceList(device=args.device): On 2015/11/23 22:46:00, jbudorick wrote: > I would rework this to run in parallel across the devices using pMap from the > parallelizer. Serially flashing everything would take a while... Done.
lgtm w/nits https://codereview.chromium.org/1412623009/diff/40001/build/android/devil/and... File build/android/devil/android/tools/flash_device.py (right): https://codereview.chromium.org/1412623009/diff/40001/build/android/devil/and... build/android/devil/android/tools/flash_device.py:62: logging.critical('Device in blacklist, not flashing devices.') nit: maybe also list the serial's of the blacklisted devices https://codereview.chromium.org/1412623009/diff/40001/build/android/devil/and... build/android/devil/android/tools/flash_device.py:63: return nit: return a string with the failure reason, python will print it and make the script return a non-zero exit status https://codereview.chromium.org/1412623009/diff/40001/build/android/devil/and... build/android/devil/android/tools/flash_device.py:85: logging.critical(' %s', ' '.join(str(d) for d in failed_devices)) nit: return string with error if there were failed devices (so we also get non-zero exit status)
https://codereview.chromium.org/1412623009/diff/40001/build/android/devil/and... File build/android/devil/android/tools/flash_device.py (right): https://codereview.chromium.org/1412623009/diff/40001/build/android/devil/and... build/android/devil/android/tools/flash_device.py:62: logging.critical('Device in blacklist, not flashing devices.') On 2015/11/25 09:40:21, perezju wrote: > nit: maybe also list the serial's of the blacklisted devices Done. https://codereview.chromium.org/1412623009/diff/40001/build/android/devil/and... build/android/devil/android/tools/flash_device.py:63: return On 2015/11/25 09:40:21, perezju wrote: > nit: return a string with the failure reason, python will print it and make the > script return a non-zero exit status Do we want it to come up as a failure if it doesn't flash? If so, what kind of failure do we want the step to show? Infra? https://codereview.chromium.org/1412623009/diff/40001/build/android/devil/and... build/android/devil/android/tools/flash_device.py:85: logging.critical(' %s', ' '.join(str(d) for d in failed_devices)) On 2015/11/25 09:40:22, perezju wrote: > nit: return string with error if there were failed devices (so we also get > non-zero exit status) Done.
https://codereview.chromium.org/1412623009/diff/40001/build/android/devil/and... File build/android/devil/android/tools/flash_device.py (right): https://codereview.chromium.org/1412623009/diff/40001/build/android/devil/and... build/android/devil/android/tools/flash_device.py:63: return On 2015/11/25 15:24:35, rnephew1 wrote: > On 2015/11/25 09:40:21, perezju wrote: > > nit: return a string with the failure reason, python will print it and make > the > > script return a non-zero exit status > > Do we want it to come up as a failure if it doesn't flash? If so, what kind of > failure do we want the step to show? Infra? Those are good questions. I guess infra error is more appropriate. John, any thoughts?
https://codereview.chromium.org/1412623009/diff/40001/build/android/devil/and... File build/android/devil/android/tools/flash_device.py (right): https://codereview.chromium.org/1412623009/diff/40001/build/android/devil/and... build/android/devil/android/tools/flash_device.py:63: return On 2015/11/26 09:31:34, perezju wrote: > On 2015/11/25 15:24:35, rnephew1 wrote: > > On 2015/11/25 09:40:21, perezju wrote: > > > nit: return a string with the failure reason, python will print it and make > > the > > > script return a non-zero exit status > > > > Do we want it to come up as a failure if it doesn't flash? If so, what kind of > > failure do we want the step to show? Infra? > > Those are good questions. I guess infra error is more appropriate. John, any > thoughts? log an error and exit with an infra failure. It may be time to promote the exit codes from pylib/constants/__init__.py to devil/.
On 2015/11/30 18:10:13, jbudorick wrote: Now exiting with infra exit code in pylib.constants. Will move to the new one when it moves to devil.
On 2015/12/01 15:50:40, rnephew1 wrote: > On 2015/11/30 18:10:13, jbudorick wrote: > > Now exiting with infra exit code in pylib.constants. Will move to the new one > when it moves to devil. exit codes moved to devil/constants/exit_codes.py
On 2015/12/01 15:50:40, rnephew1 wrote: > On 2015/11/30 18:10:13, jbudorick wrote: > > Now exiting with infra exit code in pylib.constants. Will move to the new one > when it moves to devil. exit codes moved to devil/constants/exit_codes.py
https://codereview.chromium.org/1412623009/diff/100001/build/android/devil/an... File build/android/devil/android/tools/flash_device.py (right): https://codereview.chromium.org/1412623009/diff/100001/build/android/devil/an... build/android/devil/android/tools/flash_device.py:11: sys.path.append(os.path.join(os.path.dirname(os.path.abspath(__file__)), I don't think this shouldn't be necessary... https://codereview.chromium.org/1412623009/diff/100001/build/android/devil/an... build/android/devil/android/tools/flash_device.py:27: for d in adb_wrapper.AdbWrapper.GetDevices()] nit: one too many spaces after in https://codereview.chromium.org/1412623009/diff/100001/build/android/devil/an... build/android/devil/android/tools/flash_device.py:38: def SetLoggingLevel(level): devil.utils.run_tests_helper.SetLogLevel I really need to rename that module. https://codereview.chromium.org/1412623009/diff/100001/build/android/devil/an... build/android/devil/android/tools/flash_device.py:90: return 0 https://codereview.chromium.org/1412623009/diff/100001/build/android/devil/co... File build/android/devil/constants/exit_codes.py (right): https://codereview.chromium.org/1412623009/diff/100001/build/android/devil/co... build/android/devil/constants/exit_codes.py:7: ERROR_EXIT_CODE = 1 nit: just ERROR/INFRA/WARNING here. exit_codes.ERROR_EXIT_CODE is a bit redundant.
https://codereview.chromium.org/1412623009/diff/100001/build/android/devil/an... File build/android/devil/android/tools/flash_device.py (right): https://codereview.chromium.org/1412623009/diff/100001/build/android/devil/an... build/android/devil/android/tools/flash_device.py:11: sys.path.append(os.path.join(os.path.dirname(os.path.abspath(__file__)), On 2015/12/01 16:43:26, jbudorick wrote: > I don't think this shouldn't be necessary... Done. https://codereview.chromium.org/1412623009/diff/100001/build/android/devil/an... build/android/devil/android/tools/flash_device.py:27: for d in adb_wrapper.AdbWrapper.GetDevices()] On 2015/12/01 16:43:26, jbudorick wrote: > nit: one too many spaces after in Done. https://codereview.chromium.org/1412623009/diff/100001/build/android/devil/an... build/android/devil/android/tools/flash_device.py:38: def SetLoggingLevel(level): On 2015/12/01 16:43:26, jbudorick wrote: > devil.utils.run_tests_helper.SetLogLevel > > I really need to rename that module. Done. https://codereview.chromium.org/1412623009/diff/100001/build/android/devil/an... build/android/devil/android/tools/flash_device.py:90: On 2015/12/01 16:43:26, jbudorick wrote: > return 0 Done. https://codereview.chromium.org/1412623009/diff/100001/build/android/devil/co... File build/android/devil/constants/exit_codes.py (right): https://codereview.chromium.org/1412623009/diff/100001/build/android/devil/co... build/android/devil/constants/exit_codes.py:7: ERROR_EXIT_CODE = 1 On 2015/12/01 16:43:26, jbudorick wrote: > nit: just ERROR/INFRA/WARNING here. exit_codes.ERROR_EXIT_CODE is a bit > redundant. Done.
lgtm w/ nit https://codereview.chromium.org/1412623009/diff/120001/build/android/devil/an... File build/android/devil/android/tools/flash_device.py (right): https://codereview.chromium.org/1412623009/diff/120001/build/android/devil/an... build/android/devil/android/tools/flash_device.py:38: parser.add_argument('build_path') nit: add help text for build_path
https://codereview.chromium.org/1412623009/diff/120001/build/android/devil/an... File build/android/devil/android/tools/flash_device.py (right): https://codereview.chromium.org/1412623009/diff/120001/build/android/devil/an... build/android/devil/android/tools/flash_device.py:38: parser.add_argument('build_path') On 2015/12/01 20:21:26, jbudorick wrote: > nit: add help text for build_path Done.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1412623009/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412623009/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412623009/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rnephew@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412623009/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412623009/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [Android] Add CLI for flashing devices. BUG=543257 ========== to ========== [Android] Add CLI for flashing devices. BUG=543257 Committed: https://crrev.com/838d3efd56b79320c49a4d226ba2c540e86d6f99 Cr-Commit-Position: refs/heads/master@{#362581} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/838d3efd56b79320c49a4d226ba2c540e86d6f99 Cr-Commit-Position: refs/heads/master@{#362581} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
