Reland of 'New run shell implementation for DeviceUtils'

Reland of 'New run shell implementation for DeviceUtils'

This is a reland of https://codereview.chromium.org/659533002/

Previous CL broke telemetry unittests and perf unittests because it
didn't handle properly shell commands which may end with a newline
character when check_rc is not None.

This revised patch strips any whitespace at the end of shell commands
to prevent this.

BUG=267773
Committed: https://crrev.com/c2e31ff56e2486ff4173edb774a2e092eba9ce61
Cr-Commit-Position: refs/heads/master@{#300322}

[perezju, 2014-10-20 15:37:10]

perezju@chromium.org changed reviewers:
+ jbudorick@chromium.org

[perezju, 2014-10-20 15:37:10]

https://codereview.chromium.org/669573003/diff/1/build/android/pylib/device/a...
File build/android/pylib/device/adb_wrapper.py (right):

https://codereview.chromium.org/669573003/diff/1/build/android/pylib/device/a...
build/android/pylib/device/adb_wrapper.py:174: actual_command = '%s; echo $?;' %
command.rstrip()
This is the fix.

[jbudorick, 2014-10-20 15:40:59]

https://codereview.chromium.org/669573003/diff/1/build/android/pylib/device/a...
File build/android/pylib/device/adb_wrapper.py (right):

https://codereview.chromium.org/669573003/diff/1/build/android/pylib/device/a...
build/android/pylib/device/adb_wrapper.py:179: rc =
int(output[output_end:].strip())
I think we should catch ValueError here and raise ... probably an
AdbShellCommandFailedError, since not finding the return code on the last line
indicates that something horribly wrong happened with the shell command.

[perezju, 2014-10-20 16:20:01]

https://codereview.chromium.org/669573003/diff/20001/build/android/pylib/devi...
File build/android/pylib/device/adb_wrapper.py (right):

https://codereview.chromium.org/669573003/diff/20001/build/android/pylib/devi...
build/android/pylib/device/adb_wrapper.py:178: output_end = output.rfind('%')
Trying to find our own marker, rather than a new line, to consider the case when
(for whatever reason) the output from the command did not end with a new line.

[jbudorick, 2014-10-20 16:28:24]

lgtm

[perezju, 2014-10-20 16:45:37]

The CQ bit was checked by perezju@chromium.org

[commit-bot: I haz the power, 2014-10-20 16:46:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/669573003/20001

[jbudorick, 2014-10-20 17:17:03]

The CQ bit was unchecked by jbudorick@chromium.org

[jbudorick, 2014-10-20 17:17:55]

I un-CQ'ed this because it adds a minute onto unit_tests and I want to figure
out why.

[jbudorick, 2014-10-20 18:00:06]

On 2014/10/20 17:17:55, jbudorick wrote:
> I un-CQ'ed this because it adds a minute onto unit_tests and I want to figure
> out why.

not lgtm

We need to resolve some timeout issues that come up with pushing. If you look at
the log for unit_tests on android_dbg_tests_recipe, you'll see a bunch of these:

C  101.223s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb) 
********************************************************************************
C  101.223s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)  Stack dump for
thread 'TimeoutThread-for-TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)'
C  101.223s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb) 
********************************************************************************
C  101.242s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)  File:
"/usr/lib/python2.7/threading.py", line 524, in __bootstrap
C  101.242s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)   
self.__bootstrap_inner()
C  101.242s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)  File:
"/usr/lib/python2.7/threading.py", line 551, in __bootstrap_inner
C  101.242s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)    self.run()
C  101.242s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)  File:
"/b/build/slave/android_dbg_tests_recipe/build/src/build/android/pylib/utils/reraiser_thread.py",
line 76, in run
C  101.242s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)    self._ret =
self._func(*self._args, **self._kwargs)
C  101.242s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)  File:
"/b/build/slave/android_dbg_tests_recipe/build/src/build/android/pylib/utils/timeout_retry.py",
line 37, in RunOnTimeoutThread
C  101.242s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)    ret[0] =
func(*args, **kwargs)
C  101.242s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)  File:
"/b/build/slave/android_dbg_tests_recipe/build/src/build/android/pylib/device/decorators.py",
line 49, in impl
C  101.242s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)    return f(*args,
**kwargs)
C  101.242s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)  File:
"/b/build/slave/android_dbg_tests_recipe/build/src/build/android/pylib/device/adb_wrapper.py",
line 52, in _RunAdbCmd
C  101.243s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)    exit_code,
output = cmd_helper.GetCmdStatusAndOutput(cmd)
C  101.243s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)  File:
"/b/build/slave/android_dbg_tests_recipe/build/src/build/android/pylib/cmd_helper.py",
line 147, in GetCmdStatusAndOutput
C  101.243s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)    stdout, stderr =
pipe.communicate()
C  101.243s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)  File:
"/usr/lib/python2.7/subprocess.py", line 754, in communicate
C  101.243s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)    return
self._communicate(input)
C  101.243s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)  File:
"/usr/lib/python2.7/subprocess.py", line 1312, in _communicate
C  101.243s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)    stdout, stderr =
self._communicate_with_poll(input)
C  101.243s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)  File:
"/usr/lib/python2.7/subprocess.py", line 1366, in _communicate_with_poll
C  101.243s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb)    ready =
poller.poll()
C  101.243s TimeoutThread-for-PushDataDeps(01cb757b610fb9bb) 
********************************************************************************
I  101.244s TimeoutThread-for-TimeoutThread-for-PushDataDeps(01cb757b610fb9bb) 
[host]> adb -s 01cb757b610fb9bb shell 'PATH="$PATH:/data/local/tmp/bin" su -c
unzip /storage/emulated/legacy/tmp.zip; echo %$?;

There are a couple of options here:

1) figure out the decorator issues we talked about last week
2) retain the timeout parameter to _RunShellCommandImpl and pass it into
adb.Shell until we do (1) in a later CL

I think (2) is the way to go in this CL

[perezju, 2014-10-20 18:21:36]

On 2014/10/20 18:00:06, jbudorick wrote:
> There are a couple of options here:
> 
> 1) figure out the decorator issues we talked about last week
> 2) retain the timeout parameter to _RunShellCommandImpl and pass it into
> adb.Shell until we do (1) in a later CL
> 
> I think (2) is the way to go in this CL

Ah! I thought about that too, but for some reason forgot to make the change and
pass the timeout option.

[jbudorick, 2014-10-20 18:23:28]

lgtm

[perezju, 2014-10-20 18:51:21]

The CQ bit was checked by perezju@chromium.org

[commit-bot: I haz the power, 2014-10-20 18:53:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/669573003/40001

[commit-bot: I haz the power, 2014-10-20 20:26:49]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-20 20:27:30]

Patchset 3 (id:??) landed as
https://crrev.com/c2e31ff56e2486ff4173edb774a2e092eba9ce61
Cr-Commit-Position: refs/heads/master@{#300322}