Allow RunShellCommand to work even with very large commands

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
 
@@ skipping 40 matching lines @@
   """Restarts the adb server.
 
   Raises:
     CommandFailedError if we fail to kill or restart the server.
   """
   pylib.android_commands.AndroidCommands().RestartAdbServer()
 
 
 class DeviceUtils(object):
 
+  _MAX_ADB_COMMAND_LENGTH = 512
   _VALID_SHELL_VARIABLE = re.compile('^[a-zA-Z_][a-zA-Z0-9_]*$')
 
   def __init__(self, device, default_timeout=_DEFAULT_TIMEOUT,
                default_retries=_DEFAULT_RETRIES):
     """DeviceUtils constructor.
 
     Args:
       device: Either a device serial, an existing AdbWrapper instance, an
               an existing AndroidCommands instance, or nothing.
       default_timeout: An integer containing the default number of seconds to
@@ skipping 291 matching lines @@
     if should_install:
       try:
         out = self.old_interface.Install(apk_path, reinstall=reinstall)
         for line in out.splitlines():
           if 'Failure' in line:
             raise device_errors.CommandFailedError(line.strip(), str(self))
       except AssertionError as e:
         raise device_errors.CommandFailedError(
             str(e), str(self)), None, sys.exc_info()[2]
 
+  def _PrepareShellCommand(self, cmd, cwd=None, env=None, as_root=False):

[jbudorick, 2014/11/27 17:17:34]

Why is this separate from RunShellCommand? Would we ever call it if we weren't
subsequently calling RunShellCommand?

[perezju, 2014/11/28 15:39:34]

see below vv

+    def env_quote(key, value):
+      if not DeviceUtils._VALID_SHELL_VARIABLE.match(key):
+        raise KeyError('Invalid shell variable name %r' % key)
+      # using double quotes here to allow interpolation of shell variables
+      return '%s=%s' % (key, cmd_helper.DoubleQuote(value))
+
+    if not isinstance(cmd, basestring):
+      cmd = ' '.join(cmd_helper.SingleQuote(s) for s in cmd)
+    if env:
+      env = ' '.join(env_quote(k, v) for k, v in env.iteritems())
+      cmd = '%s %s' % (env, cmd)
+    if cwd:
+      cmd = 'cd %s && %s' % (cmd_helper.SingleQuote(cwd), cmd)
+    if as_root and self.NeedsSU():
+      # "su -c sh -c" allows using shell features in |cmd|
+      cmd = 'su -c sh -c %s' % cmd_helper.SingleQuote(cmd)
+    return cmd
+
   @decorators.WithTimeoutAndRetriesFromInstance()
   def RunShellCommand(self, cmd, check_return=False, cwd=None, env=None,
                       as_root=False, single_line=False,
                       timeout=None, retries=None):
     """Run an ADB shell command.
 
     The command to run |cmd| should be a sequence of program arguments or else
     a single string.
 
     When |cmd| is a sequence, it is assumed to contain the name of the command
@@ skipping 32 matching lines @@
       (with the optional newline at the end stripped).
 
     Raises:
       AdbCommandFailedError if check_return is True and the exit code of
         the command run on the device is non-zero.
       CommandFailedError if single_line is True but the output contains two or
         more lines.
       CommandTimeoutError on timeout.
       DeviceUnreachableError on missing device.
     """
-    def env_quote(key, value):
-      if not DeviceUtils._VALID_SHELL_VARIABLE.match(key):
-        raise KeyError('Invalid shell variable name %r' % key)
-      # using double quotes here to allow interpolation of shell variables
-      return '%s=%s' % (key, cmd_helper.DoubleQuote(value))
+    def do_run(cmd):
+      try:
+        return self.adb.Shell(cmd)
+      except device_errors.AdbCommandFailedError as exc:
+        if check_return:
+          raise
+        else:
+          return exc.output
 
-    if not isinstance(cmd, basestring):
-      cmd = ' '.join(cmd_helper.SingleQuote(s) for s in cmd)
-    if env:
-      env = ' '.join(env_quote(k, v) for k, v in env.iteritems())
-      cmd = '%s %s' % (env, cmd)
-    if cwd:
-      cmd = 'cd %s && %s' % (cmd_helper.SingleQuote(cwd), cmd)
-    if as_root and self.NeedsSU():
-      # "su -c sh -c" allows using shell features in |cmd|
-      cmd = 'su -c sh -c %s' % cmd_helper.SingleQuote(cmd)
     if timeout is None:
       timeout = self._default_timeout
 
-    try:
-      output = self.adb.Shell(cmd)
-    except device_errors.AdbCommandFailedError as e:
-      if check_return:
-        raise
-      else:
-        output = e.output
+    cmd = self._PrepareShellCommand(cmd, cwd=cwd, env=env, as_root=as_root)
+    if len(cmd) < self._MAX_ADB_COMMAND_LENGTH:
+      output = do_run(cmd)
+    else:
+      with device_temp_file.DeviceTempFile(self.adb, suffix='.sh') as script:
+        self.PushContents(cmd, script.name)
+        output = do_run('sh %s' % script.name_quoted)
 
     output = output.splitlines()
     if single_line:
       if not output:
         return ''
       elif len(output) == 1:
         return output[0]
       else:
         msg = 'one line of output was expected, but got: %s'
         raise device_errors.CommandFailedError(msg % output, str(self))
@@ skipping 417 matching lines @@
     # the implementation switch, and if file not found should raise exception.
     if as_root:
       if not self.old_interface.CanAccessProtectedFileContents():
         raise device_errors.CommandFailedError(
           'Cannot read from %s with root privileges.' % device_path)
       return self.old_interface.GetProtectedFileContents(device_path)
     else:
       return self.old_interface.GetFileContents(device_path)
 
   @decorators.WithTimeoutAndRetriesFromInstance()
+  def PushContents(self, contents, device_path, timeout=None, retries=None):

[jbudorick, 2014/11/27 17:17:34]

I don't think this should be part of the public interface. Also, the argument
order should be consistent with the argument order of WriteFile.

[perezju, 2014/11/28 15:39:34]

The argument order is consistent with that of Push, and yeah, the fact that it's
"the other way around" w.r.t. WriteFile has confused me  already more than once.

Maybe I'll make it private, call it "_WriteFileWithPush", and keep the  argument
order the same as WriteFile.

[jbudorick, 2014/11/28 18:49:01]

The three should probably be consistent. Perhaps we should change WriteFile's
order.

[perezju, 2014/11/28 19:43:37]

I'm not sure we want to do that. That would require changing client calls all
over the place. More importantly, I'm not quite sure what the "right order"
should be (I can make arguments both ways). I'm sure we have other
WriteToFie-like functions in other places, and of course some python libraries
should have this sort of methods as well. Maybe we should explore those first,
see if there are any patterns, and try to come up with a reasonable convention.

+    """Pushes |contents| to a file on the device.
+
+    Note: If the contents are small, or root access is needed, then |WriteFile|
+    is a better alternative.
+
+    Args:
+      contents: A string containing the data to write to the device.
+      device_path: A string containing the absolute path to the file to write
+          on the device.
+      timeout: timeout in seconds
+      retries: number of retries
+
+    Raises:
+      CommandFailedError if the file could not be written on the device.
+      CommandTimeoutError on timeout.
+      DeviceUnreachableError on missing device.
+    """
+    with tempfile.NamedTemporaryFile() as host_temp:
+      host_temp.write(contents)
+      host_temp.flush()
+      self.adb.Push(host_temp.name, device_path)
+
+  @decorators.WithTimeoutAndRetriesFromInstance()
   def WriteFile(self, device_path, contents, as_root=False, force_push=False,
                 timeout=None, retries=None):
     """Writes |contents| to a file on the device.
 
     Args:
       device_path: A string containing the absolute path to the file to write
           on the device.
       contents: A string containing the data to write to the device.
       as_root: A boolean indicating whether the write should be executed with
           root privileges (if available).
       force_push: A boolean indicating whether to force the operation to be
           performed by pushing a file to the device. The default is, when the
           contents are short, to pass the contents using a shell script instead.
       timeout: timeout in seconds
       retries: number of retries
 
     Raises:
       CommandFailedError if the file could not be written on the device.
       CommandTimeoutError on timeout.
       DeviceUnreachableError on missing device.
     """
-    if len(contents) < 512 and not force_push:
-      cmd = 'echo -n %s > %s' % (cmd_helper.SingleQuote(contents),
-                                 cmd_helper.SingleQuote(device_path))
-      self.RunShellCommand(cmd, as_root=as_root, check_return=True)
+    if not force_push and len(contents) < self._MAX_ADB_COMMAND_LENGTH:
+      cmd = self._PrepareShellCommand(
+          'echo -n %s > %s' % (cmd_helper.SingleQuote(contents),
+                               cmd_helper.SingleQuote(device_path)),
+          as_root=as_root)

[perezju, 2014/11/28 15:39:34]

^^ this is where I need to prepare the command before sending to run shell
(because I want to avoid the case when the contents are < 512, but the resulting
script is >= 512).

Now I've thought that, in fact, PrepareShellCommand is quite general, and might
be useful to prepare commands to run on other shells (e.g. the host, or on
command line files). Not sure how you would feel about moving it to cmd_helper
(and changing the "as_root" option for "with_su")?

Also to add to my wishlist (but definitely not for this CL) how would you feel
about this feature: if one of the arguments is a (key, val) pair, encode it as a
"key=val" argument (with appropriate quoting as needed). Right now there is a
huge mess with clients trying to create such commands (e.g. [1]), on things that
don't end up necessarily being run by the adb shell.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

[jbudorick, 2014/11/28 18:49:01]

If you "prepare" a shell command twice -- as you will in this flow -- is the
result adversely affected, or just extremely quote-escaped?
This would seem to make it easier to forget. If we want to generalize this, we
should rework how we're handling shell commands broadly. Perhaps they shouldn't
be handled as strings until absolutely necessary, but rather as some object that
holds the command, arguments, pwd, env vars, etc?

[perezju, 2014/11/28 19:43:37]

Nothing happens. Because the second "prepare" gets a string (already prepared
and with no extra options added) so the literal string makes its way unchanged
to adb.Shell. (And that's actually the point, we want to pass something to
RunShellCommand which: (1) we know it's size and (2) it's size won't change). I
could add a test that makes this explicit.
That's actually an extremely good idea. We should keep that in our minds, but
probably to do only after all the adb migration has finished.

+      if len(cmd) < self._MAX_ADB_COMMAND_LENGTH:
+        self.RunShellCommand(cmd, check_return=True)
+        return
+    if as_root and self.NeedsSU():
+      with device_temp_file.DeviceTempFile(self.adb) as device_temp:
+        self.PushContents(contents, device_temp.name)
+        # Here we need 'cp' rather than 'mv' because the temp and
+        # destination files might be on different file systems (e.g.
+        # on internal storage and an external sd card)
+        self.RunShellCommand(['cp', device_temp.name, device_path],
+                             as_root=True, check_return=True)
     else:
-      with tempfile.NamedTemporaryFile() as host_temp:
-        host_temp.write(contents)
-        host_temp.flush()
-        if as_root and self.NeedsSU():
-          with device_temp_file.DeviceTempFile(self) as device_temp:
-            self.adb.Push(host_temp.name, device_temp.name)
-            # Here we need 'cp' rather than 'mv' because the temp and
-            # destination files might be on different file systems (e.g.
-            # on internal storage and an external sd card)
-            self.RunShellCommand(['cp', device_temp.name, device_path],
-                                 as_root=True, check_return=True)
-        else:
-          self.adb.Push(host_temp.name, device_path)
+      self.PushContents(contents, device_path)
 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def Ls(self, device_path, timeout=None, retries=None):
     """Lists the contents of a directory on the device.
 
     Args:
       device_path: A string containing the path of the directory on the device
                    to list.
       timeout: timeout in seconds
       retries: number of retries
@@ skipping 220 matching lines @@
     Returns:
       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])