[build/android] Switch hand-rolled 'ls' to StatDirectory

[build/android] Switch hand-rolled 'ls' to StatDirectory

Replace previous hand-rolled shell commands with more robust
alternatives from device_utils.

In particular "install_metadata", which used to be just the
output of "ls -l" on the device, is now a list of dictionaries
with stat file data for each installed package on the device.

BUG=552376
Committed: https://crrev.com/40cb86544a3c7f5fc2cca51dc181624b2995eed8
Cr-Commit-Position: refs/heads/master@{#400123}

[perezju, 2016-06-15 14:54:44]

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

[perezju, 2016-06-15 14:54:45]

Please have a look. I did a few tests running gyp/get_device_configuration.py
and gyp/apk_install.py; but I'm not sure exactly how and where are these scripts
being run today.

https://codereview.chromium.org/2069113002/diff/1/build/android/gyp/apk_insta...
File build/android/gyp/apk_install.py (left):

https://codereview.chromium.org/2069113002/diff/1/build/android/gyp/apk_insta...
build/android/gyp/apk_install.py:39: return matches[0] if matches else None
Replaced below with device.GetInstallMetadata(apk_package, refresh=True) to
avoid code duplication.

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

https://codereview.chromium.org/2069113002/diff/1/build/android/gyp/apk_insta...
build/android/gyp/apk_install.py:36: return True
Give some tolerance for old files (with the output of "ls -l") lying around. We
force install in those cases.

https://codereview.chromium.org/2069113002/diff/1/build/android/gyp/util/buil...
File build/android/gyp/util/build_device.py (right):

https://codereview.chromium.org/2069113002/diff/1/build/android/gyp/util/buil...
build/android/gyp/util/build_device.py:32: 'Invalid BuildDevice configuration')
This will break hard if there are old config files lying around.

https://codereview.chromium.org/2069113002/diff/1/build/android/gyp/util/buil...
build/android/gyp/util/build_device.py:62: apk_pattern =
re.compile('%s(-[0-9]*)?(.apk)?$' % re.escape(apk_package))
Wondering when/if this is actually being run. Note the previous regex doesn't
match apk files with a missing extension (as it happened when I installed
Chrome.apk on a device today).

[agrieve, 2016-06-15 17:25:24]

Change lgtm. One comment which you're free to ignore.

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

https://codereview.chromium.org/2069113002/diff/1/build/android/gyp/apk_insta...
build/android/gyp/apk_install.py:36: return True
On 2016/06/15 14:54:45, perezju wrote:
> Give some tolerance for old files (with the output of "ls -l") lying around.
We
> force install in those cases.

One step even better would be to change this file to use DeviceUtil's built-in
caching (device.DumpCacheData()). Not as easy as enabling a flag, but you can
see how it's used here:
https://cs.chromium.org/chromium/src/build/android/incremental_install/instal...

[jbudorick, 2016-06-15 20:47:37]

https://codereview.chromium.org/2069113002/diff/1/build/android/gyp/util/buil...
File build/android/gyp/util/build_device.py (right):

https://codereview.chromium.org/2069113002/diff/1/build/android/gyp/util/buil...
build/android/gyp/util/build_device.py:62: apk_pattern =
re.compile('%s(-[0-9]*)?(.apk)?$' % re.escape(apk_package))
On 2016/06/15 14:54:45, perezju wrote:
> Wondering when/if this is actually being run. Note the previous regex doesn't
> match apk files with a missing extension (as it happened when I installed
> Chrome.apk on a device today).

Good (?) news -- apk_install.py was only used in gyp (for gyp_managed_install,
iirc), and this function was only used in apk_install.py.

[perezju, 2016-06-16 09:31:28]

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

https://codereview.chromium.org/2069113002/diff/1/build/android/gyp/apk_insta...
build/android/gyp/apk_install.py:36: return True
On 2016/06/15 17:25:24, agrieve wrote:
> On 2016/06/15 14:54:45, perezju wrote:
> > Give some tolerance for old files (with the output of "ls -l") lying around.
> We
> > force install in those cases.
> 
> One step even better would be to change this file to use DeviceUtil's built-in
> caching (device.DumpCacheData()). Not as easy as enabling a flag, but you can
> see how it's used here:
>
https://cs.chromium.org/chromium/src/build/android/incremental_install/instal...
> 
> 

Had a quick look, but the change would be a bit more involved and require some
larger refactoring. The metadata stored here is only for a specific apk, while
DumpCacheData is for the whole device. So prefer to leave it as is for now.

[perezju, 2016-06-16 09:31:35]

The CQ bit was checked by perezju@chromium.org

[commit-bot: I haz the power, 2016-06-16 09:32:15]

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

[commit-bot: I haz the power, 2016-06-16 10:36:45]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-06-16 10:39:05]

Description was changed from

==========
[build/android] Switch hand-rolled 'ls' to StatDirectory

Replace previous hand-rolled shell commands with more robust
alternatives from device_utils.

In particular "install_metadata", which used to be just the
output of "ls -l" on the device, is now a list of dictionaries
with stat file data for each installed package on the device.

BUG=552376
==========

to

==========
[build/android] Switch hand-rolled 'ls' to StatDirectory

Replace previous hand-rolled shell commands with more robust
alternatives from device_utils.

In particular "install_metadata", which used to be just the
output of "ls -l" on the device, is now a list of dictionaries
with stat file data for each installed package on the device.

BUG=552376

Committed: https://crrev.com/40cb86544a3c7f5fc2cca51dc181624b2995eed8
Cr-Commit-Position: refs/heads/master@{#400123}
==========

[commit-bot: I haz the power, 2016-06-16 10:39:06]

Patchset 1 (id:??) landed as
https://crrev.com/40cb86544a3c7f5fc2cca51dc181624b2995eed8
Cr-Commit-Position: refs/heads/master@{#400123}