[Android] Extract MD5sum logic.

build/android/pylib/utils/md5sum.py

+# Copyright 2014 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import collections
+import logging
+import os
+import tempfile
+import types
+
+from pylib import cmd_helper
+from pylib import constants
+from pylib.utils import device_temp_file
+
+HashAndPath = collections.namedtuple('HashAndPath', ['hash', 'path'])
+
+MD5SUM_DEVICE_LIB_PATH = '/data/local/tmp/md5sum/'
+MD5SUM_DEVICE_BIN_PATH = MD5SUM_DEVICE_LIB_PATH + 'md5sum_bin'
+
+MD5SUM_DEVICE_SCRIPT_FORMAT = (
+    'test -f {path} -o -d {path} '

[perezju, 2014/11/20 17:04:53]

why not just 'test -e {path}' ?

[jbudorick, 2014/11/20 19:38:07]

This avoids file types we don't want to md5sum, e.g. named pipes.

[perezju, 2014/11/20 20:38:41]

Ok!

+    '&& LD_LIBRARY_PATH={md5sum_lib} {md5sum_bin} {path}')

[perezju, 2014/11/20 17:04:53]

could we send the LD_LIBRARY_PATH using the RunShellCommand env?

[jbudorick, 2014/11/20 19:38:08]

No, not with the way this script is rolled.

+
+
+def CalculateHostMd5Sums(paths):
+  """Calculates the MD5 sum value for all items in |paths|.
+
+  Args:
+    paths: A list of host paths to md5sum.
+  Returns:
+    A list of named tuples with 'hash' and 'path' attributes.
+  """
+  if not isinstance(paths, list):
+    paths = [paths]

[perezju, 2014/11/20 17:04:53]

I would check 'if isinstance(paths, basestr)' so that, rather than just lists,
we can also pass any kind of sequence. Something like:

cmd = [path_to_md5bin_thing]
if isinstance(paths, basestr):
  cmd.append(paths)
else:
  cmd.extend(paths)

[jbudorick, 2014/11/20 19:38:07]

With the list addition below, we actually do want this to be a list. I switched
this check to be the same as the one below (i.e., it now checks for
GeneratorType, too)

[perezju, 2014/11/20 20:38:42]

But what if I want to pass a tuple? or some other fancy object that just
implements the sequence interface? (granted, we  will probably never do that,
but I think my suggestion was somewhat cleaner and more general, any thoughts on
that?)

[jbudorick, 2014/11/20 20:58:13]

Good point.
(this one seems unlikely, though)
Yeah, I guess that makes sense. Switched.

+  out = cmd_helper.GetCmdOutput(
+      [os.path.join(constants.GetOutDirectory(), 'md5sum_bin_host')] + paths)

[perezju, 2014/11/20 17:04:53]

if GetOutDirectory() is a constant, can we move this join to the top of the
module so we only do it once?

[jbudorick, 2014/11/20 19:38:07]

It's one of the not-quite-constant constants. I was talking to mikecase@ about
those recently; we should talk about them this week.

[perezju, 2014/11/20 20:38:42]

Ok!

+  return [HashAndPath(*l.split(None, 1)) for l in out.splitlines()]
+
+
+def CalculateDeviceMd5Sums(paths, device):
+  """Calculates the MD5 sum value for all items in |paths|.
+
+  Args:
+    paths: A list of device paths to md5sum.
+  Returns:
+    A list of named tuples with 'hash' and 'path' attributes.
+  """
+  if isinstance(paths, types.GeneratorType):
+    paths = list(paths)
+  elif not isinstance(paths, list):
+    paths = [paths]

[perezju, 2014/11/20 17:04:53]

Same here, we could just write:

if isinstance(paths, basestr):
  paths = [paths]

note, there is no need to turn the generator into a list, since we'll only
iterate over it anyway.

[jbudorick, 2014/11/20 19:38:07]

Sticking with this for consistency with the above, although here, you're correct
w.r.t generator -> list conversion since we're only using it once.

[jbudorick, 2014/11/20 20:58:13]

(also switched)

+
+  if not device.FileExists(MD5SUM_DEVICE_BIN_PATH):

[perezju, 2014/11/20 17:04:53]

Probably not something to do now, but maybe DeviceUtils should own these sorts
of checks, so that they can be cached there? (i.e. some "Is Device Tool
Installed"?, or "Install Device Tool if Needed"?)

[jbudorick, 2014/11/20 19:38:07]

Definitely not something to do now, but not a bad idea.

+    device.adb.Push(
+        os.path.join(constants.GetOutDirectory(), 'md5sum_dist'),

[perezju, 2014/11/20 17:04:53]

Same thing about the constant, though.

[jbudorick, 2014/11/20 19:38:07]

same response.

+        MD5SUM_DEVICE_LIB_PATH)
+
+  out = []
+  with tempfile.NamedTemporaryFile() as md5sum_script_file:
+    with device_temp_file.DeviceTempFile(device) as md5sum_device_script_file:
+      md5sum_script = (
+          MD5SUM_DEVICE_SCRIPT_FORMAT.format(
+              path=p, md5sum_lib=MD5SUM_DEVICE_LIB_PATH,
+              md5sum_bin=MD5SUM_DEVICE_BIN_PATH)
+          for p in paths)
+      md5sum_script_file.write('; '.join(md5sum_script))
+      md5sum_script_file.flush()
+      device.adb.Push(md5sum_script_file.name, md5sum_device_script_file.name)
+      out = device.RunShellCommand(['sh', md5sum_device_script_file.name])

[perezju, 2014/11/20 17:04:53]

Why don't we just run the script on the device rather than pushing it through
temp files?

btw, I also thought about this when doing the for each cpu loop, maybe we want a
well written and well tested "RepeatShellCommand" that does the loop for us in
the device?

[perezju, 2014/11/20 17:16:13]

Another btw: looking at md5sum.cc, it also seems that we can pass a list of
files as arguments to the tool.

[jbudorick, 2014/11/20 19:38:07]

Because the script is pretty likely to be longer than 1024 characters, which
means adb would choke on it.
I was considering a RunShellScript that handles the script file logic. I'm not
sure that we do either enough at the moment to warrant a new function in
DeviceUtils.

[jbudorick, 2014/11/20 19:38:08]

We can. I tried doing so (https://codereview.chromium.org/627083002/#ps550001),
but:
 - it's noticeably slower if we prune the list of files beforehand to only those
that exist, and
 - it doesn't really cooperate if we don't.

[perezju, 2014/11/20 20:38:42]

Yeah, I thought about all those problems just after hitting send. Another idea
(for another CL, and when WriteFile is ready), just make RunShellCommand write
the file and run it if the script is too big.

+
+  return [HashAndPath(*l.split(None, 1)) for l in out]
+