Add InstallSplitApk function to device utils.

Add 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}

[mikecase (-- gone --), 2015-06-08 19:13:28]

mikecase@chromium.org changed reviewers:
+ jbudorick@chromium.org

[mikecase (-- gone --), 2015-06-08 19:13:28]

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.

[mikecase (-- gone --), 2015-06-08 19:14:52]

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.

[jbudorick, 2015-06-08 19:38:26]

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 (-- gone --), 2015-06-17 22:00:41]

mikecase@chromium.org changed reviewers:
+ agrieve@chromium.org, perezju@chromium.org

[mikecase (-- gone --), 2015-06-17 22:00:43]

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.

[perezju, 2015-06-18 09:30:17]

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.

[agrieve, 2015-06-18 14:01:08]

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

[mikecase (-- gone --), 2015-06-18 19:22:20]

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.

[agrieve, 2015-06-18 20:28:50]

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

[perezju, 2015-06-19 11:15:19]

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?

[jbudorick, 2015-06-19 13:20:14]

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.

[mikecase (-- gone --), 2015-06-19 17:17:53]

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.

[jbudorick, 2015-06-19 17:32:13]

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/

[jbudorick, 2015-06-19 17:32:27]

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/

[mikecase (-- gone --), 2015-06-19 18:35:51]

The CQ bit was checked by mikecase@chromium.org

[mikecase (-- gone --), 2015-06-19 18:35:51]

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.")

[commit-bot: I haz the power, 2015-06-19 18:36:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1166113002/180001

[commit-bot: I haz the power, 2015-06-19 20:43:48]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-06-19 20:44:37]

Patchset 10 (id:??) landed as
https://crrev.com/89d5ad7274ce871af5f4cd3f9f0a09b9ff90cf02
Cr-Commit-Position: refs/heads/master@{#335336}