New run shell implementation for DeviceUtils

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=W0613
 
 import multiprocessing
 import os
 import pipes
+import re
 import sys
 import tempfile
 import time
 import zipfile
 
 import pylib.android_commands
 from pylib.device import adb_wrapper
 from pylib.device import decorators
 from pylib.device import device_errors
 from pylib.device.commands import install_commands
@@ skipping 55 matching lines @@
       self.adb = adb_wrapper.AdbWrapper(device.GetDevice())
       self.old_interface = device
     elif not device:
       self.adb = adb_wrapper.AdbWrapper('')
       self.old_interface = pylib.android_commands.AndroidCommands()
     else:
       raise ValueError('Unsupported type passed for argument "device"')
     self._commands_installed = False
     self._default_timeout = default_timeout
     self._default_retries = default_retries
+    self._cache = {}
     assert(hasattr(self, decorators.DEFAULT_TIMEOUT_ATTR))
     assert(hasattr(self, decorators.DEFAULT_RETRIES_ATTR))
 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def IsOnline(self, timeout=None, retries=None):
     """Checks whether the device is online.
 
     Args:
       timeout: timeout in seconds
       retries: number of retries
@@ skipping 38 matching lines @@
       retries: number of retries
 
     Raises:
       CommandFailedError if root could not be enabled.
       CommandTimeoutError on timeout.
     """
     if not self.old_interface.EnableAdbRoot():
       raise device_errors.CommandFailedError(
           'Could not enable root.', device=str(self))
 
-  @decorators.WithTimeoutAndRetriesFromInstance()
-  def GetExternalStoragePath(self, timeout=None, retries=None):
+  def GetExternalStoragePath(self, timeout=_DEFAULT_TIMEOUT,

[jbudorick, 2014/10/14 16:55:19]

I'm not crazy about moving the timeout/retry boundary way down to the
implementation of AdbWrapper.Shell, which is what this appears to do. I'd like
to stick with handling them on the DeviceUtils boundary.

+                             retries=_DEFAULT_RETRIES):
     """Get the device's path to its SD card.
 
     Args:
       timeout: timeout in seconds
       retries: number of retries
 
     Returns:
       The device's path to its SD card.
 
     Raises:
       CommandFailedError if the external storage path could not be determined.
       CommandTimeoutError on timeout.
       DeviceUnreachableError on missing device.
     """
-    return self._GetExternalStoragePathImpl()
+    if 'external_storage' in self._cache:
+      return self._cache['external_storage']
 
-  def _GetExternalStoragePathImpl(self):
-    try:
-      return self.old_interface.GetExternalStorage()
-    except AssertionError as e:
-      raise device_errors.CommandFailedError(
-          str(e), device=str(self)), None, sys.exc_info()[2]
+    value = self._NewRunShellImpl('echo $EXTERNAL_STORAGE', timeout=timeout,
+                                  retries=retries).rstrip()
+    if not value:

[jbudorick, 2014/10/14 16:55:19]

I'm not sure that this _has_ to be an error (although there may be enough
clients depending on it existing that it should be).

[perezju, 2014/10/15 09:22:54]

This is what the current implementation does, so not sure if clients are
depending on it.

+      raise device_errors.CommandFailedError('$EXTERNAL_STORAGE is not set',
+                                             str(self))
+    self._cache['external_storage'] = value

[jbudorick, 2014/10/14 16:55:19]

I like the idea of caching things like this and certain system properties -- we
do both of these already in AndroidCommands -- but let's talk about the
implementation.

[perezju, 2014/10/15 09:22:54]

Acknowledged.

+    return value
 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def WaitUntilFullyBooted(self, wifi=False, timeout=None, retries=None):
     """Wait for the device to fully boot.
 
     This means waiting for the device to boot, the package manager to be
     available, and the SD card to be ready. It can optionally mean waiting
     for wifi to come up, too.
 
     Args:
@@ skipping 138 matching lines @@
     if check_return:
       code, output = self.old_interface.GetShellCommandStatusAndOutput(
           cmd, timeout_time=timeout)
       if int(code) != 0:
         raise device_errors.AdbCommandFailedError(
             cmd.split(), 'Nonzero exit code (%d)' % code, device=str(self))
     else:
       output = self.old_interface.RunShellCommand(cmd, timeout_time=timeout)
     return output
 
+  def _NewRunShellImpl(self, cmd, cwd=None, env=None, as_root=False,

[jbudorick, 2014/10/14 16:55:19]

I'm not sure about eliminating check_return here. I think the goal is to
eventually switch to default True, but I don't think we want to get rid of it.

[perezju, 2014/10/15 09:22:54]

I would argue that we shouldn't make it easy for clients to ignore errors. If a
non-zero exit code is to be expected, they should just catch the exception and
read the exit code from it. (And we should  make sure that the exception allows
to easily recover the exit code.) Thoughts?

+                       timeout=_DEFAULT_TIMEOUT, retries=_DEFAULT_RETRIES):
+    """Proposed new interface to run ADB shell commands.
+
+    When the command to run |cmd| is given as a string, it will be interpreted
+    and run by the shell on the device.
+
+    Alternatively, one can supply |cmd| as a sequence containing the name of
+    the command to run followed by its arguments. In this case, arguments are
+    passed to the command exactly as given, without any further processing by
+    the shell. This allows to easily pass arguments containing spaces or
+    special characters without having to worry about getting quoting right.
+
+    Args:
+      cmd: A string with the full command to run on the device, or a sequence
+        containing the command and its arguments.
+      cwd: The device directory in which the command should be run.
+      env: The environment variables with which the command should be run.
+      as_root: A boolean indicating whether the shell command should be run
+        with root privileges.
+      timeout: timeout in seconds
+      retries: number of retries
+
+    Returns:
+      The output of the command as a single string.
+
+    Raises:
+      CommandFailedError if the exit status of the command run is non-zero.
+      CommandTimeoutError on timeout.
+    """
+    def env_quote(key, value):
+      if not re.match('^[a-zA-Z_][a-zA-Z0-9_]*$', key):

[jbudorick, 2014/10/14 16:55:19]

Compile the regex as a constant and then use that to do the matching.

[perezju, 2014/10/15 09:22:54]

Acknowledged.

+        raise KeyError('Invalid shell variable name %r' % key)
+      return '%s=%s' % (key, pipes.quote(value))
+
+    if not isinstance(cmd, basestring):
+      cmd = ' '.join(pipes.quote(s) for s in cmd)
+    if as_root and not self._HasRootImpl():
+      cmd = 'su -c %s' % cmd
+    if env is not None:
+      cmd = '%s %s' % (' '.join(env_quote(*kv) for kv in env.iteritems()), cmd)

[jbudorick, 2014/10/14 16:55:19]

This seems a little more readable to me:

  env_quote(k, v) for k, v in env.iteritems()

[perezju, 2014/10/15 09:22:54]

Acknowledged.

+    if cwd is not None:
+      cmd = 'cd %s && %s' % (pipes.quote(cwd), cmd)
+    return self.adb.Shell(cmd, expect_rc=0, timeout=timeout, retries=retries)
+
   @decorators.WithTimeoutAndRetriesFromInstance()
   def KillAll(self, process_name, signum=9, as_root=False, blocking=False,
               timeout=None, retries=None):
     """Kill all processes with the given name on the device.
 
     Args:
       process_name: A string containing the name of the process to kill.
       signum: An integer containing the signal number to send to kill. Defaults
               to 9 (SIGKILL).
       as_root: A boolean indicating whether the kill should be executed with
@@ skipping 276 matching lines @@
 
     self._InstallCommands()
 
     with tempfile.NamedTemporaryFile(suffix='.zip') as zip_file:
       zip_proc = multiprocessing.Process(
           target=DeviceUtils._CreateDeviceZip,
           args=(zip_file.name, files))
       zip_proc.start()
       zip_proc.join()
 
-      zip_on_device = '%s/tmp.zip' % self._GetExternalStoragePathImpl()
+      zip_on_device = '%s/tmp.zip' % self.GetExternalStoragePath()
       try:
         self.adb.Push(zip_file.name, zip_on_device)
         self._RunShellCommandImpl(
             ['unzip', zip_on_device],
             as_root=True, check_return=True,
             env={'PATH': '$PATH:%s' % install_commands.BIN_DIR})
       finally:
         if zip_proc.is_alive():
           zip_proc.terminate()
         if self._IsOnlineImpl():
@@ skipping 329 matching lines @@
       A Parallelizer operating over |devices|.
     """
     if not devices or len(devices) == 0:
       devices = pylib.android_commands.GetAttachedDevices()
     parallelizer_type = (parallelizer.Parallelizer if async
                          else parallelizer.SyncParallelizer)
     return parallelizer_type([
         d if isinstance(d, DeviceUtils) else DeviceUtils(d)
         for d in devices])