Get rid of device/host clock synchronization in android_commands.py.

build/android/pylib/android_commands.py

Index: build/android/pylib/android_commands.py
diff --git a/build/android/pylib/android_commands.py b/build/android/pylib/android_commands.py
index b5559ed63c7efb4731cd42e5314a6ed6654c0c27..d36cc285ae3e1ca47e7f87d3b70909558cd21a40 100644
--- a/build/android/pylib/android_commands.py
+++ b/build/android/pylib/android_commands.py
@@ -65,6 +65,7 @@ KEYCODE_DPAD_RIGHT = 22
 KEYCODE_ENTER = 66
 KEYCODE_MENU = 82
 
+MD5SUM_DEVICE_PATH = '/data/local/tmp/md5sum'
 
 def GetEmulators():
   """Returns a list of emulators.  Does not filter by status (e.g. offline).
@@ -112,42 +113,6 @@ def GetAttachedDevices():
     devices.insert(0, preferred_device)
   return devices
 
-
-def _GetHostFileInfo(file_name):
-  """Returns a tuple containing size and modified UTC time for file_name."""
-  # The time accuracy on device is only to minute level, remove the second and
-  # microsecond from host results.
-  utc_time = datetime.datetime.utcfromtimestamp(os.path.getmtime(file_name))
-  time_delta = datetime.timedelta(seconds=utc_time.second,
-                                  microseconds=utc_time.microsecond)
-  return os.path.getsize(file_name), utc_time - time_delta
-
-
-def ListHostPathContents(path):
-  """Lists files in all subdirectories of |path|.
-
-  Args:
-    path: The path to list.
-
-  Returns:
-    A dict of {"name": (size, lastmod), ...}.
-  """
-  if os.path.isfile(path):
-    return {os.path.basename(path): _GetHostFileInfo(path)}
-  ret = {}
-  for root, dirs, files in os.walk(path):
-    for d in dirs:
-      if d.startswith('.'):
-        dirs.remove(d)  # Prune the dir for subsequent iterations.
-    for f in files:
-      if f.startswith('.'):
-        continue
-      full_file_name = os.path.join(root, f)
-      file_name = os.path.relpath(full_file_name, path)
-      ret[file_name] = _GetHostFileInfo(full_file_name)
-  return ret
-
-
 def _GetFilesFromRecursiveLsOutput(path, ls_output, re_file, utc_offset=None):
   """Gets a list of files from `ls` command output.
 
@@ -200,6 +165,14 @@ def _GetFilesFromRecursiveLsOutput(path, ls_output, re_file, utc_offset=None):
       files[filename] = (int(file_match.group('size')), lastmod)
   return files
 
+def _ComputeFileListHash(md5sum_output):

[Isaac (away), 2012/08/27 10:43:15]

Consider using a list comprehension:
return [l.split('  ')[0] for l in lines]

[Philippe, 2012/08/28 09:48:29]

Good point, indeed.

+  """Returns a list of MD5 strings from the provided md5sum output."""
+  hashes = []
+  lines = md5sum_output

[Isaac (away), 2012/08/27 10:43:15]

Unnecessary variable?

[Philippe, 2012/08/28 09:48:29]

Done.

+  for line in lines:
+    hashes.append(line.split('  ')[0])
+  return hashes
+
 
 def GetLogTimestamp(log_line, year):
   """Returns the timestamp of the given |log_line| in the given year."""
@@ -227,6 +200,19 @@ class AndroidCommands(object):
     self._original_governor = None
     self._pushed_files = []
     self._device_utc_offset = self.RunShellCommand('date +%z')[0]
+    chrome_src_path = os.getenv('CHROME_SRC')

[Isaac (away), 2012/08/27 10:43:15]

prefer we avoid new environment variables dependencies.  Look at the way we
import android_testrunner at the top of file, maybe you can look at CHROME_SRC
but have a viable default value.

[Philippe, 2012/08/28 09:48:29]

$CHROME_SRC is not new but indeed it's better to use CHROME_SRC defined at the
top of the file.

+    assert chrome_src_path
+    self._md5sum_path = '%s/out/Debug/md5sum' % (chrome_src_path)

[Isaac (away), 2012/08/27 10:43:15]

rather than guessing Release vs. Debug, maybe take optional arguments.

[Philippe, 2012/08/28 09:48:29]

Unfortunately some clients of AndroidCommands don't know the build type and have
no simple way to figure it out I think (e.g. buildbot_functions.sh).
 
The problem we have right now that I already faced last week is that we don't
clearly export the build type.
We should export $BUILD_TYPE very early (e.g. in envsetup.sh) rather than
setting it in some late python scripts and then passing it around (we could put
it in constants.py and set it from $BUILD_TYPE).
What do you think?
Xianzhu should be part of this discussion too.

About this specific case, we can also push the md5sum binary in all cases. It
would be simpler and probably safer without impacting tests time too much
assuming that clients don't instantiate AndroidCommands() in a loop.

[Isaac (away), 2012/08/28 17:12:11]

I've recently added the build type to buildbot factory properties and as of
yesterday, buildbot_functions extract this from factory properties.
(https://chromiumcodereview.appspot.com/10878031/).  But better to parse factory
properties in python.  Look at bb_zip_build in buildbot_functions and
zip_build.py for how the parsing works.  Basically:

# /b/build/slave/scripts is in buildbot pythonpath
from common import chromium_utils
....
chromium_utils.AddPropertiesOptions(parser)
options, args = parser.parse_args()

target = options.factory_properties['target']

[Philippe, 2012/08/29 08:36:45]

What about non-buildbot environments (i.e. developer environments)?
That's why I wanted to handle that in envsetup so that all environments are
affected.
I think it's good to try to keep bots/developer environments consistent so that
we can easily debug locally.
Am I misunderstanding something?

[Isaac (away), 2012/08/30 06:50:57]

Good point.  I think this is OK for now then.

+    if not os.path.exists(self._md5sum_path):
+      self._md5sum_path = '%s/out/Release/md5sum' % (chrome_src_path)
+      if not os.path.exists(self._md5sum_path):
+        print >>sys.stderr, 'Please build md5sum (\'make md5sum\')'
+        sys.exit(1)
+    # Push the md5sum binary to the device if needed.
+    if not self.FileExistsOnDevice(MD5SUM_DEVICE_PATH):
+      command = 'push %s %s' % (self._md5sum_path, MD5SUM_DEVICE_PATH)
+      logging.info(command)
+      assert self._adb.SendCommand(command)
 
   def Adb(self):
     """Returns our AdbInterface to avoid us wrapping all its methods."""
@@ -263,10 +249,6 @@ class AndroidCommands(object):
         self.RestartShell()
     raise last_err  # Only reached after max retries, re-raise the last error.
 
-  def SynchronizeDateTime(self):
-    """Synchronize date/time between host and device."""
-    self._adb.SendShellCommand('date -u %f' % time.time())
-
   def RestartShell(self):
     """Restarts the shell on the device. Does not block for it to return."""
     self.RunShellCommand('stop')
@@ -546,26 +528,20 @@ class AndroidCommands(object):
     """Pushes |local_path| to |device_path|.
 
     Works for files and directories. This method skips copying any paths in
-    |test_data_paths| that already exist on the device with the same timestamp
-    and size.
+    |test_data_paths| that already exist on the device with the same md5.
 
     All pushed files can be removed by calling RemovePushedFiles().
     """
     assert os.path.exists(local_path), 'Local path not found %s' % local_path
     self._pushed_files.append(device_path)
 
-    # If the path contents are the same, there's nothing to do.
-    local_contents = ListHostPathContents(local_path)
-    device_contents = self.ListPathContents(device_path)
-    # Only compare the size and timestamp if only copying a file because
-    # the filename on device can be renamed.
-    if os.path.isfile(local_path):
-      assert len(local_contents) == 1
-      is_equal = local_contents.values() == device_contents.values()
-    else:
-      is_equal = local_contents == device_contents
-    if is_equal:
-      logging.info('%s is up-to-date. Skipping file push.', device_path)
+    hashes_on_device = _ComputeFileListHash(
+        self.RunShellCommand(MD5SUM_DEVICE_PATH + ' ' + device_path))

[Isaac (away), 2012/08/27 10:43:15]

Push md5sum binary if not on device?

[Philippe, 2012/08/28 09:48:29]

This is in the constructor on line 212. I avoided putting the check here (needed
only once) since FileExistsOnDevice() is surprisingly quite expensive (~150ms
IIRC).

+    assert os.path.exists(local_path), 'Local path not found %s' % local_path
+    hashes_on_host = _ComputeFileListHash(
+        subprocess.Popen('%s_host_bin %s' % (self._md5sum_path, local_path),

[Isaac (away), 2012/08/27 10:43:15]

Look at cmd_helper.py :: GetCmdOutput()

[Philippe, 2012/08/28 09:48:29]

Thanks.

FYI, I'm using shell=True because subprocess.Popen() uses execvp() with
shell=False which only works with a relative path.

+                         shell=True, stdout=subprocess.PIPE).stdout)
+    if hashes_on_device == hashes_on_host:
       return
 
     # They don't match, so remove everything first and then create it.
@@ -987,3 +963,17 @@ class AndroidCommands(object):
         break
     logging.info('PidsUsingDevicePort: %s', pids)
     return pids
+
+  def FileExistsOnDevice(self, file_name):
+    """Checks whether the given (regular) file exists on the device.
+
+    Args:
+      file_name: Full path of file to check.
+
+    Returns:
+      True if the file exists, False otherwise.
+    """
+    assert '"' not in file_name, 'file_name cannot contain double quotes'
+    status = self._adb.SendShellCommand(

[Isaac (away), 2012/08/27 10:43:15]

1) Prefer self.RunShellCommand() (they do the same thing)

2) This seems a bit hacky.  Would ls -a work better?

[Philippe, 2012/08/28 09:48:29]

This is coming from downstream. I personally think test -f is nice but using $?
is shorter.
Which part did you not like?

[Isaac (away), 2012/08/28 17:12:11]

Quote escaping, as that will be hard to understand later.  Can you add a comment
about why you needed to do it like that?

+        '"test -f \\"%s\\" && echo 1 || echo 0"' % (file_name))
+    return int(status) == 1