Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1397663002:
DeviceUtils.StartInstrumentation: Shrink command-line (Closed)
|
Created:
5 years, 2 months ago by agrieve Modified:
5 years, 2 months ago Reviewers:
jbudorick CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDeviceUtils.StartInstrumentation: Shrink command-line
Uses a shell variable to make the command-line shorter than
MAX_ADB_COMMAND_LENGTH (runs faster)
BUG=540857
Committed: https://crrev.com/c62c20dc43491940e78b4bcee93dde142c5139a8
Cr-Commit-Position: refs/heads/master@{#354082}
Patch Set 1 #
Total comments: 4
Patch Set 2 : make helper func #
Total comments: 5
Patch Set 3 : add blank line #
Depends on Patchset: Messages
Total messages: 23 (5 generated)
agrieve@chromium.org changed reviewers: + jbudorick@chromium.org
On 2015/10/08 01:09:35, agrieve wrote: > mailto:agrieve@chromium.org changed reviewers: > + mailto:jbudorick@chromium.org
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1397663002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1397663002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1397663002/diff/1/build/android/devil/android... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1397663002/diff/1/build/android/devil/android... build/android/devil/android/device_utils.py:934: def shrink(value): 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?
https://codereview.chromium.org/1397663002/diff/1/build/android/devil/android... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1397663002/diff/1/build/android/devil/android... build/android/devil/android/device_utils.py:934: def shrink(value): On 2015/10/08 14:27:28, jbudorick wrote: > 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? 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})
https://codereview.chromium.org/1397663002/diff/1/build/android/devil/android... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1397663002/diff/1/build/android/devil/android... build/android/devil/android/device_utils.py:934: def shrink(value): On 2015/10/08 15:42:10, agrieve wrote: > On 2015/10/08 14:27:28, jbudorick wrote: > > 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? > > 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. Yeah, I didn't think env would work here. > > 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). I don't mean callers of StartInstrumentation. I mean callers of RunShellCommand. > > 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])? Yeah, something like that. > > 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}) > > 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.
https://codereview.chromium.org/1397663002/diff/1/build/android/devil/android... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1397663002/diff/1/build/android/devil/android... build/android/devil/android/device_utils.py:934: def shrink(value): On 2015/10/08 15:51:09, jbudorick wrote: > On 2015/10/08 15:42:10, agrieve wrote: > > On 2015/10/08 14:27:28, jbudorick wrote: > > > 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? > > > > 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. > > Yeah, I didn't think env would work here. > > > > > 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). > > I don't mean callers of StartInstrumentation. I mean callers of RunShellCommand. > > > > > 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])? > > Yeah, something like that. > > > > > 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}) > > > > > > 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. 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.
https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/and... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/and... build/android/devil/android/device_utils.py:944: cmd_helper.ShrinkToSnippet(cmd, 'p', package)) Hrm, I was thinking something more along the lines of shell_snippet = cmd_helper.ShrinkToSnippet(cmd, [package]) where the second parameter is a list of strings to shrink and then have ShrinkToSnippet handle the shell variable part. (or just do ShrinkToSnippet(cmd) and have it figure out what to shrink, but giving it hints seems a bit more straightforward)
https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/and... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/and... build/android/devil/android/device_utils.py:944: cmd_helper.ShrinkToSnippet(cmd, 'p', package)) On 2015/10/08 20:55:39, jbudorick wrote: > Hrm, I was thinking something more along the lines of > > shell_snippet = cmd_helper.ShrinkToSnippet(cmd, [package]) > > where the second parameter is a list of strings to shrink and then have > ShrinkToSnippet handle the shell variable part. (or just do ShrinkToSnippet(cmd) > and have it figure out what to shrink, but giving it hints seems a bit more > straightforward) The helper is quite limiting already since it can only act on a single cmd list. If it inserts the variable as well, then you couldn't efficiently combine two commands at once that use the same substitutions. e.g.: cmd1 =['foo', 'somelongarg1', 'somelongarg2'] cmd2 =['foo', 'somelongarg3', 'somelongarg4'] snippet1 = ShrinkToSnippet(cmd1, 'a', 'somelongarg') snippet2 = ShrinkToSnippet(cmd2, 'a', 'somelongarg') cmd = 'a=somelongarg;%s;%s' % (snippet1, snippet2) I think it'd be best to just leave it as is and wait to see if it can be used elsewhere before finalizing its API. E.g. it can't be used in https://codereview.chromium.org/1398613002/, although I don't see how we could come up with a helper that would.
On 2015/10/09 00:08:30, agrieve wrote: > https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/and... > File build/android/devil/android/device_utils.py (right): > > https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/and... > build/android/devil/android/device_utils.py:944: cmd_helper.ShrinkToSnippet(cmd, > 'p', package)) > On 2015/10/08 20:55:39, jbudorick wrote: > > Hrm, I was thinking something more along the lines of > > > > shell_snippet = cmd_helper.ShrinkToSnippet(cmd, [package]) > > > > where the second parameter is a list of strings to shrink and then have > > ShrinkToSnippet handle the shell variable part. (or just do > ShrinkToSnippet(cmd) > > and have it figure out what to shrink, but giving it hints seems a bit more > > straightforward) > > The helper is quite limiting already since it can only act on a single cmd list. > If it inserts the variable as well, then you couldn't efficiently combine two > commands at once that use the same substitutions. > e.g.: > cmd1 =['foo', 'somelongarg1', 'somelongarg2'] > cmd2 =['foo', 'somelongarg3', 'somelongarg4'] > > snippet1 = ShrinkToSnippet(cmd1, 'a', 'somelongarg') > snippet2 = ShrinkToSnippet(cmd2, 'a', 'somelongarg') > cmd = 'a=somelongarg;%s;%s' % (snippet1, snippet2) > > I think it'd be best to just leave it as is and wait to see if it can be used > elsewhere before finalizing its API. E.g. it can't be used in > https://codereview.chromium.org/1398613002/, although I don't see how we could > come up with a helper that would. Another thing I thought of: cmd = "a=/path.zip;" + ShinkToSnippet(...) + ";unzip $a" If ShrinkToSnippet re-defined a, then this would break. another example: cmd = "cd foo && " + ShrinkToSnippet(...) This wouldn't do what you'd think it should if ShrinkToSnippet comprised two statements. You could add in more ()s, but those are starting to get ugly and shouldn't be necessary.
On 2015/10/09 18:11:43, agrieve wrote: > On 2015/10/09 00:08:30, agrieve wrote: > > > https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/and... > > File build/android/devil/android/device_utils.py (right): > > > > > https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/and... > > build/android/devil/android/device_utils.py:944: > cmd_helper.ShrinkToSnippet(cmd, > > 'p', package)) > > On 2015/10/08 20:55:39, jbudorick wrote: > > > Hrm, I was thinking something more along the lines of > > > > > > shell_snippet = cmd_helper.ShrinkToSnippet(cmd, [package]) > > > > > > where the second parameter is a list of strings to shrink and then have > > > ShrinkToSnippet handle the shell variable part. (or just do > > ShrinkToSnippet(cmd) > > > and have it figure out what to shrink, but giving it hints seems a bit more > > > straightforward) > > > > The helper is quite limiting already since it can only act on a single cmd > list. > > If it inserts the variable as well, then you couldn't efficiently combine two > > commands at once that use the same substitutions. > > e.g.: > > cmd1 =['foo', 'somelongarg1', 'somelongarg2'] > > cmd2 =['foo', 'somelongarg3', 'somelongarg4'] > > > > snippet1 = ShrinkToSnippet(cmd1, 'a', 'somelongarg') > > snippet2 = ShrinkToSnippet(cmd2, 'a', 'somelongarg') > > cmd = 'a=somelongarg;%s;%s' % (snippet1, snippet2) > > > > I think it'd be best to just leave it as is and wait to see if it can be used > > elsewhere before finalizing its API. E.g. it can't be used in > > https://codereview.chromium.org/1398613002/, although I don't see how we could > > come up with a helper that would. > > Another thing I thought of: > > cmd = "a=/path.zip;" + ShinkToSnippet(...) + ";unzip $a" > > If ShrinkToSnippet re-defined a, then this would break. > > another example: > cmd = "cd foo && " + ShrinkToSnippet(...) > > This wouldn't do what you'd think it should if ShrinkToSnippet comprised two > statements. You could add in more ()s, but those are starting to get ugly and > shouldn't be necessary. ping
On 2015/10/13 15:21:41, agrieve wrote: > On 2015/10/09 18:11:43, agrieve wrote: > > On 2015/10/09 00:08:30, agrieve wrote: > > > > > > https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/and... > > > File build/android/devil/android/device_utils.py (right): > > > > > > > > > https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/and... > > > build/android/devil/android/device_utils.py:944: > > cmd_helper.ShrinkToSnippet(cmd, > > > 'p', package)) > > > On 2015/10/08 20:55:39, jbudorick wrote: > > > > Hrm, I was thinking something more along the lines of > > > > > > > > shell_snippet = cmd_helper.ShrinkToSnippet(cmd, [package]) > > > > > > > > where the second parameter is a list of strings to shrink and then have > > > > ShrinkToSnippet handle the shell variable part. (or just do > > > ShrinkToSnippet(cmd) > > > > and have it figure out what to shrink, but giving it hints seems a bit > more > > > > straightforward) > > > > > > The helper is quite limiting already since it can only act on a single cmd > > list. > > > If it inserts the variable as well, then you couldn't efficiently combine > two > > > commands at once that use the same substitutions. > > > e.g.: > > > cmd1 =['foo', 'somelongarg1', 'somelongarg2'] > > > cmd2 =['foo', 'somelongarg3', 'somelongarg4'] > > > > > > snippet1 = ShrinkToSnippet(cmd1, 'a', 'somelongarg') > > > snippet2 = ShrinkToSnippet(cmd2, 'a', 'somelongarg') > > > cmd = 'a=somelongarg;%s;%s' % (snippet1, snippet2) > > > > > > I think it'd be best to just leave it as is and wait to see if it can be > used > > > elsewhere before finalizing its API. E.g. it can't be used in > > > https://codereview.chromium.org/1398613002/, although I don't see how we > could > > > come up with a helper that would. > > > > Another thing I thought of: > > > > cmd = "a=/path.zip;" + ShinkToSnippet(...) + ";unzip $a" > > > > If ShrinkToSnippet re-defined a, then this would break. > > > > another example: > > cmd = "cd foo && " + ShrinkToSnippet(...) > > > > This wouldn't do what you'd think it should if ShrinkToSnippet comprised two > > statements. You could add in more ()s, but those are starting to get ugly and > > shouldn't be necessary. > > ping pong
On 2015/10/14 14:18:24, agrieve wrote: > On 2015/10/13 15:21:41, agrieve wrote: > > On 2015/10/09 18:11:43, agrieve wrote: > > > On 2015/10/09 00:08:30, agrieve wrote: > > > > > > > > > > https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/and... > > > > File build/android/devil/android/device_utils.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/and... > > > > build/android/devil/android/device_utils.py:944: > > > cmd_helper.ShrinkToSnippet(cmd, > > > > 'p', package)) > > > > On 2015/10/08 20:55:39, jbudorick wrote: > > > > > Hrm, I was thinking something more along the lines of > > > > > > > > > > shell_snippet = cmd_helper.ShrinkToSnippet(cmd, [package]) > > > > > > > > > > where the second parameter is a list of strings to shrink and then have > > > > > ShrinkToSnippet handle the shell variable part. (or just do > > > > ShrinkToSnippet(cmd) > > > > > and have it figure out what to shrink, but giving it hints seems a bit > > more > > > > > straightforward) > > > > > > > > The helper is quite limiting already since it can only act on a single cmd > > > list. > > > > If it inserts the variable as well, then you couldn't efficiently combine > > two > > > > commands at once that use the same substitutions. > > > > e.g.: > > > > cmd1 =['foo', 'somelongarg1', 'somelongarg2'] > > > > cmd2 =['foo', 'somelongarg3', 'somelongarg4'] > > > > > > > > snippet1 = ShrinkToSnippet(cmd1, 'a', 'somelongarg') > > > > snippet2 = ShrinkToSnippet(cmd2, 'a', 'somelongarg') > > > > cmd = 'a=somelongarg;%s;%s' % (snippet1, snippet2) > > > > > > > > I think it'd be best to just leave it as is and wait to see if it can be > > used > > > > elsewhere before finalizing its API. E.g. it can't be used in > > > > https://codereview.chromium.org/1398613002/, although I don't see how we > > could > > > > come up with a helper that would. > > > > > > Another thing I thought of: > > > > > > cmd = "a=/path.zip;" + ShinkToSnippet(...) + ";unzip $a" > > > > > > If ShrinkToSnippet re-defined a, then this would break. > > > > > > another example: > > > cmd = "cd foo && " + ShrinkToSnippet(...) > > > > > > This wouldn't do what you'd think it should if ShrinkToSnippet comprised two > > > statements. You could add in more ()s, but those are starting to get ugly > and > > > shouldn't be necessary. > > > > ping > > pong sorry, had higher priority stuff yesterday... going to try to look today.
lgtm w/ nit https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/and... File build/android/devil/android/device_utils.py (right): https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/and... build/android/devil/android/device_utils.py:944: cmd_helper.ShrinkToSnippet(cmd, 'p', package)) On 2015/10/09 00:08:29, agrieve wrote: > On 2015/10/08 20:55:39, jbudorick wrote: > > Hrm, I was thinking something more along the lines of > > > > shell_snippet = cmd_helper.ShrinkToSnippet(cmd, [package]) > > > > where the second parameter is a list of strings to shrink and then have > > ShrinkToSnippet handle the shell variable part. (or just do > ShrinkToSnippet(cmd) > > and have it figure out what to shrink, but giving it hints seems a bit more > > straightforward) > > The helper is quite limiting already since it can only act on a single cmd list. > If it inserts the variable as well, then you couldn't efficiently combine two > commands at once that use the same substitutions. > e.g.: > cmd1 =['foo', 'somelongarg1', 'somelongarg2'] > cmd2 =['foo', 'somelongarg3', 'somelongarg4'] > > snippet1 = ShrinkToSnippet(cmd1, 'a', 'somelongarg') > snippet2 = ShrinkToSnippet(cmd2, 'a', 'somelongarg') > cmd = 'a=somelongarg;%s;%s' % (snippet1, snippet2) > > I think it'd be best to just leave it as is and wait to see if it can be used > elsewhere before finalizing its API. E.g. it can't be used in > https://codereview.chromium.org/1398613002/, although I don't see how we could > come up with a helper that would. > Fair enough; I'm ok with landing this as-is and iterating on the interface. https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/uti... File build/android/devil/utils/cmd_helper.py (right): https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/uti... build/android/devil/utils/cmd_helper.py:87: return ' '.join(shrink(part) for part in cmd_parts) nit: newline before this line
https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/uti... File build/android/devil/utils/cmd_helper.py (right): https://codereview.chromium.org/1397663002/diff/20001/build/android/devil/uti... build/android/devil/utils/cmd_helper.py:87: return ' '.join(shrink(part) for part in cmd_parts) On 2015/10/14 17:59:09, jbudorick wrote: > nit: newline before this line Done.
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1397663002/#ps40001 (title: "add blank line")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1397663002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1397663002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c62c20dc43491940e78b4bcee93dde142c5139a8 Cr-Commit-Position: refs/heads/master@{#354082} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
