[Android] Add zip pushing and refine push mode selection.

[Android] Add zip pushing and refine push mode selection.

This patch revises the implementation of DeviceUtils.PushChangedFiles
and changes its interface (slightly).

PushChangedFiles now:
  - takes a list of (host_path, device_path) tuples, each of which are
    analogous to the original parameters.
  - continues to use AndroidCommands.GetFilesChanged to determine which
    files need pushing
  - attempts to pick the fastest method out of:
    - individual file pushes
    - entire directory pushes
    - zip-push-unzip

BUG=400440
Committed: https://crrev.com/b53e3ccca7fa26a51f3980f250f5527581324316
Cr-Commit-Position: refs/heads/master@{#299098}

[jbudorick, 2014-10-09 12:25:11]

jbudorick@chromium.org changed reviewers:
+ cjhopman@chromium.org, craigdh@chromium.org, klundberg@chromium.org,
tonyg@chromium.org

[jbudorick, 2014-10-09 12:25:11]

Notable changes in step times:
- unit_tests ~6m15s -> ~4m
- content_browsertests ~8m45s -> ~7m30s

previously tested in https://codereview.chromium.org/627083002/

Reviewers:
klundberg: general
craigdh: pylib, particularly PushChangedFiles, since you did the last major
overhaul of PushIfNeeded IIRC
cjhopman: gyp
tonyg: telemetry owners

[craigdh, 2014-10-09 16:21:00]

pylib lgtm.

https://codereview.chromium.org/646523002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/646523002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:537:
self._PushChangedFilesZipped(files)
have you looked into using rsync? It's probably not too hard to build for arm.
http://stackoverflow.com/questions/8130889/build-rsync-for-android

https://codereview.chromium.org/646523002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:632: zip_file.close()
close in a finally?

[craigdh, 2014-10-09 16:22:43]

https://codereview.chromium.org/646523002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/646523002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:632: zip_file.close()
On 2014/10/09 16:21:00, craigdh wrote:
> close in a finally?

Wait, you're already in a "with", you don't need to explicitly close at all.

[jbudorick, 2014-10-09 16:43:59]

https://codereview.chromium.org/646523002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/646523002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:537:
self._PushChangedFilesZipped(files)
On 2014/10/09 16:21:00, craigdh wrote:
> have you looked into using rsync? It's probably not too hard to build for arm.
> http://stackoverflow.com/questions/8130889/build-rsync-for-android

No, although perhaps it's worth looking at later. I'm not crazy about building +
deploying another device binary every run, but if it's faster overall...

https://codereview.chromium.org/646523002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:632: zip_file.close()
On 2014/10/09 16:22:43, craigdh wrote:
> On 2014/10/09 16:21:00, craigdh wrote:
> > close in a finally?
> 
> Wait, you're already in a "with", you don't need to explicitly close at all.

Probably a leftover from a version that didn't use zip_file as a context
manager... removed.

[craigdh, 2014-10-09 16:48:44]

https://codereview.chromium.org/646523002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/646523002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:537:
self._PushChangedFilesZipped(files)
On 2014/10/09 16:43:58, jbudorick wrote:
> On 2014/10/09 16:21:00, craigdh wrote:
> > have you looked into using rsync? It's probably not too hard to build for
arm.
> > http://stackoverflow.com/questions/8130889/build-rsync-for-android
> 
> No, although perhaps it's worth looking at later. I'm not crazy about building
+
> deploying another device binary every run, but if it's faster overall...

Fair enough, but if it works it might save you from having to push md5sum and
the zip utility.

[jbudorick, 2014-10-09 16:58:32]

https://codereview.chromium.org/646523002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/646523002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:537:
self._PushChangedFilesZipped(files)
On 2014/10/09 16:48:44, craigdh wrote:
> On 2014/10/09 16:43:58, jbudorick wrote:
> > On 2014/10/09 16:21:00, craigdh wrote:
> > > have you looked into using rsync? It's probably not too hard to build for
> arm.
> > > http://stackoverflow.com/questions/8130889/build-rsync-for-android
> > 
> > No, although perhaps it's worth looking at later. I'm not crazy about
building
> +
> > deploying another device binary every run, but if it's faster overall...
> 
> Fair enough, but if it works it might save you from having to push md5sum and
> the zip utility.

True, and it would vastly simplify this code. Filed separately as
https://code.google.com/p/chromium/issues/detail?id=421938

[cjhopman, 2014-10-09 17:50:23]

*.gyp/gypi lgtm

https://codereview.chromium.org/646523002/diff/50001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/646523002/diff/50001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:508:
self._RunShellCommandImpl(['mkdir', '-p', '"%s"' % d],
It's may be a little faster to just unconditionally make the directory (since
almost all the cost of file_exists/mkdir is adb overhead).

https://codereview.chromium.org/646523002/diff/50001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:584: zip_dir = tempfile.mkdtemp()
this can use tempfile.NamedTemporaryFile for the zip file instead of the
hand-rolled temp dir.

https://codereview.chromium.org/646523002/diff/50001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:627: for host_path, device_path in
host_device_tuples:
I think this would be simpler with os.walk.

something like:

for host_path, device_path in host_device_tuples:
  for root, _, files in os.walk(host_path):
    hd = os.path.join(host_path, root)
    dd = '%s/%s' % (device_path, root)
    z.write(hd, dd, zipfile.ZIP_STORED)
    for f in files:
      z.write(os.path.join(hd, f), '%s/%s' % (dd, f), zipfile.ZIP_DEFLATED)

[tonyg, 2014-10-09 18:14:01]

lgtm for telemetry owners -- I didn't review the rest of the patch

[jbudorick, 2014-10-09 19:26:40]

https://codereview.chromium.org/646523002/diff/50001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/646523002/diff/50001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:508:
self._RunShellCommandImpl(['mkdir', '-p', '"%s"' % d],
On 2014/10/09 17:50:23, cjhopman wrote:
> It's may be a little faster to just unconditionally make the directory (since
> almost all the cost of file_exists/mkdir is adb overhead).

dropped the _FileExistsImpl call

https://codereview.chromium.org/646523002/diff/50001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:584: zip_dir = tempfile.mkdtemp()
On 2014/10/09 17:50:23, cjhopman wrote:
> this can use tempfile.NamedTemporaryFile for the zip file instead of the
> hand-rolled temp dir.

What I actually want is a temporary file _name_, not an open temporary file
object. NamedTemporaryFile does the latter, and the logic it uses for temporary
name creation is (understandably) not public. Making a temporary directory
ensures that the temporary file I eventually create won't conflict with one
created on another thread accidentally.

I suppose something like:

  zip_file = tempfile.NamedTemporaryFile(suffix='.zip', delete=False)
  zip_file.close()
  zip_path = zip_file.name()
  try:
    ...
  finally:
    os.remove(zip_path)

but I think I prefer what I've got to that.

https://codereview.chromium.org/646523002/diff/50001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:627: for host_path, device_path in
host_device_tuples:
On 2014/10/09 17:50:23, cjhopman wrote:
> I think this would be simpler with os.walk.
> 
> something like:
> 
> for host_path, device_path in host_device_tuples:
>   for root, _, files in os.walk(host_path):
>     hd = os.path.join(host_path, root)
>     dd = '%s/%s' % (device_path, root)
>     z.write(hd, dd, zipfile.ZIP_STORED)
>     for f in files:
>       z.write(os.path.join(hd, f), '%s/%s' % (dd, f), zipfile.ZIP_DEFLATED)
> 
>       

Done with some tweaks.

[cjhopman, 2014-10-09 20:27:39]

https://codereview.chromium.org/646523002/diff/50001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/646523002/diff/50001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:584: zip_dir = tempfile.mkdtemp()
On 2014/10/09 19:26:40, jbudorick wrote:
> On 2014/10/09 17:50:23, cjhopman wrote:
> > this can use tempfile.NamedTemporaryFile for the zip file instead of the
> > hand-rolled temp dir.
> 
> What I actually want is a temporary file _name_, not an open temporary file
> object. NamedTemporaryFile does the latter, and the logic it uses for
temporary
> name creation is (understandably) not public. Making a temporary directory
> ensures that the temporary file I eventually create won't conflict with one
> created on another thread accidentally.
> 
> I suppose something like:
> 
>   zip_file = tempfile.NamedTemporaryFile(suffix='.zip', delete=False)
>   zip_file.close()
>   zip_path = zip_file.name()
>   try:
>     ...
>   finally:
>     os.remove(zip_path)
> 
> but I think I prefer what I've got to that.

You can actually do:

with tempfile.NamedTemporaryFile(suffix='.zip'):
  zip_path = zip_file.name
  ...

and just keep the tempfile open, but it's up to you.

[jbudorick, 2014-10-10 09:33:48]

https://codereview.chromium.org/646523002/diff/50001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/646523002/diff/50001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:584: zip_dir = tempfile.mkdtemp()
On 2014/10/09 20:27:39, cjhopman wrote:
> On 2014/10/09 19:26:40, jbudorick wrote:
> > On 2014/10/09 17:50:23, cjhopman wrote:
> > > this can use tempfile.NamedTemporaryFile for the zip file instead of the
> > > hand-rolled temp dir.
> > 
> > What I actually want is a temporary file _name_, not an open temporary file
> > object. NamedTemporaryFile does the latter, and the logic it uses for
> temporary
> > name creation is (understandably) not public. Making a temporary directory
> > ensures that the temporary file I eventually create won't conflict with one
> > created on another thread accidentally.
> > 
> > I suppose something like:
> > 
> >   zip_file = tempfile.NamedTemporaryFile(suffix='.zip', delete=False)
> >   zip_file.close()
> >   zip_path = zip_file.name()
> >   try:
> >     ...
> >   finally:
> >     os.remove(zip_path)
> > 
> > but I think I prefer what I've got to that.
> 
> You can actually do:
> 
> with tempfile.NamedTemporaryFile(suffix='.zip'):
>   zip_path = zip_file.name
>   ...
> 
> and just keep the tempfile open, but it's up to you.
> 

I was thinking python would choke on having the same file open in two processes,
but it works and results in cleaner code. Changed.

[jbudorick, 2014-10-10 09:38:38]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-10-10 09:39:34]

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

[jbudorick, 2014-10-10 10:15:36]

The CQ bit was unchecked by jbudorick@chromium.org

[jbudorick, 2014-10-10 11:23:00]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-10-10 11:23:24]

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

[commit-bot: I haz the power, 2014-10-10 12:44:06]

Committed patchset #5 (id:440001)

[commit-bot: I haz the power, 2014-10-10 12:44:39]

Patchset 5 (id:??) landed as
https://crrev.com/b53e3ccca7fa26a51f3980f250f5527581324316
Cr-Commit-Position: refs/heads/master@{#299098}