|
|
Created:
5 years, 7 months ago by agrieve Modified:
5 years, 7 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, klundberg+watch_chromium.org, sadrul, yfriedman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@uses-sdk-21 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd split-select logic to apk_install.py & fix crash when --split-apk-path is used
This makes the script able to install the correct density-based split.
BUG=447152
Committed: https://crrev.com/16cd4e3dcbac844d194be8518f9b049a35c7634e
Cr-Commit-Position: refs/heads/master@{#331205}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fix style nits and add minSdkVersion check when splits are enabled #Patch Set 3 : use constants rather than hardcoding lollipop as 21 #
Total comments: 6
Patch Set 4 : Address review nits #
Total comments: 8
Patch Set 5 : remove print, convert ShellCommannd to GetProp()s, gyp flag #Messages
Total messages: 18 (3 generated)
agrieve@chromium.org changed reviewers: + jbudorick@chromium.org
jbudorick@chromium.org changed reviewers: + cjhopman@chromium.org
+cjhopman owns b/a/gyp https://codereview.chromium.org/1141403002/diff/1/build/android/gyp/apk_insta... File build/android/gyp/apk_install.py (right): https://codereview.chromium.org/1141403002/diff/1/build/android/gyp/apk_insta... build/android/gyp/apk_install.py:28: SHELL_CONFIG_COMMAND = ('echo $(getprop persist.sys.language)-' I have questions about this that I'll ask you about out-of-band. https://codereview.chromium.org/1141403002/diff/1/build/android/gyp/apk_insta... build/android/gyp/apk_install.py:125: active_splits = SelectSplits(device_config, options.apk_path, nit: either indent params on subsequent lines to the depth of the first parameter, or drop all of the params to a subsequent line & indent four spaces. (it differs from our java indentation rules, which always gets me going the other way) https://codereview.chromium.org/1141403002/diff/1/build/android/gyp/apk_insta... build/android/gyp/apk_install.py:130: print('Installing apks: ' + ', '.join(apk_names)) nit: We don't typically use the python3 print syntax.
https://codereview.chromium.org/1141403002/diff/1/build/android/gyp/apk_insta... File build/android/gyp/apk_install.py (right): https://codereview.chromium.org/1141403002/diff/1/build/android/gyp/apk_insta... build/android/gyp/apk_install.py:123: if options.split_apk_path: This should check against a minimum SDK level, e.g. https://code.google.com/p/chromium/codesearch#chromium/src/build/android/prov...
https://codereview.chromium.org/1141403002/diff/40001/build/android/gyp/apk_i... File build/android/gyp/apk_install.py (right): https://codereview.chromium.org/1141403002/diff/40001/build/android/gyp/apk_i... build/android/gyp/apk_install.py:29: SHELL_CONFIG_COMMAND = ('echo $(getprop persist.sys.language)-' This is kind of a dumb question, but ... what does this actually do? https://codereview.chromium.org/1141403002/diff/40001/build/android/gyp/apk_i... build/android/gyp/apk_install.py:138: apk_names = [os.path.basename(path) for path in all_apks] nit: looks like you're only using this once, so it can be a generator instead of a list comprehension https://codereview.chromium.org/1141403002/diff/40001/build/android/gyp/apk_i... build/android/gyp/apk_install.py:140: device.device.adb.InstallMultiple(all_apks, reinstall=True) device.device? Is this right?
Addressed comments. https://codereview.chromium.org/1141403002/diff/1/build/android/gyp/apk_insta... File build/android/gyp/apk_install.py (right): https://codereview.chromium.org/1141403002/diff/1/build/android/gyp/apk_insta... build/android/gyp/apk_install.py:28: SHELL_CONFIG_COMMAND = ('echo $(getprop persist.sys.language)-' On 2015/05/19 15:06:59, jbudorick wrote: > I have questions about this that I'll ask you about out-of-band. Acknowledged. https://codereview.chromium.org/1141403002/diff/1/build/android/gyp/apk_insta... build/android/gyp/apk_install.py:125: active_splits = SelectSplits(device_config, options.apk_path, On 2015/05/19 15:06:59, jbudorick wrote: > nit: either indent params on subsequent lines to the depth of the first > parameter, or drop all of the params to a subsequent line & indent four spaces. > > (it differs from our java indentation rules, which always gets me going the > other way) Done. https://codereview.chromium.org/1141403002/diff/1/build/android/gyp/apk_insta... build/android/gyp/apk_install.py:130: print('Installing apks: ' + ', '.join(apk_names)) On 2015/05/19 15:06:59, jbudorick wrote: > nit: We don't typically use the python3 print syntax. Done. https://codereview.chromium.org/1141403002/diff/40001/build/android/gyp/apk_i... File build/android/gyp/apk_install.py (right): https://codereview.chromium.org/1141403002/diff/40001/build/android/gyp/apk_i... build/android/gyp/apk_install.py:29: SHELL_CONFIG_COMMAND = ('echo $(getprop persist.sys.language)-' On 2015/05/19 18:21:59, jbudorick wrote: > This is kind of a dumb question, but ... what does this actually do? Update pydoc with example output. https://codereview.chromium.org/1141403002/diff/40001/build/android/gyp/apk_i... build/android/gyp/apk_install.py:138: apk_names = [os.path.basename(path) for path in all_apks] On 2015/05/19 18:21:59, jbudorick wrote: > nit: looks like you're only using this once, so it can be a generator instead of > a list comprehension Done. https://codereview.chromium.org/1141403002/diff/40001/build/android/gyp/apk_i... build/android/gyp/apk_install.py:140: device.device.adb.InstallMultiple(all_apks, reinstall=True) On 2015/05/19 18:21:59, jbudorick wrote: > device.device? Is this right? Sad naming :( device is a BuildDevice device.device is a DeviceUtil
https://codereview.chromium.org/1141403002/diff/60001/build/android/gyp/apk_i... File build/android/gyp/apk_install.py (right): https://codereview.chromium.org/1141403002/diff/60001/build/android/gyp/apk_i... build/android/gyp/apk_install.py:65: return expected_file.read() != device.GetInstallMetadata(apk_package) Is the install metadata still valid in a split world? Also, why do we use device.GetInstallMetadata here but then when we record it we get the metadata ourselves? Is it cached in device? https://codereview.chromium.org/1141403002/diff/60001/build/android/gyp/apk_i... build/android/gyp/apk_install.py:143: print 'Installing apks:', apk_names maybe drop the print
https://codereview.chromium.org/1141403002/diff/60001/build/android/gyp/apk_i... File build/android/gyp/apk_install.py (right): https://codereview.chromium.org/1141403002/diff/60001/build/android/gyp/apk_i... build/android/gyp/apk_install.py:33: SHELL_CONFIG_COMMAND = ('echo $(getprop persist.sys.language)-' This would seemingly be much more readable as just: _DENSITIES = { 120: 'ldpi', 160: 'mdpi', ... } def RetrieveDeviceConfig(device): language = device.GetProp('persist.sys.language') country = device.GetProp('persist.sys.country') density_px = int(device.GetProp('ro.sf.lcd_density') density = _DENSITIES.get(density_px, 'tvdpi') abi = device.product_cpi_api return '%s-%s-%s:%s' % (language, country, density, abi) (not sure if this device is a DeviceUtils or a BuildDevice -- as written, this assumes the former)
https://codereview.chromium.org/1141403002/diff/60001/build/android/gyp/apk_i... File build/android/gyp/apk_install.py (right): https://codereview.chromium.org/1141403002/diff/60001/build/android/gyp/apk_i... build/android/gyp/apk_install.py:33: SHELL_CONFIG_COMMAND = ('echo $(getprop persist.sys.language)-' On 2015/05/20 01:43:05, jbudorick wrote: > This would seemingly be much more readable as just: > > _DENSITIES = { > 120: 'ldpi', > 160: 'mdpi', > ... > } > > def RetrieveDeviceConfig(device): > language = device.GetProp('persist.sys.language') > country = device.GetProp('persist.sys.country') > density_px = int(device.GetProp('ro.sf.lcd_density') > density = _DENSITIES.get(density_px, 'tvdpi') > abi = device.product_cpi_api > return '%s-%s-%s:%s' % (language, country, density, abi) > > (not sure if this device is a DeviceUtils or a BuildDevice -- as written, this > assumes the former) I should note that this is mildly slower than what you have, but given that this is used while installing APKs, that effect is minimal.
addressed comments. https://codereview.chromium.org/1141403002/diff/60001/build/android/gyp/apk_i... File build/android/gyp/apk_install.py (right): https://codereview.chromium.org/1141403002/diff/60001/build/android/gyp/apk_i... build/android/gyp/apk_install.py:33: SHELL_CONFIG_COMMAND = ('echo $(getprop persist.sys.language)-' On 2015/05/20 01:43:05, jbudorick wrote: > This would seemingly be much more readable as just: > > _DENSITIES = { > 120: 'ldpi', > 160: 'mdpi', > ... > } > > def RetrieveDeviceConfig(device): > language = device.GetProp('persist.sys.language') > country = device.GetProp('persist.sys.country') > density_px = int(device.GetProp('ro.sf.lcd_density') > density = _DENSITIES.get(density_px, 'tvdpi') > abi = device.product_cpi_api > return '%s-%s-%s:%s' % (language, country, density, abi) > > (not sure if this device is a DeviceUtils or a BuildDevice -- as written, this > assumes the former) Done. https://codereview.chromium.org/1141403002/diff/60001/build/android/gyp/apk_i... build/android/gyp/apk_install.py:33: SHELL_CONFIG_COMMAND = ('echo $(getprop persist.sys.language)-' On 2015/05/20 01:44:05, jbudorick wrote: > On 2015/05/20 01:43:05, jbudorick wrote: > > This would seemingly be much more readable as just: > > > > _DENSITIES = { > > 120: 'ldpi', > > 160: 'mdpi', > > ... > > } > > > > def RetrieveDeviceConfig(device): > > language = device.GetProp('persist.sys.language') > > country = device.GetProp('persist.sys.country') > > density_px = int(device.GetProp('ro.sf.lcd_density') > > density = _DENSITIES.get(density_px, 'tvdpi') > > abi = device.product_cpi_api > > return '%s-%s-%s:%s' % (language, country, density, abi) > > > > (not sure if this device is a DeviceUtils or a BuildDevice -- as written, this > > assumes the former) > > I should note that this is mildly slower than what you have, but given that this > is used while installing APKs, that effect is minimal. Yeah, this was my original reasoning, but seems fast enough this way. https://codereview.chromium.org/1141403002/diff/60001/build/android/gyp/apk_i... build/android/gyp/apk_install.py:65: return expected_file.read() != device.GetInstallMetadata(apk_package) On 2015/05/20 01:37:28, cjhopman wrote: > Is the install metadata still valid in a split world? > > Also, why do we use device.GetInstallMetadata here but then when we record it we > get the metadata ourselves? Is it cached in device? The install metadata is an "ls -l" of the apk directory on device. Looks like: drwxr-xr-x system system 2015-05-16 22:35 com.google.android.apps.chrome-2 Since it's listing the directory, I don't think it's affected by splits. https://codereview.chromium.org/1141403002/diff/60001/build/android/gyp/apk_i... build/android/gyp/apk_install.py:143: print 'Installing apks:', apk_names On 2015/05/20 01:37:28, cjhopman wrote: > maybe drop the print Done.
On 2015/05/20 14:33:58, agrieve wrote: > addressed comments. > > https://codereview.chromium.org/1141403002/diff/60001/build/android/gyp/apk_i... > File build/android/gyp/apk_install.py (right): > > https://codereview.chromium.org/1141403002/diff/60001/build/android/gyp/apk_i... > build/android/gyp/apk_install.py:33: SHELL_CONFIG_COMMAND = ('echo $(getprop > persist.sys.language)-' > On 2015/05/20 01:43:05, jbudorick wrote: > > This would seemingly be much more readable as just: > > > > _DENSITIES = { > > 120: 'ldpi', > > 160: 'mdpi', > > ... > > } > > > > def RetrieveDeviceConfig(device): > > language = device.GetProp('persist.sys.language') > > country = device.GetProp('persist.sys.country') > > density_px = int(device.GetProp('ro.sf.lcd_density') > > density = _DENSITIES.get(density_px, 'tvdpi') > > abi = device.product_cpi_api > > return '%s-%s-%s:%s' % (language, country, density, abi) > > > > (not sure if this device is a DeviceUtils or a BuildDevice -- as written, this > > assumes the former) > > Done. > > https://codereview.chromium.org/1141403002/diff/60001/build/android/gyp/apk_i... > build/android/gyp/apk_install.py:33: SHELL_CONFIG_COMMAND = ('echo $(getprop > persist.sys.language)-' > On 2015/05/20 01:44:05, jbudorick wrote: > > On 2015/05/20 01:43:05, jbudorick wrote: > > > This would seemingly be much more readable as just: > > > > > > _DENSITIES = { > > > 120: 'ldpi', > > > 160: 'mdpi', > > > ... > > > } > > > > > > def RetrieveDeviceConfig(device): > > > language = device.GetProp('persist.sys.language') > > > country = device.GetProp('persist.sys.country') > > > density_px = int(device.GetProp('ro.sf.lcd_density') > > > density = _DENSITIES.get(density_px, 'tvdpi') > > > abi = device.product_cpi_api > > > return '%s-%s-%s:%s' % (language, country, density, abi) > > > > > > (not sure if this device is a DeviceUtils or a BuildDevice -- as written, > this > > > assumes the former) > > > > I should note that this is mildly slower than what you have, but given that > this > > is used while installing APKs, that effect is minimal. > > Yeah, this was my original reasoning, but seems fast enough this way. > > https://codereview.chromium.org/1141403002/diff/60001/build/android/gyp/apk_i... > build/android/gyp/apk_install.py:65: return expected_file.read() != > device.GetInstallMetadata(apk_package) > On 2015/05/20 01:37:28, cjhopman wrote: > > Is the install metadata still valid in a split world? > > > > Also, why do we use device.GetInstallMetadata here but then when we record it > we > > get the metadata ourselves? Is it cached in device? > > The install metadata is an "ls -l" of the apk directory on device. Looks like: > > drwxr-xr-x system system 2015-05-16 22:35 > com.google.android.apps.chrome-2 > > > Since it's listing the directory, I don't think it's affected by splits. > > https://codereview.chromium.org/1141403002/diff/60001/build/android/gyp/apk_i... > build/android/gyp/apk_install.py:143: print 'Installing apks:', apk_names > On 2015/05/20 01:37:28, cjhopman wrote: > > maybe drop the print > > Done. bump
lgtm not an owner in build/, though.
lgtm
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1141403002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/16cd4e3dcbac844d194be8518f9b049a35c7634e Cr-Commit-Position: refs/heads/master@{#331205} |