Add --split-apk-path flag to apk_install.py, and install-multiple logic to adb_wrapper (issue 1134353002 by agrieve@chromium.org)

[agr...@chromium.org]

Reviewers: jbudorick,

Description:
Add --split-apk-path flag to apk_install.py, and install-multiple logic to
adb_wrapper

There's a TODO for using split-select, but it won't be necessary until
we have resource splits being generated.

BUG=447152

Please review this at https://codereview.chromium.org/1134353002/

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+41, -12 lines):
M build/android/gyp/apk_install.py
M build/android/pylib/device/adb_wrapper.py
M build/android/pylib/device/device_utils.py
M build/android/pylib/device/device_utils_test.py


Index: build/android/gyp/apk_install.py
diff --git a/build/android/gyp/apk_install.py
b/build/android/gyp/apk_install.py
index
19a217ccf5234a22814168ed7341f5fb9a76fe8a..4b4c7723deb95fbdd8a23e54ffeb584a62fbd6af
100755
--- a/build/android/gyp/apk_install.py
+++ b/build/android/gyp/apk_install.py
@@ -58,6 +58,10 @@ def main():
parser = optparse.OptionParser()
parser.add_option('--apk-path',
help='Path to .apk to install.')
+ parser.add_option('--split-apk-path',
+ help='Path to .apk splits (can specify multiple times, causes '
+ '--install-multiple to be used.',
+ action='append')
parser.add_option('--install-record',
help='Path to install record (touched only when APK is installed).')
parser.add_option('--build-device-configuration',
@@ -85,7 +89,10 @@ def main():
force_install = HasInstallMetadataChanged(device, apk_package,
metadata_path)

def Install():
- device.Install(options.apk_path, reinstall=True)
+ # TODO: Filter splits using split-select.
+ active_splits = options.split_apk_path
+ device.Install(options.apk_path, reinstall=True,
split_paths=active_splits)
+
RecordInstallMetadata(device, apk_package, metadata_path)
build_utils.Touch(options.install_record)

Index: build/android/pylib/device/adb_wrapper.py
diff --git a/build/android/pylib/device/adb_wrapper.py
b/build/android/pylib/device/adb_wrapper.py
index
8e8abf8b5e01bcc7d9bdbc911b3f4e7a634e2758..d56ba2529a1ec5827706737b3627b3a37bd501e0
100644
--- a/build/android/pylib/device/adb_wrapper.py
+++ b/build/android/pylib/device/adb_wrapper.py
@@ -390,7 +390,8 @@ class AdbWrapper(object):
self._RunDeviceAdbCmd(['jdwp'], timeout, retries).split('\n')]

def Install(self, apk_path, forward_lock=False, reinstall=False,
- sd_card=False, timeout=60*2, retries=_DEFAULT_RETRIES):
+ sd_card=False, timeout=60*2, retries=_DEFAULT_RETRIES,
+ split_paths=None):
"""Install an apk on the device.

Args:
@@ -400,9 +401,14 @@ class AdbWrapper(object):
sd_card: (optional) If set installs on the SD card.
timeout: (optional) Timeout per try in seconds.
retries: (optional) Number of retries to attempt.
+ split_paths: (optional) List of splits to install. Triggers use of
+ "adb install-multiple" rather than "adb install".
"""
_VerifyLocalFileExists(apk_path)
- cmd = ['install']
+ if split_paths:
+ cmd = ['install-multiple']
+ else:
+ cmd = ['install']
if forward_lock:
cmd.append('-l')
if reinstall:
@@ -410,6 +416,8 @@ class AdbWrapper(object):
if sd_card:
cmd.append('-s')
cmd.append(apk_path)
+ if split_paths:
+ cmd.extend(split_paths)
output = self._RunDeviceAdbCmd(cmd, timeout, retries)
if 'Success' not in output:
raise device_errors.AdbCommandFailedError(
@@ -570,4 +578,3 @@ class AdbWrapper(object):
return self.GetState() == _READY_STATE
except device_errors.CommandFailedError:
return False
-
Index: build/android/pylib/device/device_utils.py
diff --git a/build/android/pylib/device/device_utils.py
b/build/android/pylib/device/device_utils.py
index
967809ff8e2860d5846f181b780e1ed7c341dc33..72422467ee3d8d075d36be3c3599280ddc62f778
100644
--- a/build/android/pylib/device/device_utils.py
+++ b/build/android/pylib/device/device_utils.py
@@ -444,7 +444,8 @@ class DeviceUtils(object):
@decorators.WithTimeoutAndRetriesDefaults(
INSTALL_DEFAULT_TIMEOUT,
INSTALL_DEFAULT_RETRIES)
- def Install(self, apk_path, reinstall=False, timeout=None, retries=None):
+ def Install(self, apk_path, reinstall=False, timeout=None, retries=None,
+ split_paths=None):
"""Install an APK.

Noop if an identical APK is already installed.
@@ -454,6 +455,8 @@ class DeviceUtils(object):
reinstall: A boolean indicating if we should keep any existing app
data.
timeout: timeout in seconds
retries: number of retries
+ split_paths: (optional) List of splits to install. Triggers use of
+ "adb install-multiple" rather than "adb install".

Raises:
CommandFailedError if the installation fails.
@@ -469,7 +472,7 @@ class DeviceUtils(object):
else:
should_install = True
if should_install:
- self.adb.Install(apk_path, reinstall=reinstall)
+ self.adb.Install(apk_path, reinstall=reinstall,
split_paths=split_paths)

@decorators.WithTimeoutAndRetriesFromInstance()
def RunShellCommand(self, cmd, check_return=False, cwd=None, env=None,
@@ -1585,4 +1588,3 @@ class DeviceUtils(object):

return [cls(adb) for adb in adb_wrapper.AdbWrapper.Devices()
if not blacklisted(adb)]
-
Index: build/android/pylib/device/device_utils_test.py
diff --git a/build/android/pylib/device/device_utils_test.py
b/build/android/pylib/device/device_utils_test.py
index
23a1a325e7680dda7e12d2fa8a3c8e6a084b8d3b..b6c5a2fea38f87da86d42e3ef5811f1795f13e81
100755
--- a/build/android/pylib/device/device_utils_test.py
+++ b/build/android/pylib/device/device_utils_test.py
@@ -474,9 +474,20 @@ class DeviceUtilsInstallTest(DeviceUtilsTest):

(mock.call.pylib.utils.apk_helper.GetPackageName('/fake/test/app.apk'),
'this.is.a.test.package'),
(self.call.device.GetApplicationPath('this.is.a.test.package'),
None),
- self.call.adb.Install('/fake/test/app.apk', reinstall=False)):
+ self.call.adb.Install('/fake/test/app.apk', reinstall=False,
+ split_paths=None)):
self.device.Install('/fake/test/app.apk', retries=0)

+ def testInstall_withSplits(self):
+ with self.assertCalls(
+
(mock.call.pylib.utils.apk_helper.GetPackageName('/fake/test/app.apk'),
+ 'this.is.a.test.package'),
+ (self.call.device.GetApplicationPath('this.is.a.test.package'),
None),
+ self.call.adb.Install('/fake/test/app.apk', reinstall=False,
+ split_paths=['/fake/test/split.apk'])):
+ self.device.Install('/fake/test/app.apk', retries=0,
+ split_paths=['/fake/test/split.apk'])
+
def testInstall_differentPriorInstall(self):
with self.assertCalls(

(mock.call.pylib.utils.apk_helper.GetPackageName('/fake/test/app.apk'),
@@ -487,7 +498,8 @@ class DeviceUtilsInstallTest(DeviceUtilsTest):
'/fake/test/app.apk', '/fake/data/app/this.is.a.test.package.apk'),

[('/fake/test/app.apk', '/fake/data/app/this.is.a.test.package.apk')]),
self.call.adb.Uninstall('this.is.a.test.package'),
- self.call.adb.Install('/fake/test/app.apk', reinstall=False)):
+ self.call.adb.Install('/fake/test/app.apk', reinstall=False,
+ split_paths=None)):
self.device.Install('/fake/test/app.apk', retries=0)

def testInstall_differentPriorInstall_reinstall(self):
@@ -499,7 +511,8 @@ class DeviceUtilsInstallTest(DeviceUtilsTest):
(self.call.device._GetChangedFilesImpl(
'/fake/test/app.apk', '/fake/data/app/this.is.a.test.package.apk'),

[('/fake/test/app.apk', '/fake/data/app/this.is.a.test.package.apk')]),
- self.call.adb.Install('/fake/test/app.apk', reinstall=True)):
+ self.call.adb.Install('/fake/test/app.apk', reinstall=True,
+ split_paths=None)):
self.device.Install('/fake/test/app.apk', reinstall=True, retries=0)

def testInstall_identicalPriorInstall(self):
@@ -518,7 +531,8 @@ class DeviceUtilsInstallTest(DeviceUtilsTest):

(mock.call.pylib.utils.apk_helper.GetPackageName('/fake/test/app.apk'),
'this.is.a.test.package'),
(self.call.device.GetApplicationPath('this.is.a.test.package'),
None),
- (self.call.adb.Install('/fake/test/app.apk', reinstall=False),
+ (self.call.adb.Install('/fake/test/app.apk', reinstall=False,
+ split_paths=None),
self.CommandError('Failure\r\n'))):
with self.assertRaises(device_errors.CommandFailedError):
self.device.Install('/fake/test/app.apk', retries=0)
@@ -1679,4 +1693,3 @@ class
DeviceUtilsHealthyDevicesTest(mock_calls.TestCase):
if __name__ == '__main__':
logging.getLogger().setLevel(logging.DEBUG)
unittest.main(verbosity=2)
-

[jbud...@chromium.org]

https://codereview.chromium.org/1134353002/diff/1/build/android/gyp/apk_install.py
File build/android/gyp/apk_install.py (right):

https://codereview.chromium.org/1134353002/diff/1/build/android/gyp/apk_install.py#newcode94
build/android/gyp/apk_install.py:94: device.Install(options.apk_path,
reinstall=True, split_paths=active_splits)
Why should we do this instead of just installing each apk in
active_splits?

https://codereview.chromium.org/1134353002/diff/1/build/android/pylib/device/adb_wrapper.py
File build/android/pylib/device/adb_wrapper.py (right):

https://codereview.chromium.org/1134353002/diff/1/build/android/pylib/device/adb_wrapper.py#newcode409
build/android/pylib/device/adb_wrapper.py:409: cmd =
['install-multiple']
This should be in its own InstallMultiple function if we want it at all.
(I'm not entirely convinced that we do, though.)

https://codereview.chromium.org/1134353002/

[agr...@chromium.org]

https://codereview.chromium.org/1134353002/diff/1/build/android/gyp/apk_install.py
File build/android/gyp/apk_install.py (right):

https://codereview.chromium.org/1134353002/diff/1/build/android/gyp/apk_install.py#newcode94
build/android/gyp/apk_install.py:94: device.Install(options.apk_path,
reinstall=True, split_paths=active_splits)

On 2015/05/13 15:15:42, jbudorick wrote:
> Why should we do this instead of just installing each apk in
active_splits?

I'm guessing you're question is about using "adb install-multiple" vs
"adb
install" for each split?
The answer is that you can't. Splits are not normal .apks, and they can
only be
installed via "adb install-multiple"

https://codereview.chromium.org/1134353002/diff/1/build/android/pylib/device/adb_wrapper.py
File build/android/pylib/device/adb_wrapper.py (right):

https://codereview.chromium.org/1134353002/diff/1/build/android/pylib/device/adb_wrapper.py#newcode409
build/android/pylib/device/adb_wrapper.py:409: cmd =
['install-multiple']

On 2015/05/13 15:15:42, jbudorick wrote:
> This should be in its own InstallMultiple function if we want it at
all. (I'm
> not entirely convinced that we do, though.)

Done.

https://codereview.chromium.org/1134353002/

[jbud...@chromium.org]

https://codereview.chromium.org/1134353002/diff/1/build/android/gyp/apk_install.py
File build/android/gyp/apk_install.py (right):

https://codereview.chromium.org/1134353002/diff/1/build/android/gyp/apk_install.py#newcode94
build/android/gyp/apk_install.py:94: device.Install(options.apk_path,
reinstall=True, split_paths=active_splits)

On 2015/05/13 15:47:46, agrieve wrote:
> On 2015/05/13 15:15:42, jbudorick wrote:
> > Why should we do this instead of just installing each apk in
active_splits?

> I'm guessing you're question is about using "adb install-multiple" vs
"adb
> install" for each split?
> The answer is that you can't. Splits are not normal .apks, and they
can only be
> installed via "adb install-multiple"

Yeah, that was the question. Sorry, should've phrased it better.

In that case, this should not go through DeviceUtils.Install.

https://codereview.chromium.org/1134353002/

[agr...@chromium.org]

Makes sense. Done.

https://codereview.chromium.org/1134353002/

[jbud...@chromium.org]

lgtm w/ nits


https://codereview.chromium.org/1134353002/diff/40001/build/android/pylib/device/adb_wrapper.py
File build/android/pylib/device/adb_wrapper.py (right):

https://codereview.chromium.org/1134353002/diff/40001/build/android/pylib/device/adb_wrapper.py#newcode419
build/android/pylib/device/adb_wrapper.py:419: sd_card=False,
timeout=60*2, retries=_DEFAULT_RETRIES,
nits:
- indentation is off
- timeout and retries should be the last two parameters

https://codereview.chromium.org/1134353002/

[agr...@google.com]

Thanks for the speedy review! :)

https://codereview.chromium.org/1134353002/diff/1/build/android/gyp/apk_install.py
File build/android/gyp/apk_install.py (right):

https://codereview.chromium.org/1134353002/diff/1/build/android/gyp/apk_install.py#newcode94
build/android/gyp/apk_install.py:94: device.Install(options.apk_path,
reinstall=True, split_paths=active_splits)

On 2015/05/13 15:15:42, jbudorick wrote:
> Why should we do this instead of just installing each apk in
active_splits?

I'm guessing you're question is about using "adb install-multiple" vs
"adb install" for each split?
The answer is that you can't. Splits are not normal .apks, and they can
only be installed via "adb install-multiple"

https://codereview.chromium.org/1134353002/diff/40001/build/android/pylib/device/adb_wrapper.py
File build/android/pylib/device/adb_wrapper.py (right):

https://codereview.chromium.org/1134353002/diff/40001/build/android/pylib/device/adb_wrapper.py#newcode419
build/android/pylib/device/adb_wrapper.py:419: sd_card=False,
timeout=60*2, retries=_DEFAULT_RETRIES,

On 2015/05/13 16:15:07, jbudorick wrote:
> nits:
> - indentation is off
> - timeout and retries should be the last two parameters

Done.

https://codereview.chromium.org/1134353002/

[commi...@chromium.org]

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


https://codereview.chromium.org/1134353002/

[commi...@chromium.org]

Committed patchset #4 (id:60001)

https://codereview.chromium.org/1134353002/

[commi...@chromium.org]

Patchset 4 (id:??) landed as
https://crrev.com/845f198cd6e47f25d68355898fdff474796e2165
Cr-Commit-Position: refs/heads/master@{#329844}

https://codereview.chromium.org/1134353002/