DeviceUtils.StartInstrumentation: Shrink command-line

build/android/devil/android/device_utils.py

Index: build/android/devil/android/device_utils.py
diff --git a/build/android/devil/android/device_utils.py b/build/android/devil/android/device_utils.py
index e9d16935ad2fc307b406fb5e8e45b5b7decf4993..87345025e8d3a12aad70ae42d6ee78d5a7f38116 100644
--- a/build/android/devil/android/device_utils.py
+++ b/build/android/devil/android/device_utils.py
@@ -928,14 +928,23 @@ class DeviceUtils(object):
     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()