New run shell implementation for DeviceUtils

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 b726cb90f37e8a41193cef021cacd22b69bf677a..c447200a5685cbdee2db8fe032407ab74f638499 100644
--- a/build/android/pylib/device/device_utils.py
+++ b/build/android/pylib/device/device_utils.py
@@ -11,13 +11,14 @@ Eventually, this will be based on adb_wrapper.
 import logging
 import multiprocessing
 import os
-import pipes
+import re
 import sys
 import tempfile
 import time
 import zipfile
 
 import pylib.android_commands
+from pylib import cmd_helper
 from pylib.device import adb_wrapper
 from pylib.device import decorators
 from pylib.device import device_errors
@@ -28,6 +29,7 @@ from pylib.utils import parallelizer
 
 _DEFAULT_TIMEOUT = 30
 _DEFAULT_RETRIES = 3
+_VALID_SHELL_VARIABLE = re.compile('^[a-zA-Z_][a-zA-Z0-9_]*$')

[jbudorick, 2014/10/17 09:04:53]

Not sure about having this at this scope; it should probably be a constant in
DeviceUtils.

(The other two are out here because they're used by top level functions. All
other constants in this file are defined within DeviceUtils.)

[perezju, 2014/10/17 11:17:09]

Done.

 
 
 @decorators.WithExplicitTimeoutAndRetries(
@@ -87,6 +89,7 @@ class DeviceUtils(object):
     self._commands_installed = None
     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))
 
@@ -127,7 +130,11 @@ class DeviceUtils(object):
     return self._HasRootImpl()
 
   def _HasRootImpl(self):
-    return self.old_interface.IsRootEnabled()
+    try:

[jbudorick, 2014/10/17 09:04:53]

Sneaking a conversion in here, I see.

+      self._RunShellCommandImpl('ls /root')
+      return True
+    except device_errors.AdbShellCommandFailedError:
+      return False
 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def EnableRoot(self, timeout=None, retries=None):
@@ -182,11 +189,15 @@ class DeviceUtils(object):
     return self._GetExternalStoragePathImpl()
 
   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]
+    if 'external_storage' in self._cache:

[jbudorick, 2014/10/17 09:04:53]

Another sneaky conversion.

+      return self._cache['external_storage']
+
+    value = self._RunShellCommandImpl('echo $EXTERNAL_STORAGE').rstrip()
+    if not value:
+      raise device_errors.CommandFailedError('$EXTERNAL_STORAGE is not set',
+                                             str(self))
+    self._cache['external_storage'] = value
+    return value
 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def WaitUntilFullyBooted(self, wifi=False, timeout=None, retries=None):
@@ -216,7 +227,7 @@ class DeviceUtils(object):
     self.old_interface.WaitForSdCardReady(timeout)
     if wifi:
       while not 'Wi-Fi is enabled' in (
-          self._RunShellCommandImpl('dumpsys wifi')):
+          self.old_interface.RunShellCommand('dumpsys wifi')):

[jbudorick, 2014/10/17 09:04:53]

What's up with this? Is this because of the unit test?

[perezju, 2014/10/17 11:17:09]

Yes, because all the others are calls to old_interface, I had to also move back
this call or try writing some DeviceUtilsHybridImplTest with mixed calls to
run_command.RunCommand and adb.Shell. :S

[jbudorick, 2014/10/17 15:48:00]

Hmm. I'm not really a fan of either of those options, but I can't think of a
reasonable alternative.

         time.sleep(1)
 
   REBOOT_DEFAULT_TIMEOUT = 10 * _DEFAULT_TIMEOUT
@@ -292,58 +303,84 @@ class DeviceUtils(object):
             str(e), device=str(self)), None, sys.exc_info()[2]
 
   @decorators.WithTimeoutAndRetriesFromInstance()
-  def RunShellCommand(self, cmd, check_return=False, as_root=False, cwd=None,
-                      env=None, timeout=None, retries=None):
+  def RunShellCommand(self, cmd, check_return=False, want_lines=True,
+                      cwd=None, env=None, as_root=False,
+                      timeout=None, retries=None):
     """Run an ADB shell command.
 
-    TODO(jbudorick) Switch the default value of check_return to True after
-    AndroidCommands is gone.
+    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.
+
+    TODO(perezju) Remove the options |check_return| and |want_lines|,
+      or change their default vaules, when callers have switched to the new
+      behaviour.
 
     Args:
-      cmd: A list containing the command to run on the device and any arguments.
+      cmd: A string with the full command to run on the device, or a sequence
+        containing the command and its arguments.
       check_return: A boolean indicating whether or not the return code should
-                    be checked.
-      as_root: A boolean indicating whether the shell command should be run
-               with root privileges.
+        be checked.
+      want_lines: A boolean indicating whether to return the output of the

[jbudorick, 2014/10/17 09:04:53]

After looking at this and thinking about it, I'm not crazy about this. I'm not
convinced that an entire string _should_ be the new behavior.

Even if we do want to go that way, we shouldn't attempt to do so in this CL. The
switch to AdbWrapper.Shell has sweeping effects on our code as it is.

+        command as a list of lines or a single string.
       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.
+      The output of the command either as list of lines, when want_lines is
+        True, or a single string, otherwise.
 
     Raises:
-      CommandFailedError if check_return is True and the return code is nozero.
+      AdbShellCommandFailedError if check_return is True and the exit code of
+        the command run on the device is non-zero.
       CommandTimeoutError on timeout.
       DeviceUnreachableError on missing device.
     """
-    return self._RunShellCommandImpl(
-        cmd, check_return=check_return, as_root=as_root, cwd=cwd, env=env,
-        timeout=timeout)
+    try:

[jbudorick, 2014/10/17 09:04:53]

This implementation all should move into the _Impl function. The _only_ thing in
here should be the call to _RunShellCommandImpl. The only point of the _Impl
split is to allow calling functions that are already running in their own
timeout/retry thread to avoid spawning another one.

Splitting the functions beyond that will make it more challenging to revisit the
decorator implementation to do something smarter. At the moment, we can revise
the decorator, remove the _Impl functions, and change all calls to _Impl
functions to whatever smarter thing we come up with.

[perezju, 2014/10/17 11:17:09]

Done.

+      output = self._RunShellCommandImpl(cmd, cwd=cwd, env=env, as_root=as_root)
+    except device_errors.AdbShellCommandFailedError as e:
+      if check_return:
+        raise
+      else:
+        output = e.output
+    if want_lines:
+      output = output.splitlines()
+    return output
 
-  def _RunShellCommandImpl(self, cmd, check_return=False, as_root=False,
-                           cwd=None, env=None, timeout=None):
-    # TODO(jbudorick): Remove the timeout parameter once this is no longer
-    # backed by AndroidCommands.
-    if isinstance(cmd, list):
-      cmd = ' '.join(cmd)
+  def _RunShellCommandImpl(self, cmd, cwd=None, env=None, as_root=False):

[jbudorick, 2014/10/17 09:04:53]

You'll have to restore the options you split between the two functions.

+    '''Implementation of RunShellCommand.
+
+    Note that this function already implements the proposed new behaviour of
+    RunShellCommand (i.e, check_return=True, want_lines=False) and is available
+    to be used by other DeviceUtils' methods.
+    '''
+    def env_quote(key, value):
+      if not _VALID_SHELL_VARIABLE.match(key):
+        raise KeyError('Invalid shell variable name %r' % key)
+      # using cmd_helper.dquote to quote but allow interpolation of shell vars
+      return '%s=%s' % (key, cmd_helper.dquote(value))

[jbudorick, 2014/10/17 09:04:53]

Maybe the dquote functionality should just live inside env_quote? I don't
anticipate us using it anywhere else...

+
+    if not isinstance(cmd, basestring):
+      cmd = ' '.join(cmd_helper.quote(s) for s in cmd)

[jbudorick, 2014/10/17 09:04:53]

I really don't like the 'quote = pipes.quote' in practice.

     if as_root and not self._HasRootImpl():
       cmd = 'su -c %s' % cmd
-    if env:
-      cmd = '%s %s' % (
-          ' '.join('%s=%s' % (k, v) for k, v in env.iteritems()), cmd)
-    if cwd:
-      cmd = 'cd %s && %s' % (cwd, cmd)
-    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
+    if env is not None:

[jbudorick, 2014/10/17 09:04:53]

As below, this change only matters if env is empty, and I don't think we want to
do the below in that case.

[perezju, 2014/10/17 11:17:09]

Acknowledged.

+      env = ' '.join(env_quote(k, v) for k, v in env.iteritems())
+      cmd = '%s %s' % (env, cmd)
+    if cwd is not None:

[jbudorick, 2014/10/17 09:04:53]

The only time when this change matters is if cwd is the empty string, and I
don't think we want to add the cwd logic in that case.

+      cmd = 'cd %s && %s' % (cmd_helper.quote(cwd), cmd)
+    # TODO(perezju) still need to make sure that we call a version of adb.Shell
+    # without a timeout-and-retries wrapper.
+    return self.adb.Shell(cmd, expect_rc=0)
 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def KillAll(self, process_name, signum=9, as_root=False, blocking=False,
@@ -371,7 +408,7 @@ class DeviceUtils(object):
       raise device_errors.CommandFailedError(
           'No process "%s"' % process_name, device=str(self))
 
-    cmd = 'kill -%d %s' % (signum, ' '.join(pids.values()))
+    cmd = ['kill', '-%d' % signum] + pids.values()
     self._RunShellCommandImpl(cmd, as_root=as_root)
 
     if blocking:
@@ -523,8 +560,7 @@ class DeviceUtils(object):
     files = []
     for h, d in host_device_tuples:
       if os.path.isdir(h):
-        self._RunShellCommandImpl(['mkdir', '-p', '"%s"' % d],
-                                  check_return=True)
+        self._RunShellCommandImpl(['mkdir', '-p', d])

[jbudorick, 2014/10/17 09:04:53]

following from my comments on the _Impl split, restore check_return=True here.

[perezju, 2014/10/17 11:17:09]

All Done.

       files += self._GetChangedFilesImpl(h, d)
 
     if not files:
@@ -564,8 +600,7 @@ class DeviceUtils(object):
     real_host_path = os.path.realpath(host_path)
     try:
       real_device_path = self._RunShellCommandImpl(
-          ['realpath', device_path], check_return=True)
-      real_device_path = real_device_path[0]
+          ['realpath', device_path]).rstrip()

[jbudorick, 2014/10/17 09:04:53]

and here.

     except device_errors.CommandFailedError:
       return [(host_path, device_path)]
 
@@ -657,7 +692,7 @@ class DeviceUtils(object):
         self.adb.Push(zip_file.name, zip_on_device)
         self._RunShellCommandImpl(
             ['unzip', zip_on_device],
-            as_root=True, check_return=True,
+            as_root=True,

[jbudorick, 2014/10/17 09:04:53]

and here.

             env={'PATH': '$PATH:%s' % install_commands.BIN_DIR})
       finally:
         if zip_proc.is_alive():
@@ -799,8 +834,9 @@ class DeviceUtils(object):
       CommandTimeoutError on timeout.
       DeviceUnreachableError on missing device.
     """
-    self._RunShellCommandImpl('echo {1} > {0}'.format(device_path,
-        pipes.quote(text)), check_return=True, as_root=as_root)
+    cmd = 'echo %s > %s' % (cmd_helper.quote(text),
+                            cmd_helper.quote(device_path))
+    self._RunShellCommandImpl(cmd, as_root=as_root)

[jbudorick, 2014/10/17 09:04:53]

and here.

 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def Ls(self, device_path, timeout=None, retries=None):
@@ -917,7 +953,7 @@ class DeviceUtils(object):
 
   def _GetPidsImpl(self, process_name):
     procs_pids = {}
-    for line in self._RunShellCommandImpl('ps'):
+    for line in self._RunShellCommandImpl('ps').splitlines():
       try:
         ps_data = line.split()
         if process_name in ps_data[-1]: