Allow RunShellCommand to work even with very large commands

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 272e613b6817f915a2cee039caf6ff53de13bea7..53f9e44f3f9397236f88834f8d08d65ff3eab551 100644
--- a/build/android/pylib/device/device_utils.py
+++ b/build/android/pylib/device/device_utils.py
@@ -58,6 +58,7 @@ def RestartServer():
 
 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,
@@ -369,6 +370,25 @@ class DeviceUtils(object):
         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,
@@ -421,32 +441,25 @@ class DeviceUtils(object):
       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:
@@ -884,6 +897,30 @@ class DeviceUtils(object):
       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.
@@ -905,24 +942,24 @@ class DeviceUtils(object):
       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):