remove stale test data on the device

build/android/pylib/device/device_utils.py

Index: build/android/pylib/device/device_utils.py
diff --git a/build/android/pylib/device/device_utils.py b/build/android/pylib/device/device_utils.py
index 72ff55db995e38979bf38fb211946bfba769f344..fa506f285cc9405d88da77267549d3a91a885b46 100644
--- a/build/android/pylib/device/device_utils.py
+++ b/build/android/pylib/device/device_utils.py
@@ -836,6 +836,52 @@ class DeviceUtils(object):
     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)
@@ -866,24 +912,57 @@ class DeviceUtils(object):
           ['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)]
@@ -891,6 +970,8 @@ class DeviceUtils(object):
     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:
@@ -900,10 +981,17 @@ class DeviceUtils(object):
       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: