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

build/android/pylib/android_commands.py

 # Copyright (c) 2012 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.
 
 """Provides an interface to communicate with the device via the adb command.
 
 Assumes adb binary is currently on system path.
 """
 
 import collections
 import datetime
 import logging
 import os
 import re
 import shlex
 import subprocess
 import sys
 import tempfile
 import time
 
 import pexpect
 import io_stats_parser
 
 # adb_interface.py is under ../../../third_party/android_testrunner/
 sys.path.append(os.path.join(os.path.abspath(os.path.dirname(__file__)), '..',

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

Extract part of this into a var named CHROME_SRC?

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

Done.

                              '..', '..', 'third_party', 'android_testrunner'))
 import adb_interface
 import cmd_helper
 import errors  #  is under ../../../third_party/android_testrunner/errors.py
 
 
 # Pattern to search for the next whole line of pexpect output and capture it
 # into a match group. We can't use ^ and $ for line start end with pexpect,
 # see http://www.noah.org/python/pexpect/#doc for explanation why.
 PEXPECT_LINE_RE = re.compile('\n([^\r]*)\r')
@@ skipping 22 matching lines @@
 
 # Keycode "enum" suitable for passing to AndroidCommands.SendKey().
 KEYCODE_HOME = 3
 KEYCODE_BACK = 4
 KEYCODE_DPAD_UP = 19
 KEYCODE_DPAD_DOWN = 20
 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).
 
   Both devices starting with 'emulator' will be returned in below output:
 
     * daemon not running. starting it now on port 5037 *
     * daemon started successfully *
     List of devices attached
     027c10494100b4d7        device
@@ skipping 27 matching lines @@
     emulator-5554   offline
   """
   re_device = re.compile('^([a-zA-Z0-9_:.-]+)\tdevice$', re.MULTILINE)
   devices = re_device.findall(cmd_helper.GetCmdOutput(['adb', 'devices']))
   preferred_device = os.environ.get('ANDROID_SERIAL')
   if preferred_device in devices:
     devices.remove(preferred_device)
     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.
 
   Python's os.walk isn't used because it doesn't work over adb shell.
 
   Args:
     path: The path to list.
     ls_output: A list of lines returned by an `ls -lR` command.
     re_file: A compiled regular expression which parses a line into named groups
         consisting of at minimum "filename", "date", "time", "size" and
@@ skipping 32 matching lines @@
         utc_offset = file_match.group('timezone')
       if isinstance(utc_offset, str) and len(utc_offset) == 5:
         utc_delta = datetime.timedelta(hours=int(utc_offset[1:3]),
                                        minutes=int(utc_offset[3:5]))
         if utc_offset[0:1] == '-':
           utc_delta = -utc_delta
         lastmod -= utc_delta
       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."""
   try:
     return datetime.datetime.strptime('%s-%s' % (year, log_line[:18]),
                                       '%Y-%m-%d %H:%M:%S.%f')
   except (ValueError, IndexError):
     logging.critical('Error reading timestamp from ' + log_line)
     return None
 
 
 class AndroidCommands(object):
   """Helper class for communicating with Android device via adb.
 
   Args:
     device: If given, adb commands are only send to the device of this ID.
         Otherwise commands are sent to all attached devices.
   """
 
   def __init__(self, device=None):
     self._adb = adb_interface.AdbInterface()
     if device:
       self._adb.SetTargetSerial(device)
     self._logcat = None
     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."""
     return self._adb
 
   def IsRootEnabled(self):
     """Checks if root is enabled on the device."""
     root_test_output = self.RunShellCommand('ls /root') or ['']
     return not 'Permission denied' in root_test_output[0]
 
@@ skipping 16 matching lines @@
       try:
         self._adb.WaitForDevicePm()
         return  # Success
       except errors.WaitForResponseTimedOutError as e:
         last_err = e
         logging.warning('Restarting and retrying after timeout: %s', e)
         retries -= 1
         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')
     self.RunShellCommand('start')
 
   def Reboot(self, full_reboot=True):
     """Reboots the device and waits for the package manager to return.
 
     Args:
       full_reboot: Whether to fully reboot the device or just restart the shell.
@@ skipping 259 matching lines @@
 
     Args:
       keycode: Numeric keycode to send (see "enum" at top of file).
     """
     self.RunShellCommand('input keyevent %d' % keycode)
 
   def PushIfNeeded(self, local_path, device_path):
     """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.
     if os.path.isdir(local_path):
       self.RunShellCommand('rm -r %s' % device_path, timeout_time=2*60)
       self.RunShellCommand('mkdir -p %s' % device_path)
 
     # NOTE: We can't use adb_interface.Push() because it hardcodes a timeout of
     # 60 seconds which isn't sufficient for a lot of users of this method.
     push_command = 'push %s %s' % (local_path, device_path)
@@ skipping 401 matching lines @@
           if len(process_results) <= 8:
             continue
           # Column 0 is the executable name
           # Column 1 is the pid
           # Column 8 is the Inode in use
           if process_results[8] == socket_name:
             pids.append((int(process_results[1]), process_results[0]))
         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