DeviceUtils.StartInstrumentation: Shrink command-line

build/android/devil/android/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=unused-argument
 
@@ skipping 910 matching lines @@
     for line in self.RunShellCommand(cmd, check_return=True):
       if line.startswith('Error:'):
         raise device_errors.CommandFailedError(line, str(self))
 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def StartInstrumentation(self, component, finish=True, raw=False,
                            extras=None, timeout=None, retries=None):
     if extras is None:
       extras = {}
 
-    cmd = ['am', 'instrument']
+    # Store the package name in a shell variable to help the command stay under
+    # the _MAX_ADB_COMMAND_LENGTH limit.
+    package = component.split('/')[0]
+    def shrink(value):

[jbudorick, 2015/10/08 14:27:28]

I don't agree with handling this this way. It leads toward having multiple
callers needing to all individually remember to SingleQuote everything.

I'd instead prefer adding support for setting shell variables in
RunShellCommand. wdyt?

[agrieve, 2015/10/08 15:42:10]

RunShellCommand can already be passed env variables as a separate parameter, but
you can't effectively use them in your command when passing the command as a
list (which I assume is the concern here) because they'll end up single-quoted.

There shouldn't be a need to have callers need to quote things here since
shrink() still takes care of quoting (there's a test for it).

Are you thinking rather that we could add a parameter to RunShellCommand along
the lines of:
RunShellCommand([cmd, as, before], repeated_tokens=[package,
some_other_common_substring])?

It'd be super tough to support repeated_tokens when cmd is passed as a string,
but would be doable when passed as a list. Maybe instead this should refactored
as:
   cmd_helper.ShrinkCommand(cmd_as_list, {'p': package})

[jbudorick, 2015/10/08 15:51:09]

Yeah, I didn't think env would work here.
I don't mean callers of StartInstrumentation. I mean callers of RunShellCommand.
Yeah, something like that.
I would be fine with something like this, too. The main point is that we
shouldn't be doing this kind of handrolling in so many places -- you're adding
it in enough places (and, in this case, in a way where it needs additional
handling typically provided by RunShellCommand) that it should be handled by a
function. I'm not sure that I have a preference between calling that function
from within RunShellCommand and calling it beforehand.

[agrieve, 2015/10/08 20:03:33]

Extracted it into a helper. Wasn't actually able to use the helper from any
other sites though.

At first I started implementing it with a map of var -> value, but that turns
out to be more complicated, so might as well wait and see if it's ever desired.

Also noted that the env= parameter of RunShellCommand() doesn't work for this
case, and the cwd= also assumes a single command. Maybe we should consider
making those errors if a string is passed rather than a []? 

the env doesn't work because it does:
FOO=BAR cmd

if cmd == echo $FOO, you'll get a blank line

but what might be better is:
export FOO=BAR;cmd

Don't want to touch that in this CL though and I think rather than change it, it
may be worth just making it an error to use shell script when also setting env
or cwd.

+      parts = (x and cmd_helper.SingleQuote(x) for x in value.split(package))
+      return '$p'.join(parts)
+
+    cmd = 'p=%s;am instrument' % package
     if finish:
-      cmd.append('-w')
+      cmd += ' -w'
     if raw:
-      cmd.append('-r')
+      cmd += ' -r'
+
     for k, v in extras.iteritems():
-      cmd.extend(['-e', str(k), str(v)])
-    cmd.append(component)
+      cmd += ' -e %s %s' % (shrink(str(k)), shrink(str(v)))
+
+    cmd += ' %s' % shrink(component)
     return self.RunShellCommand(cmd, check_return=True, large_output=True)
 
   @decorators.WithTimeoutAndRetriesFromInstance()
   def BroadcastIntent(self, intent_obj, timeout=None, retries=None):
     """Send a broadcast intent.
 
     Args:
       intent: An Intent to broadcast.
       timeout: timeout in seconds
       retries: number of retries
@@ skipping 1107 matching lines @@
     if ('android.permission.WRITE_EXTERNAL_STORAGE' in permissions
         and 'android.permission.READ_EXTERNAL_STORAGE' not in permissions):
       permissions.append('android.permission.READ_EXTERNAL_STORAGE')
     cmd = ';'.join('pm grant %s %s' %(package, p) for p in permissions)
     if cmd:
       output = self.RunShellCommand(cmd)
       if output:
         logging.warning('Possible problem when granting permissions. Blacklist '
                         'may need to be updated.')
         logging.warning(output)