Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 6 months ago by mikecase (-- gone --) Modified:
5 years, 6 months ago Reviewers:
perezju, jbudorick, agrieve 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. |
DescriptionAdd InstallSplitApk function to device utils.
Adding a InstallSplitApk function to device utils so that our
test runner will be able to support install/testing split
apks.
BUG=
Committed: https://crrev.com/89d5ad7274ce871af5f4cd3f9f0a09b9ff90cf02
Cr-Commit-Position: refs/heads/master@{#335336}
Patch Set 1 #Patch Set 2 : #
Total comments: 7
Patch Set 3 : Removed RequireSdk decorator. Added split-select wrapper. #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 12
Patch Set 7 : Addressed perezju's and agrieve's comments. #Patch Set 8 : #
Total comments: 14
Patch Set 9 : Addressed comments. Moved split_select to pylib/sdk. #
Total comments: 3
Patch Set 10 : Fixed jbudorick's nits. #
Created: 5 years, 6 months ago
Messages
Total messages: 20 (4 generated)
mikecase@chromium.org changed reviewers: + jbudorick@chromium.org
Could you take a quick look at this to see if this is how/where you would add a function to install split apks. Thanks.
https://codereview.chromium.org/1166113002/diff/20001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1166113002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:205: # pylint: disable=no-self-argument Guessing you won't like this function (kinda hacky), but could be useful if we add other functionality in the future that requires a certain SDK level. I could always just move this check to the beginning of my InstallSplitApk function.
mentioned most of this offline already https://codereview.chromium.org/1166113002/diff/20001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1166113002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:205: # pylint: disable=no-self-argument On 2015/06/08 at 19:14:52, mikecase wrote: > Guessing you won't like this function (kinda hacky), but could be useful if we add other functionality in the future that requires a certain SDK level. > > I could always just move this check to the beginning of my InstallSplitApk function. should be in decorators.py https://codereview.chromium.org/1166113002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:514: def select_splits(): extract to its own module https://codereview.chromium.org/1166113002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:529: if device_apk_paths: I'm not sure about this part. It looks like it's installing all of the splits if any of them change. That doesn't seem right...
mikecase@chromium.org changed reviewers: + agrieve@chromium.org, perezju@chromium.org
https://codereview.chromium.org/1166113002/diff/20001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1166113002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:205: # pylint: disable=no-self-argument On 2015/06/08 at 19:38:26, jbudorick (ooo til 2015-06-18) wrote: > On 2015/06/08 at 19:14:52, mikecase wrote: > > Guessing you won't like this function (kinda hacky), but could be useful if we add other functionality in the future that requires a certain SDK level. > > > > I could always just move this check to the beginning of my InstallSplitApk function. > > should be in decorators.py Removed decorator. Added _CheckSdkLevel function. https://codereview.chromium.org/1166113002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:514: def select_splits(): On 2015/06/08 at 19:38:26, jbudorick (ooo til 2015-06-18) wrote: > extract to its own module done https://codereview.chromium.org/1166113002/diff/20001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:529: if device_apk_paths: On 2015/06/08 at 19:38:26, jbudorick wrote: > I'm not sure about this part. It looks like it's installing all of the splits if any of them change. That doesn't seem right... I think you have to install them all or nothing. I don't think you can install a single split at a time.
https://codereview.chromium.org/1166113002/diff/100001/build/android/pylib/de... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1166113002/diff/100001/build/android/pylib/de... build/android/pylib/device/device_utils.py:365: return [s[len('package:'):] for s in output] This is walking over the output twice and building two lists. I guess we can walk over the list only once, checking each line and extracting the path as we go. Also, now it's probably fine to return an empty list when no paths are found (i.e. don't special case the empty output case). Also, are we sure we have updated all clients of this method? https://codereview.chromium.org/1166113002/diff/100001/build/android/pylib/de... build/android/pylib/device/device_utils.py:489: (files_to_push, _) = self._GetChangedAndStaleFiles( maybe add a warning if device_paths has more than one element in it. https://codereview.chromium.org/1166113002/diff/100001/build/android/pylib/de... build/android/pylib/device/device_utils.py:502: def InstallSplitApk(self, base_apk, split_apks, reinstall=False, This is duplicating a lot of the code in Install. Don't know how others would feel about this, but how about adding a split_apks option (defaulting to None) to that method instead? https://codereview.chromium.org/1166113002/diff/100001/build/android/pylib/de... build/android/pylib/device/device_utils.py:1363: DPI_TO_DENSITY = { not sure, but maybe this dict should be a private module constant. https://codereview.chromium.org/1166113002/diff/100001/build/android/pylib/de... File build/android/pylib/device/split_select_wrapper.py (right): https://codereview.chromium.org/1166113002/diff/100001/build/android/pylib/de... build/android/pylib/device/split_select_wrapper.py:42: 'Failed command %s with output %s' % (str(cmd), output)) I'm not sure if these are supposed to be "device" errors, as they did not happen in a device.
Thanks for taking this on! Much improved :) https://codereview.chromium.org/1166113002/diff/100001/build/android/pylib/de... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1166113002/diff/100001/build/android/pylib/de... build/android/pylib/device/device_utils.py:539: self.adb.InstallMultiple( It would be great to only install the splits that have changed. You can do this only for a re-install (not for a fresh install), and you use the command: adb install-multiple -r -p $PACKAGE_NAME a.apk c.apk
https://codereview.chromium.org/1166113002/diff/100001/build/android/pylib/de... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1166113002/diff/100001/build/android/pylib/de... build/android/pylib/device/device_utils.py:365: return [s[len('package:'):] for s in output] On 2015/06/18 at 09:30:17, perezju wrote: > This is walking over the output twice and building two lists. > > I guess we can walk over the list only once, checking each line and extracting the path as we go. Also, now it's probably fine to return an empty list when no paths are found (i.e. don't special case the empty output case). > > Also, are we sure we have updated all clients of this method? Now only iterate through list once. Removed special case to return None if no command output (will return [] in this case now) Missed one use of GetApplicationPath from build/android/update_verification.py. Fixed. https://codereview.chromium.org/1166113002/diff/100001/build/android/pylib/de... build/android/pylib/device/device_utils.py:489: (files_to_push, _) = self._GetChangedAndStaleFiles( On 2015/06/18 at 09:30:17, perezju wrote: > maybe add a warning if device_paths has more than one element in it. Done https://codereview.chromium.org/1166113002/diff/100001/build/android/pylib/de... build/android/pylib/device/device_utils.py:502: def InstallSplitApk(self, base_apk, split_apks, reinstall=False, On 2015/06/18 at 09:30:17, perezju wrote: > This is duplicating a lot of the code in Install. Don't know how others would feel about this, but how about adding a split_apks option (defaulting to None) to that method instead? Added some more to the InstallSplitApk function so that only splits that are not currently installed are reinstalled. I think it would be messy to merge the two function. https://codereview.chromium.org/1166113002/diff/100001/build/android/pylib/de... build/android/pylib/device/device_utils.py:539: self.adb.InstallMultiple( On 2015/06/18 at 14:01:08, agrieve wrote: > It would be great to only install the splits that have changed. You can do this only for a re-install (not for a fresh install), and you use the command: > > adb install-multiple -r -p $PACKAGE_NAME a.apk c.apk Done. https://codereview.chromium.org/1166113002/diff/100001/build/android/pylib/de... build/android/pylib/device/device_utils.py:1363: DPI_TO_DENSITY = { On 2015/06/18 at 09:30:16, perezju wrote: > not sure, but maybe this dict should be a private module constant. Leaving this here for now. Nothing else needs this dict. https://codereview.chromium.org/1166113002/diff/100001/build/android/pylib/de... File build/android/pylib/device/split_select_wrapper.py (right): https://codereview.chromium.org/1166113002/diff/100001/build/android/pylib/de... build/android/pylib/device/split_select_wrapper.py:42: 'Failed command %s with output %s' % (str(cmd), output)) On 2015/06/18 at 09:30:17, perezju wrote: > I'm not sure if these are supposed to be "device" errors, as they did not happen in a device. Changed to just raise an Exception.
lgtm https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... File build/android/pylib/device/device_utils_test.py (right): https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... build/android/pylib/device/device_utils_test.py:577: def testInstallSplitApk_noPriorInstall(self): might want to add a test when partial != None
lgtm, just a couple of comments https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... build/android/pylib/device/device_utils.py:534: partial_install = package_name nit: I find this name confusing as I would expect it to hold a bool. Maybe name it: partial_for_package ? https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... File build/android/pylib/device/split_select_wrapper.py (right): https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... build/android/pylib/device/split_select_wrapper.py:40: raise Exception('Failed command %s with output %s' % (str(cmd), output)) so split-select might return zero-exit code but with "error" in its output? have we actually seen this? or just being overly cautious?
https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... build/android/pylib/device/device_utils.py:555: ('Requires SDK level %s, device is SDK level %s' % nit: indentation is off https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... build/android/pylib/device/device_utils.py:1359: def langauge_setting(self): langauge -> language Also, I'd prefer dropping the "_setting" from both this and country_setting. https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... File build/android/pylib/device/split_select_wrapper.py (right): https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... build/android/pylib/device/split_select_wrapper.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. I don't think this should be in build/android/pylib/device, at least not directly. Instead, I'd prefer we create a new sdk/ directory (either in device/ or in pylib/) for modules that are thin wrappers of tools in the android sdk. (build/android/pylib/utils/proguard.py would move in there, too) Also, I'd rather this just be "split_select.py" in that scenario. https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... build/android/pylib/device/split_select_wrapper.py:20: @decorators.WithTimeoutAndRetries Why are we decorating this? It's private and there's no call to a device.
https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... build/android/pylib/device/device_utils.py:534: partial_install = package_name On 2015/06/19 at 11:15:19, perezju wrote: > nit: I find this name confusing as I would expect it to hold a bool. Maybe name it: partial_for_package ? renamed to partial_install_package https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... build/android/pylib/device/device_utils.py:555: ('Requires SDK level %s, device is SDK level %s' % On 2015/06/19 at 13:20:13, jbudorick wrote: > nit: indentation is off Done https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... build/android/pylib/device/device_utils.py:1359: def langauge_setting(self): On 2015/06/19 at 13:20:13, jbudorick wrote: > langauge -> language > > Also, I'd prefer dropping the "_setting" from both this and country_setting. Done https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... File build/android/pylib/device/device_utils_test.py (right): https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... build/android/pylib/device/device_utils_test.py:577: def testInstallSplitApk_noPriorInstall(self): On 2015/06/18 at 20:28:50, agrieve wrote: > might want to add a test when partial != None Done. Added new test for partial install. https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... File build/android/pylib/device/split_select_wrapper.py (right): https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... build/android/pylib/device/split_select_wrapper.py:20: @decorators.WithTimeoutAndRetries On 2015/06/19 at 13:20:13, jbudorick wrote: > Why are we decorating this? It's private and there's no call to a device. Didn't know this decorator was only for code run on devices. Will remove. https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... build/android/pylib/device/split_select_wrapper.py:40: raise Exception('Failed command %s with output %s' % (str(cmd), output)) On 2015/06/19 at 11:15:19, perezju wrote: > so split-select might return zero-exit code but with "error" in its output? have we actually seen this? or just being overly cautious? Hmm. Good catch, I was kinda copying what the adb wrapper did. The only time I can get "error" in the output is with a exit code of 1. Remove this error as it will never get hit.
https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... File build/android/pylib/device/split_select_wrapper.py (right): https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... build/android/pylib/device/split_select_wrapper.py:20: @decorators.WithTimeoutAndRetries On 2015/06/19 at 17:17:52, mikecase wrote: > On 2015/06/19 at 13:20:13, jbudorick wrote: > > Why are we decorating this? It's private and there's no call to a device. > > Didn't know this decorator was only for code run on devices. Will remove. It isn't, but it's usually for things that have a chance of flakily timing out or failing, which device interaction tends to do. Additionally, it's usually better to do it at the public interface level rather than at the private implementation function level. https://codereview.chromium.org/1166113002/diff/160001/build/android/pylib/de... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1166113002/diff/160001/build/android/pylib/de... build/android/pylib/device/device_utils.py:350: List of paths to the apks on the device if they exists, None otherwise. nit: "None otherwise" is no longer accurate. https://codereview.chromium.org/1166113002/diff/160001/build/android/pylib/de... build/android/pylib/device/device_utils.py:1359: def langauge(self): nit: langauge -> language https://codereview.chromium.org/1166113002/diff/160001/build/android/pylib/sd... File build/android/pylib/sdk/__init__.py (right): https://codereview.chromium.org/1166113002/diff/160001/build/android/pylib/sd... build/android/pylib/sdk/__init__.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. note that this will probably conflict with the addition of the same file in https://codereview.chromium.org/1188253013/
forgot to add: lgtm w/ nits On 2015/06/19 at 17:32:13, jbudorick wrote: > https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... > File build/android/pylib/device/split_select_wrapper.py (right): > > https://codereview.chromium.org/1166113002/diff/140001/build/android/pylib/de... > build/android/pylib/device/split_select_wrapper.py:20: @decorators.WithTimeoutAndRetries > On 2015/06/19 at 17:17:52, mikecase wrote: > > On 2015/06/19 at 13:20:13, jbudorick wrote: > > > Why are we decorating this? It's private and there's no call to a device. > > > > Didn't know this decorator was only for code run on devices. Will remove. > > It isn't, but it's usually for things that have a chance of flakily timing out or failing, which device interaction tends to do. Additionally, it's usually better to do it at the public interface level rather than at the private implementation function level. > > https://codereview.chromium.org/1166113002/diff/160001/build/android/pylib/de... > File build/android/pylib/device/device_utils.py (right): > > https://codereview.chromium.org/1166113002/diff/160001/build/android/pylib/de... > build/android/pylib/device/device_utils.py:350: List of paths to the apks on the device if they exists, None otherwise. > nit: "None otherwise" is no longer accurate. > > https://codereview.chromium.org/1166113002/diff/160001/build/android/pylib/de... > build/android/pylib/device/device_utils.py:1359: def langauge(self): > nit: langauge -> language > > https://codereview.chromium.org/1166113002/diff/160001/build/android/pylib/sd... > File build/android/pylib/sdk/__init__.py (right): > > https://codereview.chromium.org/1166113002/diff/160001/build/android/pylib/sd... > build/android/pylib/sdk/__init__.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. > note that this will probably conflict with the addition of the same file in https://codereview.chromium.org/1188253013/
The CQ bit was checked by mikecase@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org, agrieve@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1166113002/#ps180001 (title: "Fixed jbudorick's nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1166113002/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/89d5ad7274ce871af5f4cd3f9f0a09b9ff90cf02 Cr-Commit-Position: refs/heads/master@{#335336} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
