Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1417373002:
[Android] Add functionality to flash devices. (Closed)
|
Created:
5 years, 2 months ago by rnephew (Reviews Here) Modified:
5 years, 1 month ago CC:
chromium-reviews, jbudorick+watch_chromium.org, klundberg+watch_chromium.org, yfriedman+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 functionality to flash devices.
This requires adding fastboot_utils to put a device into fastboot mode
and interact with it while in fastboot mode.
BUG=543257
Committed: https://crrev.com/536629676d896885ca26b05ba69f2fb8c4fad702
Cr-Commit-Position: refs/heads/master@{#357939}
Patch Set 1 #
Total comments: 69
Patch Set 2 : #Patch Set 3 : #
Total comments: 31
Patch Set 4 : #
Total comments: 17
Patch Set 5 : #
Total comments: 33
Patch Set 6 : #
Total comments: 10
Patch Set 7 : #Patch Set 8 : #
Total comments: 10
Patch Set 9 : #
Total comments: 6
Patch Set 10 : #
Total comments: 3
Messages
Total messages: 33 (4 generated)
rnephew@chromium.org changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org, perezju@chromium.org
https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/device_utils.py:2107: ['bootloader', 'radio', 'boot', 'recovery', 'system', 'userdata', name this list as a constant, e.g. fastbool_utils.ALL_PARTITIONS. Also, do you expect clients wanting to call this with anything other than the full list of partitions? If that does not seem a common case, I would make the list of partitions optional and the default to flash all partitions. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:71: def EnableFastbootMode(self, timeout=None, retries=None): is this missing a timeout/retries decorator? https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:80: def Reboot(self, bootloader=False, timeout=None, retries=None): also missing a timeout/retries decorator? https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:83: It reboots the phone either back into fastboot, or to a regular boot. nit: Mention in the comment that this blocks until the device is ready. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:103: force: Boolean on if it should raise an exception if failing checks. The name 'force' is very misleading, first I thought it would mean to "force the verification to pass". When it actually means the opposite (and I only understood that after reading the FlashPartitions method below). I would suggest to remove the flag from this private method and, instead, make it return True/False with the result of the verification. The caller would then decide whether or not to raise an exception. This would also help to simplify the flow of the code. See my comments below. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:111: verified = True just 'return True' here and avoid the "for .. else" construct, i.e.: with open(FOO) as f: for word in BAR: if self._board == word: return True logging.warning('Board name not found ...') Also wondering. Might be more efficient/readable, to compile a regex to match '\b%s\b' % re.escape(self._board), and then just iterate over lines in f trying to match the pattern? This also avoids reading the whole file into memory, split into a huge array, etc. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:121: verified = True Same here, just return True and avoid the "for .. else" thingy. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:126: if not verified and not force: this just becomes return False https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:130: def _VerifyPartitionsAndImages(self, partitions, directory): This method seems to be doing more that "verifying" the partition names; it's actually finding the image partition files? https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:148: 'Failed to flash %s. Unknown partition name.' % part) I can't imagine sending a bad partition name being a common error. I would just let it raise a KeyError in the code below. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:157: 'Failed to flash %s, img not found in %s' % (part, directory)) Feel free to ignore, but I think this would be more readable as something like: files = os.lisdir(directory) def find_file(pattern): for filename in files: if fnmatch.fnmatch(filename, pattern): return filename raise ... return {name: find_file(self._FLASH_IMAGE_FILES[name]) for name in partitions)} https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:178: for part in partitions: nit: spell it out "part" -> "partition" also, would it be better to iterate over the flash_image_files; i.e. for partition, filename in flash_image_files.iteritems(): ... (maybe not if the order is important?) https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:179: if not wipe and part in ['cache', 'userdata']: nit: I think "if part in [...] and not wipe" is more readable. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:209: self.SetOemOffModeCharge(False) I think these two lines should go before the "try". https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:210: yield nit: yield self --- so you return something useful. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:213: self.Reboot() Double-checking, do we really want to reboot the device if we're bailing out due to an exception during the context managed code? https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... File build/android/devil/android/fastboot_utils_test.py (right): https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils_test.py:89: class FastbootUtilsInitTest(FastbootUtilsTest): Why are there so many test classes? I would have a single test class for each class we're testing. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils_test.py:106: self.fastboot.WaitForFastbootMode() what is this testing? https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils_test.py:187: pass Add a test for what happens when an exception is raised in the context managed code. And another for when an exception is raised, say, during EnableFastbootMode. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils_test.py:200: self.fastboot._VerifyBoard('test') don't assert that the builtin open is called. Just assert that the verification succeeds. Test the functionality, not the implementation! assertCalls should be used *very* sparingly, only for when the job of a method is essentially to translate high-level actions into a sequence of lower-level method calls (for example in wrappers like DeviceUtils mostly do). https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils_test.py:266: 'bootloader': 'test/bootloader.img', try some file names where the use of fnmatch is a bit more interesting :) https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... File build/android/devil/android/sdk/fastboot_wrapper.py (right): https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/sdk/fastboot_wrapper.py:42: constants.ANDROID_SDK_ROOT, 'platform-tools', 'fastboot') make this a class/module constant? https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/sdk/fastboot_wrapper.py:45: elif type(cmd) == str: If it's not strictly necessary, can we disable the option of passing a string and enforce that cmd is always a list? Otherwise we make it easy for clients to pass strings with badly quoted args. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/sdk/fastboot_wrapper.py:50: return cmd_helper.GetCmdStatusAndOutput(cmd) maybe get the status here, raise an exception if not zero, and return the output otherwise?
https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/device_utils.py:2096: Directory Must contain bootloader, radio, boot, recovery, system, userdata, nit :s/Must/must/ https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/device_utils.py:2102: wipe: Wipes cache and userdata if set to true. wipe variable doesnt appear to be used in this function. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. nit: :s/2015/2014 https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:45: default_timeout: An integer containing the default number of seconds to Consider adding documentation for the fastboot arg. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:71: def EnableFastbootMode(self, timeout=None, retries=None): Also, not sure EnableFastbootMode is the best name since it's less enabling fastboot and more going into fastboot mode. How about... RebootToFastbootMode? thats probably an equally bad name :/ https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:190: """Toggles off mode charging nit: :s/Toggles/Sets https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... File build/android/devil/android/fastboot_utils_test.py (right): https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils_test.py:2: # Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015
https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/device_utils.py:2096: Directory Must contain bootloader, radio, boot, recovery, system, userdata, On 2015/10/23 14:30:14, mikecase wrote: > nit > :s/Must/must/ Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/device_utils.py:2102: wipe: Wipes cache and userdata if set to true. On 2015/10/23 14:30:14, mikecase wrote: > wipe variable doesnt appear to be used in this function. Its suppose to pass through to FlashPartitions, I forgot to do that. Fixed. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/device_utils.py:2107: ['bootloader', 'radio', 'boot', 'recovery', 'system', 'userdata', On 2015/10/23 09:42:53, perezju wrote: > name this list as a constant, e.g. fastbool_utils.ALL_PARTITIONS. > > Also, do you expect clients wanting to call this with anything other than the > full list of partitions? If that does not seem a common case, I would make the > list of partitions optional and the default to flash all partitions. Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2015/10/23 14:30:14, mikecase wrote: > nit: :s/2015/2014 Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:45: default_timeout: An integer containing the default number of seconds to On 2015/10/23 14:30:14, mikecase wrote: > Consider adding documentation for the fastboot arg. Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:71: def EnableFastbootMode(self, timeout=None, retries=None): On 2015/10/23 09:42:53, perezju wrote: > is this missing a timeout/retries decorator? Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:71: def EnableFastbootMode(self, timeout=None, retries=None): On 2015/10/23 14:30:14, mikecase wrote: > Also, not sure EnableFastbootMode is the best name since it's less enabling > fastboot and more going into fastboot mode. > > How about... > > RebootToFastbootMode? > > thats probably an equally bad name :/ Naming is hard. If anyone else wants to chime in on either of those two names, I'll go with whatever group consensus is. They both get the meaning of the function across which is really the only important part. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:80: def Reboot(self, bootloader=False, timeout=None, retries=None): On 2015/10/23 09:42:54, perezju wrote: > also missing a timeout/retries decorator? Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:83: It reboots the phone either back into fastboot, or to a regular boot. On 2015/10/23 09:42:53, perezju wrote: > nit: Mention in the comment that this blocks until the device is ready. Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:103: force: Boolean on if it should raise an exception if failing checks. On 2015/10/23 09:42:54, perezju wrote: > The name 'force' is very misleading, first I thought it would mean to "force the > verification to pass". When it actually means the opposite (and I only > understood that after reading the FlashPartitions method below). > > I would suggest to remove the flag from this private method and, instead, make > it return True/False with the result of the verification. The caller would then > decide whether or not to raise an exception. > > This would also help to simplify the flow of the code. See my comments below. Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:111: verified = True On 2015/10/23 09:42:54, perezju wrote: > just 'return True' here and avoid the "for .. else" construct, i.e.: > > with open(FOO) as f: > for word in BAR: > if self._board == word: > return True > logging.warning('Board name not found ...') > > Also wondering. Might be more efficient/readable, to compile a regex to match > '\b%s\b' % re.escape(self._board), and then just iterate over lines in f trying > to match the pattern? This also avoids reading the whole file into memory, split > into a huge array, etc. Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:121: verified = True On 2015/10/23 09:42:53, perezju wrote: > Same here, just return True and avoid the "for .. else" thingy. Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:126: if not verified and not force: On 2015/10/23 09:42:54, perezju wrote: > this just becomes return False Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:130: def _VerifyPartitionsAndImages(self, partitions, directory): On 2015/10/23 09:42:54, perezju wrote: > This method seems to be doing more that "verifying" the partition names; it's > actually finding the image partition files? Its doing both. I changed the name to reflect that. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:148: 'Failed to flash %s. Unknown partition name.' % part) On 2015/10/23 09:42:54, perezju wrote: > I can't imagine sending a bad partition name being a common error. I would just > let it raise a KeyError in the code below. Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:157: 'Failed to flash %s, img not found in %s' % (part, directory)) On 2015/10/23 09:42:54, perezju wrote: > Feel free to ignore, but I think this would be more readable as something like: > > files = os.lisdir(directory) > > def find_file(pattern): > for filename in files: > if fnmatch.fnmatch(filename, pattern): > return filename > raise ... > > return {name: find_file(self._FLASH_IMAGE_FILES[name]) for name in partitions)} Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:178: for part in partitions: On 2015/10/23 09:42:53, perezju wrote: > nit: spell it out "part" -> "partition" > > also, would it be better to iterate over the flash_image_files; i.e. > > for partition, filename in flash_image_files.iteritems(): > ... > > (maybe not if the order is important?) There is no guarantee that partitions will contain everything in the dictionary. The dictionary comes pre-populated with the regexes for the images of all known partitions. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:179: if not wipe and part in ['cache', 'userdata']: On 2015/10/23 09:42:54, perezju wrote: > nit: I think "if part in [...] and not wipe" is more readable. Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:190: """Toggles off mode charging On 2015/10/23 14:30:14, mikecase wrote: > nit: > > :s/Toggles/Sets Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:209: self.SetOemOffModeCharge(False) On 2015/10/23 09:42:54, perezju wrote: > I think these two lines should go before the "try". Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:210: yield On 2015/10/23 09:42:54, perezju wrote: > nit: yield self --- so you return something useful. Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:213: self.Reboot() On 2015/10/23 09:42:54, perezju wrote: > Double-checking, do we really want to reboot the device if we're bailing out due > to an exception during the context managed code? Yes, otherwise we will leave the device in fastboot mode. ADB cannot see devices in that state. All the checks for partitions being correct and the device board being correct should be the only places that trigger an exception unless something has gone very wrong; thus why we want the device rebooted into normal mode if we see one. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... File build/android/devil/android/fastboot_utils_test.py (right): https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils_test.py:2: # Copyright 2014 The Chromium Authors. All rights reserved. On 2015/10/23 14:30:14, mikecase wrote: > nit: 2015 Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils_test.py:89: class FastbootUtilsInitTest(FastbootUtilsTest): On 2015/10/23 09:42:54, perezju wrote: > Why are there so many test classes? I would have a single test class for each > class we're testing. Its set up in the same way device_utils_test is set up. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils_test.py:106: self.fastboot.WaitForFastbootMode() On 2015/10/23 09:42:54, perezju wrote: > what is this testing? The _FastbotoWrapperMock does all the setup that is required for the unit test. (fastboot.Devices.return_value = [test_serial]) https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils_test.py:187: pass On 2015/10/23 09:42:54, perezju wrote: > Add a test for what happens when an exception is raised in the context managed > code. And another for when an exception is raised, say, during > EnableFastbootMode. Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils_test.py:200: self.fastboot._VerifyBoard('test') On 2015/10/23 09:42:54, perezju wrote: > don't assert that the builtin open is called. Just assert that the verification > succeeds. Test the functionality, not the implementation! > > assertCalls should be used *very* sparingly, only for when the job of a method > is essentially to translate high-level actions into a sequence of lower-level > method calls (for example in wrappers like DeviceUtils mostly do). Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils_test.py:266: 'bootloader': 'test/bootloader.img', On 2015/10/23 09:42:54, perezju wrote: > try some file names where the use of fnmatch is a bit more interesting :) Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... File build/android/devil/android/sdk/fastboot_wrapper.py (right): https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/sdk/fastboot_wrapper.py:42: constants.ANDROID_SDK_ROOT, 'platform-tools', 'fastboot') On 2015/10/23 09:42:54, perezju wrote: > make this a class/module constant? Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/sdk/fastboot_wrapper.py:45: elif type(cmd) == str: On 2015/10/23 09:42:54, perezju wrote: > If it's not strictly necessary, can we disable the option of passing a string > and enforce that cmd is always a list? Otherwise we make it easy for clients to > pass strings with badly quoted args. Done. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/sdk/fastboot_wrapper.py:50: return cmd_helper.GetCmdStatusAndOutput(cmd) On 2015/10/23 09:42:54, perezju wrote: > maybe get the status here, raise an exception if not zero, and return the output > otherwise? Done.
Thanks for the fixes. A few more comments. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:71: def EnableFastbootMode(self, timeout=None, retries=None): On 2015/10/23 17:19:35, rnephew1 wrote: > On 2015/10/23 09:42:53, perezju wrote: > > is this missing a timeout/retries decorator? > > Done. +1 for RebootToFastbootMode. the "Enable-" prefix makes it sound like there should also be a "DisableFastbootMode" which does not make that much sense (I think). https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:178: for part in partitions: On 2015/10/23 17:19:35, rnephew1 wrote: > On 2015/10/23 09:42:53, perezju wrote: > > nit: spell it out "part" -> "partition" > > > > also, would it be better to iterate over the flash_image_files; i.e. > > > > for partition, filename in flash_image_files.iteritems(): > > ... > > > > (maybe not if the order is important?) > > There is no guarantee that partitions will contain everything in the dictionary. > The dictionary comes pre-populated with the regexes for the images of all known > partitions. Acknowledged. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils.py:213: self.Reboot() On 2015/10/23 17:19:35, rnephew1 wrote: > On 2015/10/23 09:42:54, perezju wrote: > > Double-checking, do we really want to reboot the device if we're bailing out > due > > to an exception during the context managed code? > > Yes, otherwise we will leave the device in fastboot mode. ADB cannot see devices > in that state. All the checks for partitions being correct and the device board > being correct should be the only places that trigger an exception unless > something has gone very wrong; thus why we want the device rebooted into normal > mode if we see one. Acknowledged. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... File build/android/devil/android/fastboot_utils_test.py (right): https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils_test.py:89: class FastbootUtilsInitTest(FastbootUtilsTest): On 2015/10/23 17:19:36, rnephew1 wrote: > On 2015/10/23 09:42:54, perezju wrote: > > Why are there so many test classes? I would have a single test class for each > > class we're testing. > > Its set up in the same way device_utils_test is set up. Yeah. Don't know what I was thinking. Ignore me. https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils_test.py:106: self.fastboot.WaitForFastbootMode() On 2015/10/23 17:19:35, rnephew1 wrote: > On 2015/10/23 09:42:54, perezju wrote: > > what is this testing? > > The _FastbotoWrapperMock does all the setup that is required for the unit test. > > (fastboot.Devices.return_value = [test_serial]) I still don't get it. I don't see any asserts in the control flow of this test case. Could you add a comment explaining what behavior is this testing? https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils_test.py:187: pass On 2015/10/23 17:19:35, rnephew1 wrote: > On 2015/10/23 09:42:54, perezju wrote: > > Add a test for what happens when an exception is raised in the context managed > > code. And another for when an exception is raised, say, during > > EnableFastbootMode. > > Done. Nice. Thanks! https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/device_utils.py:2095: partitions=fastboot_utils.ALL_PARITIONS): let partitions=None instead and, in the body of the function, set it to the default value if None. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/device_utils.py:2104: wipe: Wipes cache and userdata if set to true. missing doc for 'partitions' arg https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:95: def Reboot(self, bootloader=False, timeout=None, retries=None): Probably not something to address in the CL, but trying to reason about the different timeout values in this method, and the effects that retries will produce at different points, really makes my head hurt. We should really think of something better at some point. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:134: # pylint: disable=useless-else-on-loop don't disable the warning. Just remove the "else" keyword that isn't really needed here. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:163: self._FLASH_IMAGE_FILES[name]) for name in partitions} nit: I would probably break the line as: return {key: value for thing in sequence} https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:179: if not force and not self._VerifyBoard(directory): maybe you want to switch the order, so that you get warnings even if force is True. I might even do something like: if not self._VerifyBoard(directory): if force: logging.warning('Unable to verify but flashing anyway. Dangerous!') else: raise ... https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... File build/android/devil/android/fastboot_utils_test.py (right): https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/fastboot_utils_test.py:217: self.fastboot._VerifyBoard('test') missing an assertTrue? https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/fastboot_utils_test.py:243: self.fastboot._VerifyBoard('test') ditto https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/fastboot_utils_test.py:252: mock_listdir.return_value = files ditto https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/fastboot_utils_test.py:263: self.fastboot._VerifyBoard('test') ditto https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... File build/android/devil/android/sdk/fastboot_wrapper.py (right): https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:21: nit: I think this empty line does not go here. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:37: cmd: Command to run. mention that cmd must be a list of args. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:40: Tuple of (return_code, output) update docstring https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:49: 'Commands for RunFastbootCommand must be a list.') nit: 'Command for RunFastbootCommand must be given as a list.' https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:52: raise Exception raise something more specific. e.g. https://code.google.com/p/chromium/codesearch#chromium/src/build/android/devi...
https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... File build/android/devil/android/fastboot_utils_test.py (right): https://codereview.chromium.org/1417373002/diff/1/build/android/devil/android... build/android/devil/android/fastboot_utils_test.py:106: self.fastboot.WaitForFastbootMode() On 2015/10/26 10:36:09, perezju wrote: > On 2015/10/23 17:19:35, rnephew1 wrote: > > On 2015/10/23 09:42:54, perezju wrote: > > > what is this testing? > > > > The _FastbotoWrapperMock does all the setup that is required for the unit > test. > > > > (fastboot.Devices.return_value = [test_serial]) > > I still don't get it. I don't see any asserts in the control flow of this test > case. Could you add a comment explaining what behavior is this testing? Sure, if the test times out (timeout for the test is 1 second, no retries are set) then it will fail. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/device_utils.py:2095: partitions=fastboot_utils.ALL_PARITIONS): On 2015/10/26 10:36:09, perezju wrote: > let partitions=None instead and, in the body of the function, set it to the > default value if None. Done. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/device_utils.py:2104: wipe: Wipes cache and userdata if set to true. On 2015/10/26 10:36:09, perezju wrote: > missing doc for 'partitions' arg Done. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:95: def Reboot(self, bootloader=False, timeout=None, retries=None): On 2015/10/26 10:36:09, perezju wrote: > Probably not something to address in the CL, but trying to reason about the > different timeout values in this method, and the effects that retries will > produce at different points, really makes my head hurt. We should really think > of something better at some point. I'm not 100% convinced having them have timeout/retries is correct. If it times out after rebooting, it cannot really attempt to do it again properly, since it will no longer be in fastboot mode. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:134: # pylint: disable=useless-else-on-loop On 2015/10/26 10:36:09, perezju wrote: > don't disable the warning. Just remove the "else" keyword that isn't really > needed here. Done. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:163: self._FLASH_IMAGE_FILES[name]) for name in partitions} On 2015/10/26 10:36:09, perezju wrote: > nit: I would probably break the line as: > > return {key: value > for thing in sequence} Done. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:179: if not force and not self._VerifyBoard(directory): On 2015/10/26 10:36:09, perezju wrote: > maybe you want to switch the order, so that you get warnings even if force is > True. I might even do something like: > > if not self._VerifyBoard(directory): > if force: > logging.warning('Unable to verify but flashing anyway. Dangerous!') > else: > raise ... Done. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... File build/android/devil/android/fastboot_utils_test.py (right): https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/fastboot_utils_test.py:217: self.fastboot._VerifyBoard('test') On 2015/10/26 10:36:09, perezju wrote: > missing an assertTrue? Yep, and not having that was letting some problems with the regex slip through. \bboard\b was not working, but switching it to .*board.* does. Done. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/fastboot_utils_test.py:243: self.fastboot._VerifyBoard('test') On 2015/10/26 10:36:09, perezju wrote: > ditto Done. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/fastboot_utils_test.py:252: mock_listdir.return_value = files On 2015/10/26 10:36:09, perezju wrote: > ditto Done. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/fastboot_utils_test.py:263: self.fastboot._VerifyBoard('test') On 2015/10/26 10:36:09, perezju wrote: > ditto Done. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... File build/android/devil/android/sdk/fastboot_wrapper.py (right): https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:21: On 2015/10/26 10:36:09, perezju wrote: > nit: I think this empty line does not go here. Done. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:37: cmd: Command to run. On 2015/10/26 10:36:09, perezju wrote: > mention that cmd must be a list of args. Done. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:40: Tuple of (return_code, output) On 2015/10/26 10:36:09, perezju wrote: > update docstring Done. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:49: 'Commands for RunFastbootCommand must be a list.') On 2015/10/26 10:36:09, perezju wrote: > nit: 'Command for RunFastbootCommand must be given as a list.' Done. https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:52: raise Exception On 2015/10/26 10:36:09, perezju wrote: > raise something more specific. e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/devi... Done.
Pretty much just some nits. https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:52: device: A device Utils instance. nit: s/device Utils/DeviceUtils https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:96: """ Reboots out of fastboot mode. super nit: Extra space after """ https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:115: self._BOARD_VERIFICATION_FILE or in the build archive. nit: I think this should be written as |self._BOARD_VERIFICATION_FILE| https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:121: board_regex = re.compile('.*board=%s.*' % re.escape(self._board)) Would probably change the regex to be more exact. I think you can have 'require board=%s' % re.escape(self._board) Alternatively, you could have the regex be 'require board=(.*)' and then you could return the board name. This would let you print an error message that was like... device is %s board but build is meant for %s board https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:132: if f.endswith('.zip') and self._board in f: Should you do a regex match on the file name instead of self._board in f to make it more exact? https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:179: 'Flashing device. Possibly dangerous operation.') Nit: Maybe an easier to understand error msg would be.... "Could not verify build is meant to be installed on device's board type." instead of "Unable to verify board type" your choice https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:182: 'Unable to verify board type. Run again with force=True to force ' Nit: Same as above https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:199: def SetOemOffModeCharge(self, value, timeout=None, retries=None): Nit: Should this be in fastboot_wrapper.py since it is just a single call to RunFastbootCommand?
https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:52: device: A device Utils instance. On 2015/10/26 20:30:15, mikecase wrote: > nit: s/device Utils/DeviceUtils Done. https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:96: """ Reboots out of fastboot mode. On 2015/10/26 20:30:15, mikecase wrote: > super nit: Extra space after """ Done. https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:115: self._BOARD_VERIFICATION_FILE or in the build archive. On 2015/10/26 20:30:16, mikecase wrote: > nit: I think this should be written as |self._BOARD_VERIFICATION_FILE| Done. https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:121: board_regex = re.compile('.*board=%s.*' % re.escape(self._board)) On 2015/10/26 20:30:16, mikecase wrote: > Would probably change the regex to be more exact. > > I think you can have > > 'require board=%s' % re.escape(self._board) > > Alternatively, you could have the regex be > 'require board=(.*)' and then you could return the > board name. This would let you print an error message that was like... > > device is %s board but build is meant for %s board Done. https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:132: if f.endswith('.zip') and self._board in f: On 2015/10/26 20:30:15, mikecase wrote: > Should you do a regex match on the file name instead of self._board in f to make > it more exact? Done. https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:179: 'Flashing device. Possibly dangerous operation.') On 2015/10/26 20:30:15, mikecase wrote: > Nit: Maybe an easier to understand error msg would be.... > > "Could not verify build is meant to be installed on device's board type." > instead of "Unable to verify board type" > > your choice Done. https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:182: 'Unable to verify board type. Run again with force=True to force ' On 2015/10/26 20:30:16, mikecase wrote: > Nit: Same as above Done. https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:199: def SetOemOffModeCharge(self, value, timeout=None, retries=None): On 2015/10/26 20:30:16, mikecase wrote: > Nit: Should this be in fastboot_wrapper.py since it is just a single call to > RunFastbootCommand? I thought about that, but I thought it should be exposed at this level so users of fastboot_utils can set it if they need to for some reason.
Everything lgtm but I have 1 final question https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/device_utils.py:2109: time.sleep(5) Is this necessary? The fastboot.FastbootMode() calls WaitForFastbootMode. Does this sleep always need to happen? Should it go in WaitForFastbootMode? Sorry if this was already asked.
https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/device_utils.py:523: self.adb.Reboot(to_bootloader=to_bootloader) Does this work with WaitUntilFullyBooted...? https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/device_utils.py:2097: and cache .img files from an android build. This is a dangerous operation so O_O https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/device_utils.py:2107: fastboot = fastboot_utils.FastbootUtils(self) DeviceUtils and FastbootUtils shouldn't cross-reference. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/device_utils.py:2109: time.sleep(5) On 2015/10/27 03:29:10, mikecase wrote: > Is this necessary? The fastboot.FastbootMode() calls WaitForFastbootMode. Does > this sleep always need to happen? Should it go in WaitForFastbootMode? Sorry if > this was already asked. +1, this should be handled with something other than a sleep if at all possible. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:79: time.sleep(self._FASTBOOT_WAIT_TIME) 5 seconds is a long interval between checking. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:186: 'Flashing device. Possibly dangerous operation.') This should have some sort of confirmation dialog that is only bypassed if we pass something like --force. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... File build/android/devil/android/sdk/fastboot_wrapper.py (right): https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:21: class FastbootWrapper(object): Just name this Fastboot. (I wish I had named AdbWrapper simply Adb) https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:33: def RunFastbootCommand(self, cmd): This seems like a bit too much of a catchall. Individual fastboot commands should have their own function. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:52: raise device_errors.AdbCommandFailedError( This should raise its own error type (FastbootCommandFailedError?). Adb isn't involved.
rnephew@google.com changed reviewers: + rnephew@google.com
Will implement these actual things on the bus tomorrow morning. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/device_utils.py:523: self.adb.Reboot(to_bootloader=to_bootloader) On 2015/10/27 03:57:24, jbudorick wrote: > Does this work with WaitUntilFullyBooted...? It does not, I will add a check that if both of them are set it will output a warning and not wait until it is fully rebooted. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/device_utils.py:2097: and cache .img files from an android build. This is a dangerous operation so On 2015/10/27 03:57:24, jbudorick wrote: > O_O I wanted to make it very clear that you can mess up a device doing this; the board type checks mitigate some of the danger; but if someone unplugs a device mid run, I'm sure the device will not be happy. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/device_utils.py:2107: fastboot = fastboot_utils.FastbootUtils(self) On 2015/10/27 03:57:24, jbudorick wrote: > DeviceUtils and FastbootUtils shouldn't cross-reference. I'll have it take a fastbootutils instance for the communication then. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/device_utils.py:2109: time.sleep(5) On 2015/10/27 03:29:10, mikecase wrote: > Is this necessary? The fastboot.FastbootMode() calls WaitForFastbootMode. Does > this sleep always need to happen? Should it go in WaitForFastbootMode? Sorry if > this was already asked. Thats just a legacy thing from testing, I was sleeping for 5 seconds instead of flashing testing the context manager. I'll get rid of it. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:79: time.sleep(self._FASTBOOT_WAIT_TIME) On 2015/10/27 03:57:25, jbudorick wrote: > 5 seconds is a long interval between checking. Thats roughly how long it takes to come back, I'll lower it to 1 second but it wont speed it up any. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:186: 'Flashing device. Possibly dangerous operation.') On 2015/10/27 03:57:25, jbudorick wrote: > This should have some sort of confirmation dialog that is only bypassed if we > pass something like --force. So instead of just having it throw an exception, have it ask if you want to continue? https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... File build/android/devil/android/sdk/fastboot_wrapper.py (right): https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:21: class FastbootWrapper(object): On 2015/10/27 03:57:25, jbudorick wrote: > Just name this Fastboot. (I wish I had named AdbWrapper simply Adb) Acknowledged. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:33: def RunFastbootCommand(self, cmd): On 2015/10/27 03:57:25, jbudorick wrote: > This seems like a bit too much of a catchall. Individual fastboot commands > should have their own function. I'll make this internal and do that. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:52: raise device_errors.AdbCommandFailedError( On 2015/10/27 03:57:25, jbudorick wrote: > This should raise its own error type (FastbootCommandFailedError?). Adb isn't > involved. Acknowledged.
Just a few more comments. This is looking good! https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/40001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:95: def Reboot(self, bootloader=False, timeout=None, retries=None): On 2015/10/26 18:20:21, rnephew1 wrote: > On 2015/10/26 10:36:09, perezju wrote: > > Probably not something to address in the CL, but trying to reason about the > > different timeout values in this method, and the effects that retries will > > produce at different points, really makes my head hurt. We should really think > > of something better at some point. > > I'm not 100% convinced having them have timeout/retries is correct. If it times > out after rebooting, it cannot really attempt to do it again properly, since it > will no longer be in fastboot mode. If you also have concerns, maybe it's best not to decorate this method with the timeout/retries at all. Just make sure that individual commands called from here have suitable timeouts (and retry whenever it makes sense to do so). What do others think? https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/60001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:199: def SetOemOffModeCharge(self, value, timeout=None, retries=None): On 2015/10/26 21:34:08, rnephew1 wrote: > On 2015/10/26 20:30:16, mikecase wrote: > > Nit: Should this be in fastboot_wrapper.py since it is just a single call to > > RunFastbootCommand? > > I thought about that, but I thought it should be exposed at this level so users > of fastboot_utils can set it if they need to for some reason. I would let it live in fastboot.py, and let fastboot_utils expose the reference to the fastboot object (like DeviceUtils exposes the adb property). https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:121: board_regex = re.compile(r'require board=(.*)') just checking, maybe the group should better match '(\w+)'? https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:131: self._board) nit: maybe re-organize as: m = board_regex.match(line) if m: board_name = m.group(1) if board_name == self._board: ... else: ... https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... File build/android/devil/android/fastboot_utils_test.py (right): https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/fastboot_utils_test.py:246: mock_file = io.StringIO(u'abc') maybe test these with: u'require_board=WrongBoard\n'
https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/device_utils.py:523: self.adb.Reboot(to_bootloader=to_bootloader) On 2015/10/27 03:57:24, jbudorick wrote: > Does this work with WaitUntilFullyBooted...? Done. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/device_utils.py:2107: fastboot = fastboot_utils.FastbootUtils(self) On 2015/10/27 03:57:24, jbudorick wrote: > DeviceUtils and FastbootUtils shouldn't cross-reference. Done. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/device_utils.py:2109: time.sleep(5) On 2015/10/27 03:29:10, mikecase wrote: > Is this necessary? The fastboot.FastbootMode() calls WaitForFastbootMode. Does > this sleep always need to happen? Should it go in WaitForFastbootMode? Sorry if > this was already asked. Done. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:79: time.sleep(self._FASTBOOT_WAIT_TIME) On 2015/10/27 03:57:25, jbudorick wrote: > 5 seconds is a long interval between checking. Done. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:121: board_regex = re.compile(r'require board=(.*)') On 2015/10/27 10:51:54, perezju wrote: > just checking, maybe the group should better match '(\w+)'? Done. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:131: self._board) On 2015/10/27 10:51:54, perezju wrote: > nit: maybe re-organize as: > > m = board_regex.match(line) > if m: > board_name = m.group(1) > if board_name == self._board: > ... > else: > ... Done. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/fastboot_utils.py:186: 'Flashing device. Possibly dangerous operation.') On 2015/10/27 04:49:03, rnephew wrote: > On 2015/10/27 03:57:25, jbudorick wrote: > > This should have some sort of confirmation dialog that is only bypassed if we > > pass something like --force. > > So instead of just having it throw an exception, have it ask if you want to > continue? Offline talk, nothing needed. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... File build/android/devil/android/fastboot_utils_test.py (right): https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/fastboot_utils_test.py:246: mock_file = io.StringIO(u'abc') On 2015/10/27 10:51:54, perezju wrote: > maybe test these with: u'require_board=WrongBoard\n' Done. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... File build/android/devil/android/sdk/fastboot_wrapper.py (right): https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:21: class FastbootWrapper(object): On 2015/10/27 03:57:25, jbudorick wrote: > Just name this Fastboot. (I wish I had named AdbWrapper simply Adb) Done. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:33: def RunFastbootCommand(self, cmd): On 2015/10/27 04:49:03, rnephew wrote: > On 2015/10/27 03:57:25, jbudorick wrote: > > This seems like a bit too much of a catchall. Individual fastboot commands > > should have their own function. > > I'll make this internal and do that. Done. https://codereview.chromium.org/1417373002/diff/80001/build/android/devil/and... build/android/devil/android/sdk/fastboot_wrapper.py:52: raise device_errors.AdbCommandFailedError( On 2015/10/27 03:57:25, jbudorick wrote: > This should raise its own error type (FastbootCommandFailedError?). Adb isn't > involved. Done.
https://codereview.chromium.org/1417373002/diff/100001/build/android/devil/an... File build/android/devil/android/device_errors.py (right): https://codereview.chromium.org/1417373002/diff/100001/build/android/devil/an... build/android/devil/android/device_errors.py:23: class FastbootCommandFailedError(CommandFailedError): Could we reuse the implementation of AdbCommandFailed error, maybe through a new parent class, instead of duplicating all of the code? https://codereview.chromium.org/1417373002/diff/100001/build/android/devil/an... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1417373002/diff/100001/build/android/devil/an... build/android/devil/android/device_utils.py:529: block = False I think what this reveals is that the to_bootloader option should not be added to DeviceUtils.Reboot. The 'block' option (wait until the device is ready for me to continue poking with it) clearly makes sense together with the "and reboot to bootloader mode, plz". Of course the waiting should not be done with WaitUntilFullyBooted, but through FastbootUtils.WaitForFastbootMode. Which means, I think, that the Reboot method in FastbootUtils should directly call adb.Reboot with suitable args instead. Conceptually we should have (I think): - DeviceUtils.Reboot - always boots to "normal" mode - FastbootUtils.Reboot - always boots to "fastboot" mode would that make more sense? https://codereview.chromium.org/1417373002/diff/100001/build/android/devil/an... build/android/devil/android/device_utils.py:2111: if not isinstance(fastboot, fastboot_utils.FastbootUtils): I don't think this is solving the issue. What we should have is device_utils not importing fastboot_utils at all. Same as above, I think the solution is not to add any knowledge about fastboot/bootloader stuff in device_utils; and leave all of that to fastboot_utils. If I want to wipe a device I should be able to do, in fact, just something like: device = device_utils.DeviceUtils(...) fastboot_utils.FlashDevice(device, ...) https://codereview.chromium.org/1417373002/diff/100001/build/android/devil/an... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/100001/build/android/devil/an... build/android/devil/android/fastboot_utils.py:132: 'reports a board type of %s', board_name.group(1), I think this one should just be 'board_name' now. https://codereview.chromium.org/1417373002/diff/100001/build/android/devil/an... build/android/devil/android/fastboot_utils.py:209: def SetOemOffModeCharge(self, value, timeout=None, retries=None): Do we really expect clients calling this directly? Otherwise do not expose it and always call self.fastboot.SetOemOffModeCharge when needed.
https://codereview.chromium.org/1417373002/diff/100001/build/android/devil/an... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1417373002/diff/100001/build/android/devil/an... build/android/devil/android/device_utils.py:529: block = False On 2015/10/28 15:28:58, perezju wrote: > I think what this reveals is that the to_bootloader option should not be added > to DeviceUtils.Reboot. > > The 'block' option (wait until the device is ready for me to continue poking > with it) clearly makes sense together with the "and reboot to bootloader mode, > plz". > > Of course the waiting should not be done with WaitUntilFullyBooted, but through > FastbootUtils.WaitForFastbootMode. Which means, I think, that the Reboot method > in FastbootUtils should directly call adb.Reboot with suitable args instead. > > Conceptually we should have (I think): > - DeviceUtils.Reboot - always boots to "normal" mode > - FastbootUtils.Reboot - always boots to "fastboot" mode > > would that make more sense? I really like this idea.
https://codereview.chromium.org/1417373002/diff/100001/build/android/devil/an... File build/android/devil/android/device_errors.py (right): https://codereview.chromium.org/1417373002/diff/100001/build/android/devil/an... build/android/devil/android/device_errors.py:23: class FastbootCommandFailedError(CommandFailedError): On 2015/10/28 15:28:57, perezju wrote: > Could we reuse the implementation of AdbCommandFailed error, maybe through a new > parent class, instead of duplicating all of the code? Done. https://codereview.chromium.org/1417373002/diff/100001/build/android/devil/an... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1417373002/diff/100001/build/android/devil/an... build/android/devil/android/device_utils.py:529: block = False On 2015/10/28 15:28:58, perezju wrote: > I think what this reveals is that the to_bootloader option should not be added > to DeviceUtils.Reboot. > > The 'block' option (wait until the device is ready for me to continue poking > with it) clearly makes sense together with the "and reboot to bootloader mode, > plz". > > Of course the waiting should not be done with WaitUntilFullyBooted, but through > FastbootUtils.WaitForFastbootMode. Which means, I think, that the Reboot method > in FastbootUtils should directly call adb.Reboot with suitable args instead. > > Conceptually we should have (I think): > - DeviceUtils.Reboot - always boots to "normal" mode > - FastbootUtils.Reboot - always boots to "fastboot" mode > > would that make more sense? Done. https://codereview.chromium.org/1417373002/diff/100001/build/android/devil/an... build/android/devil/android/device_utils.py:2111: if not isinstance(fastboot, fastboot_utils.FastbootUtils): On 2015/10/28 15:28:58, perezju wrote: > I don't think this is solving the issue. What we should have is device_utils not > importing fastboot_utils at all. > > Same as above, I think the solution is not to add any knowledge about > fastboot/bootloader stuff in device_utils; and leave all of that to > fastboot_utils. > > If I want to wipe a device I should be able to do, in fact, just something like: > > device = device_utils.DeviceUtils(...) > fastboot_utils.FlashDevice(device, ...) Moved to fastboot_utils.py https://codereview.chromium.org/1417373002/diff/100001/build/android/devil/an... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/100001/build/android/devil/an... build/android/devil/android/fastboot_utils.py:209: def SetOemOffModeCharge(self, value, timeout=None, retries=None): On 2015/10/28 15:28:58, perezju wrote: > Do we really expect clients calling this directly? Otherwise do not expose it > and always call self.fastboot.SetOemOffModeCharge when needed. I'm unsure if it will ever be needed by clients, Removing and we can re-add if it ever becomes needed.
This lgtm now, just a couple final comments. And I guess wait for John to have a final pass over it. https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... File build/android/devil/android/device_errors.py (right): https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... build/android/devil/android/device_errors.py:45: class FastbootCommandFailedError(AdbCommandFailedError): Ohh, but not like that. Otherwise trying to catch AdbCommandFailedError will also catch FastbootCommandFailedError. Instead maybe, at least to start, create a _BaseCommandFailedError with most of the implementation, and then create two subclasses, one for AdbCommandFailedError and one for FastbootCommandFailedError. https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... build/android/devil/android/fastboot_utils.py:50: Could you add a small example here, showing how to flash a device assuming you already have a DeviceUtils instance?
https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... build/android/devil/android/fastboot_utils.py:170: def FlashPartitions(self, partitions, directory, wipe=False, force=False): Should this really be public? When would a user use this instead of FlashDevice? https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... File build/android/devil/android/fastboot_utils_test.py (right): https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... build/android/devil/android/fastboot_utils_test.py:27: import mock # pylint: disable=F0401 disable=import-error https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... File build/android/devil/android/sdk/fastboot.py (right): https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... build/android/devil/android/sdk/fastboot.py:33: def _RunFastbootCommand(self, cmd): Should any of these functions be timeout/retry decorated? I imagine it might make sense to at least have timeouts by default...
https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... File build/android/devil/android/device_errors.py (right): https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... build/android/devil/android/device_errors.py:45: class FastbootCommandFailedError(AdbCommandFailedError): On 2015/10/29 10:09:52, perezju wrote: > Ohh, but not like that. Otherwise trying to catch AdbCommandFailedError will > also catch FastbootCommandFailedError. Instead maybe, at least to start, create > a _BaseCommandFailedError with most of the implementation, and then create two > subclasses, one for AdbCommandFailedError and one for > FastbootCommandFailedError. Done. https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... build/android/devil/android/fastboot_utils.py:50: On 2015/10/29 10:09:52, perezju wrote: > Could you add a small example here, showing how to flash a device assuming you > already have a DeviceUtils instance? Done. https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... build/android/devil/android/fastboot_utils.py:170: def FlashPartitions(self, partitions, directory, wipe=False, force=False): On 2015/10/29 13:29:22, jbudorick wrote: > Should this really be public? When would a user use this instead of FlashDevice? Fixed, relic from when FlashDevices was in DeviceUtils. https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... File build/android/devil/android/fastboot_utils_test.py (right): https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... build/android/devil/android/fastboot_utils_test.py:27: import mock # pylint: disable=F0401 On 2015/10/29 13:29:22, jbudorick wrote: > disable=import-error Done. https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... File build/android/devil/android/sdk/fastboot.py (right): https://codereview.chromium.org/1417373002/diff/140001/build/android/devil/an... build/android/devil/android/sdk/fastboot.py:33: def _RunFastbootCommand(self, cmd): On 2015/10/29 13:29:22, jbudorick wrote: > Should any of these functions be timeout/retry decorated? I imagine it might > make sense to at least have timeouts by default... Since most of the commands are rebooting commands, if it times out mid reboot the rest will fail since its no longer in fastboot mode; so I'm not sure that retries make sense for all of them. I'm also going to enforce high minimum timeouts for the reboots and flashes. I dont think its a good idea having them interrupted. Its better to have it wait (random number) minutes than brick a device.
https://codereview.chromium.org/1417373002/diff/160001/build/android/devil/an... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/160001/build/android/devil/an... build/android/devil/android/fastboot_utils.py:80: while True: nit: def fastboot_mode(): return self._serial in self.fastboot.Devices() timeout_retry.WaitFor(fastboot_mode, wait_period=self._FASTBOOT_WAIT_TIME) ? https://codereview.chromium.org/1417373002/diff/160001/build/android/devil/an... build/android/devil/android/fastboot_utils.py:95: nit: only one blank line between member functions https://codereview.chromium.org/1417373002/diff/160001/build/android/devil/an... build/android/devil/android/fastboot_utils.py:135: logging.warning('Image expects board of type %s and device ' Should this case return False rather than firing off a warning and checking the file name?
https://codereview.chromium.org/1417373002/diff/160001/build/android/devil/an... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/160001/build/android/devil/an... build/android/devil/android/fastboot_utils.py:80: while True: On 2015/11/02 18:39:16, jbudorick wrote: > nit: > > def fastboot_mode(): > return self._serial in self.fastboot.Devices() > > timeout_retry.WaitFor(fastboot_mode, wait_period=self._FASTBOOT_WAIT_TIME) > > ? Done. https://codereview.chromium.org/1417373002/diff/160001/build/android/devil/an... build/android/devil/android/fastboot_utils.py:95: On 2015/11/02 18:39:16, jbudorick wrote: > nit: only one blank line between member functions Done. https://codereview.chromium.org/1417373002/diff/160001/build/android/devil/an... build/android/devil/android/fastboot_utils.py:135: logging.warning('Image expects board of type %s and device ' On 2015/11/02 18:39:15, jbudorick wrote: > Should this case return False rather than firing off a warning and checking the > file name? Switching it to return false. I cannot see a situation where this one fails and the other one passes; and if thats the case which one should be trusted? It will still check the zip if no board name is found in the file.
lgtm w/ question https://codereview.chromium.org/1417373002/diff/180001/build/android/devil/an... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/180001/build/android/devil/an... build/android/devil/android/fastboot_utils.py:210: self.Reboot(bootloader=True) This will do multiple reboots in the course of flashing a device normally, won't it? Is that intentional?
https://codereview.chromium.org/1417373002/diff/180001/build/android/devil/an... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/180001/build/android/devil/an... build/android/devil/android/fastboot_utils.py:210: self.Reboot(bootloader=True) On 2015/11/04 22:55:57, jbudorick wrote: > This will do multiple reboots in the course of flashing a device normally, won't > it? Is that intentional? Yes. After flashing those partitions, a reboot is required. According to the clusterfuzz code anyway.
The CQ bit was checked by rnephew@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org, perezju@chromium.org Link to the patchset: https://codereview.chromium.org/1417373002/#ps180001 (title: " ")
https://codereview.chromium.org/1417373002/diff/180001/build/android/devil/an... File build/android/devil/android/fastboot_utils.py (right): https://codereview.chromium.org/1417373002/diff/180001/build/android/devil/an... build/android/devil/android/fastboot_utils.py:210: self.Reboot(bootloader=True) On 2015/11/04 22:58:42, rnephew wrote: > On 2015/11/04 22:55:57, jbudorick wrote: > > This will do multiple reboots in the course of flashing a device normally, > won't > > it? Is that intentional? > > Yes. After flashing those partitions, a reboot is required. According to the > clusterfuzz code anyway. sgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417373002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417373002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/536629676d896885ca26b05ba69f2fb8c4fad702 Cr-Commit-Position: refs/heads/master@{#357939} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
