Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
751063002:
Allow RunShellCommand to work even with very large commands (Closed)
DescriptionAllow RunShellCommand to work even with very large commands
Currently RunShellCommand may not work if the command passed to
adb.Shell is too large (e.g. > 1024). This CL addresses this
issue, allowing to run even very large commands, by first writing
the command as a shell script and then running it.
BUG=436133
Committed: https://crrev.com/b35110e91edb38ec81c84e188b35012492bbcc28
Cr-Commit-Position: refs/heads/master@{#307223}
Patch Set 1 #Patch Set 2 : less mind-bendy version #
Total comments: 8
Patch Set 3 : no minds bent #
Total comments: 2
Patch Set 4 : clients of DeviceTempFile should pass adb #
Total comments: 15
Patch Set 5 : fixes w.r.t. latest comments #
Total comments: 1
Patch Set 6 : comment implementation of WriteFile #
Messages
Total messages: 25 (5 generated)
perezju@chromium.org changed reviewers: + jbudorick@chromium.org
The code is actually quite simple, and works beautifully, but I know it
introduces some very mind-bending mutual-recursive dependencies, but bear with
me for a second. It will always terminate (and do pretty much what you expect it
to do), under one simple assumption: the names provided by TempDeviceFile should
be short (< 256 chars) and contain no funky characters.
You also don't have to trust me, this is the proof (which took me much longer to
write than the code!):
Reasonable assumption:
1. names generated by DeviceTempFile have len(device_temp.name) < 256, and no
characters that may require quoting (e.g. spaces or quotes).
We also assume that the following terminate:
2. adb.Shell(...)
3. adb.Push(..., ...)
These terminate because len(actual_cmd) < 512, and resolve to adb.Shell(...)
4. RunShellCommand('echo $EXTERNAL_STORAGE') [from 2]
5. RunShellCommand('test -e {device_temp.name}') [from 1, 2]
6. RunShellCommand("echo -n '' > {device_temp.name}", as_root=?) [from 1, 2]
These terminate when contents='':
7. WriteFile(device_temp.name, contents='', as_root=False):
- RunShellCommand('echo '' > {device_temp.name}', as_root=False) [from 6]
8. DeviceTempFile(contents=''):
- RunShellCommand('echo $EXTERNAL_STORAGE') [from 4]
- RunShellCommand('test -e {device_temp.name}') [from 5]
- WriteFile(device_temp.name, '', as_root=False) [from 7]
These terminate when len(contents) >= 512:
9. WriteFile(device_path, contents, as_root=False):
- adb.Push(tmp_host.name, device_path) [from 3]
10. DeviceTempFile(contents, ...):
- RunShellCommand('echo $EXTERNAL_STORAGE') [from 4]
- RunShellCommand('test -e {device_temp.name}') [from 5]
- WriteFile(device_temp.name, '', as_root=False) [from 8]
- WriteFile(devide_temp.name, contents, as_root=False) [from 9]
These always terminate in all cases:
11. RunShellCommand(cmd, ...):
- if len(cmd) < 512:
- adb.Shell(cmd) [from 2]
- else: # len(cmd) >= 512
- DeviceTempFile(contents=cmd, ...) [from 10]
- adb.Shell('sh {device_temp.name}') [from 2]
12. WriteFile(device_path, contents, ...):
- if len(contents) < 512 and not force_push:
- RunShellCommand(...) [from 11]
- elif as_root and self.Needs.SU():
- DeviceTempFile(contents='') [from 8]
- adb.Push(...) [from 3]
- RunShellCommand('cp ...') [from 11]
- else:
- adb.Push(...) [from 3]
13. DeviceTempFile(contents):
- RunShellCommand('echo $EXTERNAL_STORAGE') [from 4]
- RunShellCommand('test -e {device_temp.name}') [from 5]
- if len(contents) > 256:
- WriteFile(device_temp.name, '', as_root=False) [from 7]
- WriteFile(devide_temp.name, contents, as_root=False) [from 12]
Agh, as much as I love the fact that this actually works with such little changes, on my way back home I convinced myself that this simply won't be maintainable. Of course :P. Any changes in the future to any of the functions involved would have to be done with much care as to not introduce any infinite loops. So, the solution probably involves breaking these dependencies (including the one already there between TempDeviceFile and WriteFile), but I'm still not sure exactly where. One alternative I though of is try to keep TempDeviceFile as simple and "low level" as possible: it doesn't need much of what DeviceUtils has to offer, so maybe could do with a simple AdbWrapper making direct calls to adb.Shell (to test, echo and rm, all require quite simple versions with short commands and without root or any other magic). I don't like much, however, the fact that this would probably involve hardcoding of GetExternalStoragePath (i.e. the echo $EXTERNAL_STORAGE) in TempDeviceFile. The other alternative, mixing calls to DeviceUtils and some AdbWrapper, seems a bit wrong. Anyway, I'm still jet-lagged so probably shouldn't have coded this much today :P Let me know what you think about the alternatives.
From looking at this code, it looks like if the code.. (1) if the command is less that 512 characters you run it as normal. (2) else if the command is more than 512 character you push the command to the device inside of a file, then run it with "sh <filename>" Is that what is going on?
On 2014/11/24 20:59:19, mikecase wrote: > From looking at this code, it looks like if the code.. > > (1) if the command is less that 512 characters you run it as normal. > (2) else if the command is more than 512 character you push the command to the > device inside of a file, then run it with "sh <filename>" > > Is that what is going on? Yep, that is the idea. And I needed a temp file on the device where to push the command.
On 2014/11/24 20:57:20, perezju wrote: > Agh, as much as I love the fact that this actually works with such little > changes, on my way back home I convinced myself that this simply won't be > maintainable. Of course :P. Any changes in the future to any of the functions > involved would have to be done with much care as to not introduce any infinite > loops. > > So, the solution probably involves breaking these dependencies (including the > one already there between TempDeviceFile and WriteFile), but I'm still not sure > exactly where. > > One alternative I though of is try to keep TempDeviceFile as simple and "low > level" as possible: it doesn't need much of what DeviceUtils has to offer, so > maybe could do with a simple AdbWrapper making direct calls to adb.Shell (to > test, echo and rm, all require quite simple versions with short commands and > without root or any other magic). I don't like much, however, the fact that this > would probably involve hardcoding of GetExternalStoragePath (i.e. the echo > $EXTERNAL_STORAGE) in TempDeviceFile. The other alternative, mixing calls to > DeviceUtils and some AdbWrapper, seems a bit wrong. Yeah, this was nasty when we did it and it's still nasty now. In general, I don't have a problem with something mixing calls between DeviceUtils and AdbWrapper. This is not an example of the general case, though, since DeviceUtils implements some of its functions using this. As such, I agree with breaking the dependency on DeviceUtils, even if it does mean duplicating GetExternalStoragePath. I don't think we need to add contents, especially if we try to remove DeviceUtils. > > Anyway, I'm still jet-lagged so probably shouldn't have coded this much today :P > Let me know what you think about the alternatives.
Please have a look. I removed all cyclic dependencies except for a very small one between WriteFile and RunShellCommand but, as you will see, the argument for why that is OK is much simpler. https://codereview.chromium.org/751063002/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/751063002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:458: self.WriteFile(script.name, cmd, as_root=False, force_push=True) look, our first non-artificial use of force_push! :) https://codereview.chromium.org/751063002/diff/20001/build/android/pylib/util... File build/android/pylib/utils/device_temp_file.py (right): https://codereview.chromium.org/751063002/diff/20001/build/android/pylib/util... build/android/pylib/utils/device_temp_file.py:31: and isinstance(device.adb, adb_wrapper.AdbWrapper)): I think this should be quite safe, and allows us to pass both AdbWrapper or DeviceUtils objects. I couldn't easily test for isinstnance(.., device_utils.DeviceUtils) because importing device_utils causes a cyclic module dependency. (which, as we learned yesterday, is precisely what we don't want to do!) https://codereview.chromium.org/751063002/diff/20001/build/android/pylib/util... build/android/pylib/utils/device_temp_file.py:43: self.name_quoted = cmd_helper.SingleQuote(self.name) we use this quite often, so thought it was a better idea to have it cached. https://codereview.chromium.org/751063002/diff/20001/build/android/pylib/util... File build/android/pylib/utils/device_temp_file_test.py (right): https://codereview.chromium.org/751063002/diff/20001/build/android/pylib/util... build/android/pylib/utils/device_temp_file_test.py:52: return (self.call.adb.Shell(mock.ANY), check_and_return) John, have a look. I'm not quite sure if this is the same thing you wanted to do the other day (checking for a subset of arguments in assertCalls), but this is how I managed to do something similar in this case.
https://codereview.chromium.org/751063002/diff/20001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/751063002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:452: if len(cmd) < 512: We should de-magic 512. "MAX_ADB_COMMAND_LENGTH" or something like that. https://codereview.chromium.org/751063002/diff/20001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:457: # otherwise WriteFile may introduce infinite recursion This worries me. It's the same kind of cyclical dependency we had in the previous version of device_temp_file -- one that is correctly avoided _now_ and well-commented, but is tricky to modify and liable to be sneakily broken by changes elsewhere (in this case, in WriteFile). Maybe would should instead be doing this with tempfile + device_temp_file + adb.Push? https://codereview.chromium.org/751063002/diff/20001/build/android/pylib/util... File build/android/pylib/utils/device_temp_file.py (right): https://codereview.chromium.org/751063002/diff/20001/build/android/pylib/util... build/android/pylib/utils/device_temp_file.py:31: and isinstance(device.adb, adb_wrapper.AdbWrapper)): On 2014/11/25 16:25:33, perezju wrote: > I think this should be quite safe, and allows us to pass both AdbWrapper or > DeviceUtils objects. > > I couldn't easily test for isinstnance(.., device_utils.DeviceUtils) because > importing device_utils causes a cyclic module dependency. (which, as we learned > yesterday, is precisely what we don't want to do!) Does it make sense to pass DeviceUtils at all? DeviceUtils instances have a public 'adb' attribute (obviously), so perhaps we should just make the clients pass in the AdbWrapper? https://codereview.chromium.org/751063002/diff/20001/build/android/pylib/util... File build/android/pylib/utils/device_temp_file_test.py (right): https://codereview.chromium.org/751063002/diff/20001/build/android/pylib/util... build/android/pylib/utils/device_temp_file_test.py:52: return (self.call.adb.Shell(mock.ANY), check_and_return) On 2014/11/25 16:25:33, perezju wrote: > John, have a look. I'm not quite sure if this is the same thing you wanted to do > the other day (checking for a subset of arguments in assertCalls), but this is > how I managed to do something similar in this case. Not quite -- I think I was interested in something that wasn't limited to prefixes -- but I'm fine with this.
https://codereview.chromium.org/751063002/diff/40001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/751063002/diff/40001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:900: def PushContents(self, contents, device_path, timeout=None, retries=None): Turns out we need to push contents in quite a few places. So this functions comes out handy and also simplifies the logic in some of the test cases. https://codereview.chromium.org/751063002/diff/40001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:952: return There was a small chance that the contents are small enough, but when turning them into a script, their length would go over the threshold (specially in the odd case when contents are full of, e.g., quotes), causing it to *push* a script to then *echo* within the device. This bit of code, even if not quite neat, takes care of that.
https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:373: def _PrepareShellCommand(self, cmd, cwd=None, env=None, as_root=False): Why is this separate from RunShellCommand? Would we ever call it if we weren't subsequently calling RunShellCommand? https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:900: def PushContents(self, contents, device_path, timeout=None, retries=None): 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. https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/util... File build/android/pylib/utils/device_temp_file.py (right): https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/util... build/android/pylib/utils/device_temp_file.py:32: self.name = '{path}/{prefix}-{time:d}-{nonce:d}{suffix}'.format( Unrelated meddling again, I see. :) https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/util... build/android/pylib/utils/device_temp_file.py:42: self._adb.Shell('touch %s' % self.name_quoted) Does touching have the same effect?
just replying to the comments https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:373: def _PrepareShellCommand(self, cmd, cwd=None, env=None, as_root=False): On 2014/11/27 17:17:34, jbudorick wrote: > Why is this separate from RunShellCommand? Would we ever call it if we weren't > subsequently calling RunShellCommand? see below vv https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:900: def PushContents(self, contents, device_path, timeout=None, retries=None): On 2014/11/27 17:17:34, jbudorick wrote: > 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. 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. https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:949: as_root=as_root) ^^ 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... https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/util... File build/android/pylib/utils/device_temp_file.py (right): https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/util... build/android/pylib/utils/device_temp_file.py:32: self.name = '{path}/{prefix}-{time:d}-{nonce:d}{suffix}'.format( On 2014/11/27 17:17:34, jbudorick wrote: > Unrelated meddling again, I see. :) ahhh, but it looks nicer, no? :) https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/util... build/android/pylib/utils/device_temp_file.py:42: self._adb.Shell('touch %s' % self.name_quoted) On 2014/11/27 17:17:34, jbudorick wrote: > Does touching have the same effect? yep, creates a file of 0 bytes if it didn't exist already. I also tested it on a device.
This CL is continuing a worrying trend of adding more and more complexity to the implementation of DeviceUtils for seemingly minimal gain. We should talk about this more next week. https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:900: def PushContents(self, contents, device_path, timeout=None, retries=None): On 2014/11/28 15:39:34, perezju wrote: > On 2014/11/27 17:17:34, jbudorick wrote: > > 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. > > 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. > The three should probably be consistent. Perhaps we should change WriteFile's order. > Maybe I'll make it private, call it "_WriteFileWithPush", and keep the argument > order the same as WriteFile. https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:949: as_root=as_root) On 2014/11/28 15:39:34, perezju wrote: > ^^ 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). If you "prepare" a shell command twice -- as you will in this flow -- is the result adversely affected, or just extremely quote-escaped? > > 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")? 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? > > 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... https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/util... File build/android/pylib/utils/device_temp_file.py (right): https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/util... build/android/pylib/utils/device_temp_file.py:32: self.name = '{path}/{prefix}-{time:d}-{nonce:d}{suffix}'.format( On 2014/11/28 15:39:34, perezju wrote: > On 2014/11/27 17:17:34, jbudorick wrote: > > Unrelated meddling again, I see. :) > > ahhh, but it looks nicer, no? :) Yes, but I would prefer if we kept unrelated meddling to a minimum. https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/util... build/android/pylib/utils/device_temp_file.py:42: self._adb.Shell('touch %s' % self.name_quoted) On 2014/11/28 15:39:34, perezju wrote: > On 2014/11/27 17:17:34, jbudorick wrote: > > Does touching have the same effect? > > yep, creates a file of 0 bytes if it didn't exist already. I also tested it on a > device. sgtm
I'll prepare a new CL on monday. Just quickly replying now to some of the comments. https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:900: def PushContents(self, contents, device_path, timeout=None, retries=None): On 2014/11/28 18:49:01, jbudorick wrote: > On 2014/11/28 15:39:34, perezju wrote: > > On 2014/11/27 17:17:34, jbudorick wrote: > > > 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. > > > > 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. > > > > The three should probably be consistent. Perhaps we should change WriteFile's > order. > > > Maybe I'll make it private, call it "_WriteFileWithPush", and keep the > argument > > order the same as WriteFile. > 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. https://codereview.chromium.org/751063002/diff/60001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:949: as_root=as_root) On 2014/11/28 18:49:01, jbudorick wrote: > On 2014/11/28 15:39:34, perezju wrote: > > ^^ 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). > > If you "prepare" a shell command twice -- as you will in this flow -- is the > result adversely affected, or just extremely quote-escaped? 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. > > > > > 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")? > > 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? 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.
John, ping me when you're online so we can chat a bit more about this. On 2014/11/28 19:43:37, perezju wrote: > 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. I had a quick look browsing through the code. I think that indeed it is far more common in our code base to have "write"-like functions with the argument order "Write...(where, what)", for example: https://code.google.com/p/chromium/codesearch#chromium/src/tools/grit/grit/to... https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/ch... https://code.google.com/p/chromium/codesearch#chromium/build/third_party/cbui... https://code.google.com/p/chromium/codesearch#chromium/src/tools/grit/grit/to... I think I've also seem this convention in many other places as well. However, I did found one instance where the order is the other way around, e.g. more like "Write...(source, destination)" https://code.google.com/p/chromium/codesearch#chromium/src/v8/tools/js2c.py&q... Also python "dump" functions seem to follow the (data/source, destination) order, e.g. in pickle.dump, and json.dump. Other mv-, cp-, and push-like functions naturally tend to follow this order. So not sure what to prefer in the end. Any thoughts?
On 2014/12/02 11:39:06, perezju wrote: > John, ping me when you're online so we can chat a bit more about this. > > On 2014/11/28 19:43:37, perezju wrote: > > 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. > > I had a quick look browsing through the code. I think that indeed it is far more > common in our code base to have "write"-like functions with the argument order > "Write...(where, what)", for example: > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/grit/grit/to... > https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/ch... > https://code.google.com/p/chromium/codesearch#chromium/build/third_party/cbui... > https://code.google.com/p/chromium/codesearch#chromium/src/tools/grit/grit/to... > > I think I've also seem this convention in many other places as well. > > However, I did found one instance where the order is the other way around, e.g. > more like "Write...(source, destination)" > > https://code.google.com/p/chromium/codesearch#chromium/src/v8/tools/js2c.py&q... > > Also python "dump" functions seem to follow the (data/source, destination) > order, > e.g. in pickle.dump, and json.dump. Other mv-, cp-, and push-like functions > naturally tend to follow this order. Neither of these are perfect analogues -- dump functions are doing some amount of encoding, while the mv, cp, etc work on solely file paths. That said, Write...(src, dest) would seem to me to be the more intuitive of the two options. As usual, though, it's a not-in-this-CL problem, and it should generally be considered very low priority. In the meantime, I think that PushContents or _PushContents or whatever should match first what's in the file -- i.e., have the same order as WriteFile. > > So not sure what to prefer in the end. Any thoughts?
So, latest changes: - _PrepareShellCommand is no longer split from _RunShellCommand - _WriteFileWithPush(device_path, contents) replaces the public PushContents
lgtm w/ nit https://codereview.chromium.org/751063002/diff/80001/build/android/pylib/devi... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/751063002/diff/80001/build/android/pylib/devi... build/android/pylib/device/device_utils.py:927: if not force_push and len(contents) < self._MAX_ADB_COMMAND_LENGTH: nit: Can we comment the reason for the 3-way split implementation here?
The CQ bit was checked by perezju@chromium.org
The CQ bit was unchecked by perezju@chromium.org
The CQ bit was checked by perezju@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/751063002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b35110e91edb38ec81c84e188b35012492bbcc28 Cr-Commit-Position: refs/heads/master@{#307223}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/773043005/ by perezju@chromium.org. The reason for reverting is: Broke provision_devices on android perf bots.
Message was sent while issue was closed.
Patchset #7 (id:120001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
