remove stale test data on the device

build/android/pylib/device/device_utils.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.
 
 """Provides a variety of device interactions based on adb.
 
 Eventually, this will be based on adb_wrapper.
 """
 # pylint: disable=unused-argument
 
@@ skipping 818 matching lines @@
 
     files = []
     for h, d in host_device_tuples:
       if os.path.isdir(h):
         self.RunShellCommand(['mkdir', '-p', d], check_return=True)
       files += self._GetChangedFilesImpl(h, d)
 
     if not files:
       return
 
+    self._PushFilesImpl(host_device_tuples, files)
+
+  def _GetChangedFilesImpl(self, host_path, device_path):
+    (real_host_path, real_device_path) = self._GetRealHostDevicePaths(
+        host_path, device_path)
+    if not real_device_path:
+      return [(host_path, device_path)]
+
+    try:
+      host_checksums = md5sum.CalculateHostMd5Sums([real_host_path])
+      device_paths_to_md5 = (
+          real_device_path if os.path.isfile(real_host_path)
+          else ('%s/%s' % (real_device_path, os.path.relpath(p, real_host_path))
+                for p in host_checksums.iterkeys()))
+      device_checksums = md5sum.CalculateDeviceMd5Sums(
+          device_paths_to_md5, self)
+    except EnvironmentError as e:
+      logging.warning('Error calculating md5: %s', e)
+      return [(host_path, device_path)]
+
+    if os.path.isfile(host_path):
+      host_checksum = host_checksums.get(real_host_path)
+      device_checksum = device_checksums.get(real_device_path)
+      if host_checksum != device_checksum:
+        return [(host_path, device_path)]
+      else:
+        return []
+    else:
+      to_push = []
+      for host_abs_path, host_checksum in host_checksums.iteritems():
+        device_abs_path = '%s/%s' % (
+            real_device_path, os.path.relpath(host_abs_path, real_host_path))
+        if (device_checksums.get(device_abs_path) != host_checksum):
+          to_push.append((host_abs_path, device_abs_path))
+      return to_push
+
+  def _GetRealHostDevicePaths(self, host_path, device_path):
+    real_host_path = os.path.realpath(host_path)
+    try:
+      real_device_path = self.RunShellCommand(
+          ['realpath', device_path], single_line=True, check_return=True)
+    except device_errors.CommandFailedError:
+      real_device_path = None
+    return (real_host_path, real_device_path)
+
+  def _PushFilesImpl(self, host_device_tuples, files):
     size = sum(host_utils.GetRecursiveDiskUsage(h) for h, _ in files)
     file_count = len(files)
     dir_size = sum(host_utils.GetRecursiveDiskUsage(h)
                    for h, _ in host_device_tuples)
     dir_file_count = 0
     for h, _ in host_device_tuples:
       if os.path.isdir(h):
         dir_file_count += sum(len(f) for _r, _d, f in os.walk(h))
       else:
         dir_file_count += 1
@@ skipping 10 matching lines @@
         dir_push_duration < zip_duration or not self._commands_installed):
       self._PushChangedFilesIndividually(host_device_tuples)
     elif push_duration < zip_duration or not self._commands_installed:
       self._PushChangedFilesIndividually(files)
     else:
       self._PushChangedFilesZipped(files)
       self.RunShellCommand(
           ['chmod', '-R', '777'] + [d for _, d in host_device_tuples],
           as_root=True, check_return=True)
 
-  def _GetChangedFilesImpl(self, host_path, device_path):
-    real_host_path = os.path.realpath(host_path)
-    try:
-      real_device_path = self.RunShellCommand(
-          ['realpath', device_path], single_line=True, check_return=True)
-    except device_errors.CommandFailedError:
-      real_device_path = None
+  PUSH_AND_DELETE_FILES_DEFAULT_TIMEOUT = 10 * _DEFAULT_TIMEOUT
+  PUSH_AND_DELETE_FILES_DEFAULT_RETRIES = _DEFAULT_RETRIES
+
+  @decorators.WithTimeoutAndRetriesDefaults(
+      PUSH_AND_DELETE_FILES_DEFAULT_TIMEOUT,
+      PUSH_AND_DELETE_FILES_DEFAULT_RETRIES)
+  def PushAndDeleteFiles(self, device_dir, host_device_tuples, timeout=None,
+                         retries=None):
+    """Push changed files to the device, delete stale files,
+    skipping files that don't need updating.
+
+    Args:
+      device_dir: the absolute path of the directory on the device which
+        has all the data dependencies
+      host_device_tuples: A list of (host_path, device_path) tuples, where
+        |host_path| is an absolute path of a file or directory on the host
+        that should be minimially pushed to the device, and |device_path| is
+        an absolute path of the destination on the device.
+      timeout: timeout in seconds
+      retries: number of retries
+
+    Raises:
+      CommandFailedError on failure.
+      CommandTimeoutError on timeout.
+      DeviceUnreachableError on missing device.
+    """
+
+    real_device_dir_path = self.RunShellCommand(
+        ['realpath', device_dir], single_line=True, check_return=True)
+    device_checksums = md5sum.CalculateDeviceMd5Sums(
+        [real_device_dir_path], self)
+
+    files = []
+    for h, d in host_device_tuples:
+      if os.path.isdir(h):
+        self.RunShellCommand(['mkdir', '-p', d], check_return=True)
+      files += self._GetChangedFilesRemoveStaleFiles(h, d, device_checksums)
+
+    if files:
+      self._PushFilesImpl(host_device_tuples, files)
+    self._DeleteStaleFiles(device_checksums.keys())
+
+  def _GetChangedFilesRemoveStaleFiles(self, host_path, device_path,

[jbudorick, 2015/06/02 15:30:50]

This function has some significant issues:
 - it duplicates too much code from _GetChangedFiles
 - it isn't actually deleting anything
 - it's modifying one of its arguments in-place with the intention of the caller
both knowing and using the modified argument

_GetChangedFiles currently returns a list of (host_file, device_file) pairs for
each file that exists on the host that should be pushed to the device. This can
include both newly added files and changed files. Perhaps we could change what
it returns to somehow also capture what isn't present on the host?

[Menglin, 2015/06/02 16:31:44]

device_checksums is calculated in each _GetChangedFiles because it takes each
(host, device) pair as the arguments. But it is not the case in
_GetChangedFilesRemoveStaleFiles. device_checksums is calculated before
_GetChangedFilesRemoveStaleFiles and passed in to it to avoid calculating again
in the function. So _GetChangedFiles' arguments and returns need to be changed,
i.e., add a device_checksums=None as the argument, return (push_list,
delete_list) as a tuple. 

[jbudorick, 2015/06/08 18:34:29]

I'm not sure about that particular implementation, but I'm fine with changing
what _GetChangedFiles returns. Maybe instead of just a list of things to push,
it returns files and hashes in cases where the two don't match, e.g.

[
  {
    'host': {
      'path': '/path/to/host/file',
      'hash': '0123456789abcdeffedcba9876543210',
    },
    'device': None,
  },
  {
    'host': None,
    'device': {
      'path': '/path/to/device/file',
      'hash': '0123456789abcdeffedcba9876543210',
    },
  },
  # etc
]

[perezju, 2015/06/09 13:07:26]

I had a good thought about this.

First, I'm not sure whether we want to have a new function PushAndDeleteFiles or
just a 'delete_device_stale=True/False' option on 'PushChangedFiles'.

In either case, I think the cleanest approach would be to have a new
_GetChangedAndStaleFiles(host_path, device_path) which replaces _GetChangedFiles
and returns two lists: (file_tuples_to_push, device_stale_files).

I also think the logic would be simplified if the implementation starts with an
if, branching for two different implementations whether host_path is a file or a
directory (the list of device_stale_files should only be populated in the later
case).

[Menglin, 2015/06/09 22:11:44]

Done.

[Menglin, 2015/06/09 22:11:44]

Done.

[Menglin, 2015/06/09 22:11:45]

Done.

[Menglin, 2015/06/09 22:11:45]

Acknowledged.

+                                        device_checksums):
+    (real_host_path, real_device_path) = self._GetRealHostDevicePaths(
+        host_path, device_path)
     if not real_device_path:
       return [(host_path, device_path)]
 
     try:
       host_checksums = md5sum.CalculateHostMd5Sums([real_host_path])
-      device_paths_to_md5 = (
-          real_device_path if os.path.isfile(real_host_path)
-          else ('%s/%s' % (real_device_path, os.path.relpath(p, real_host_path))
-                for p in host_checksums.iterkeys()))
-      device_checksums = md5sum.CalculateDeviceMd5Sums(
-          device_paths_to_md5, self)
     except EnvironmentError as e:
       logging.warning('Error calculating md5: %s', e)
       return [(host_path, device_path)]
 
     if os.path.isfile(host_path):
       host_checksum = host_checksums.get(real_host_path)
       device_checksum = device_checksums.get(real_device_path)
+      if real_device_path in device_checksums:
+        del device_checksums[real_device_path]
       if host_checksum != device_checksum:
         return [(host_path, device_path)]
       else:
         return []
     else:
       to_push = []
       for host_abs_path, host_checksum in host_checksums.iteritems():
         device_abs_path = '%s/%s' % (
             real_device_path, os.path.relpath(host_abs_path, real_host_path))
-        if (device_checksums.get(device_abs_path) != host_checksum):
+        device_checksum = device_checksums.get(device_abs_path)
+        if device_abs_path in device_checksums:
+          del device_checksums[device_abs_path]
+        if (device_checksum != host_checksum):
           to_push.append((host_abs_path, device_abs_path))
       return to_push
 
+  def _DeleteStaleFiles(self, stale_files):
+    for stale_file_path in stale_files:
+      self.RunShellCommand(['rm', stale_file_path])
+
   def _InstallCommands(self):
     if self._commands_installed is None:
       try:
         if not install_commands.Installed(self):
           install_commands.InstallCommands(self)
         self._commands_installed = True
       except Exception as e:
         logging.warning('unzip not available: %s' % str(e))
         self._commands_installed = False
 
@@ skipping 694 matching lines @@
     blacklist = device_blacklist.ReadBlacklist()
     def blacklisted(adb):
       if adb.GetDeviceSerial() in blacklist:
         logging.warning('Device %s is blacklisted.', adb.GetDeviceSerial())
         return True
       return False
 
     return [cls(adb) for adb in adb_wrapper.AdbWrapper.Devices()
             if not blacklisted(adb)]