Migrate DeviceUtils.ReadFile to adb_wrapper

Migrate DeviceUtils.ReadFile to adb_wrapper

The implementation is based on a simple 'cat' run by shell on the
device (with optional as_root).

Interface changes:
- The return value is a string (instead of a list of lines).
- An exception is raised if the file cannot be read (instead of returning an empty list).

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

[perezju, 2014-12-04 14:16:06]

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

[perezju, 2014-12-04 14:16:07]

Regarding the TODO you had on thinking whether to return a list of lines (or
something else). I looked at the current callers of this method (not too many,
see bellow), and I thought that decision we made in RunShellCommand should  also
translate to ReadFile, namely: return lines, but provide a single_line option
for callers who only expect to read a single line.

(I still go back and forth on whether this was the best approach, or just return
a string and let the caller deal with it using splitlines or rstrip
respectively. But, if any, changes would probably better wait until we finish
the migration.)

Callers that want a list of lines:
src/build/android/tombstones.py
src/tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper_unittest.py

Callers that want a single line:
src/build/android/pylib/flag_changer.py
src/build/android/pylib/perf/thermal_throttle.py
src/tools/telemetry/telemetry/core/forwarders/android_forwarder.py
rc/tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py

Callers that want a string (so they '\n'.join the result):
src/build/android/pylib/instrumentation/test_runner.py
src/tools/telemetry/telemetry/core/platform/android_platform_backend.py

https://codereview.chromium.org/775333002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils_test.py (left):

https://codereview.chromium.org/775333002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils_test.py:1174: as_root=True)
I thought we don't need to test this one any more, as it exercises the exact
same paths as file does not exist. (And is already tested in RunShellCommand
anyway)

https://codereview.chromium.org/775333002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils_test.py (right):

https://codereview.chromium.org/775333002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils_test.py:256: ['shell', cmd], output,
exit_code, str(self.device))
Fixed a small bug I've found when testing this change. The output would come up
as "c a t ' ' s o m e / f i l e", because it thought cmd was a list of args.
This is of course only in the test code, the actual live errors are not
affected.

[jbudorick, 2014-12-04 15:41:55]

On 2014/12/04 14:16:07, perezju wrote:
> Regarding the TODO you had on thinking whether to return a list of lines (or
> something else). I looked at the current callers of this method (not too many,
> see bellow), and I thought that decision we made in RunShellCommand should 
also
> translate to ReadFile, namely: return lines, but provide a single_line option
> for callers who only expect to read a single line.

I'm not sure either a list of lines or single_line makes much sense here. I
(now) think that ReadFile should probably behave in a manner similar to read(),
which just returns the string.

> 
> (I still go back and forth on whether this was the best approach, or just
return
> a string and let the caller deal with it using splitlines or rstrip
> respectively. But, if any, changes would probably better wait until we finish
> the migration.)

(In RunShellCommand's case, I also go back and forth on this, but in the
opposite direction - just return a list, or provide the option.)

> 
> Callers that want a list of lines:
> src/build/android/tombstones.py
>
src/tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper_unittest.py
> 
> Callers that want a single line:
> src/build/android/pylib/flag_changer.py
> src/build/android/pylib/perf/thermal_throttle.py
> src/tools/telemetry/telemetry/core/forwarders/android_forwarder.py
>
rc/tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py
> 
> Callers that want a string (so they '\n'.join the result):
> src/build/android/pylib/instrumentation/test_runner.py
> src/tools/telemetry/telemetry/core/platform/android_platform_backend.py
> 
>
https://codereview.chromium.org/775333002/diff/1/build/android/pylib/device/d...
> File build/android/pylib/device/device_utils_test.py (left):
> 
>
https://codereview.chromium.org/775333002/diff/1/build/android/pylib/device/d...
> build/android/pylib/device/device_utils_test.py:1174: as_root=True)
> I thought we don't need to test this one any more, as it exercises the exact
> same paths as file does not exist. (And is already tested in RunShellCommand
> anyway)
> 
>
https://codereview.chromium.org/775333002/diff/1/build/android/pylib/device/d...
> File build/android/pylib/device/device_utils_test.py (right):
> 
>
https://codereview.chromium.org/775333002/diff/1/build/android/pylib/device/d...
> build/android/pylib/device/device_utils_test.py:256: ['shell', cmd], output,
> exit_code, str(self.device))
> Fixed a small bug I've found when testing this change. The output would come
up
> as "c a t ' ' s o m e / f i l e", because it thought cmd was a list of args.
> This is of course only in the test code, the actual live errors are not
> affected.

[jbudorick, 2014-12-04 15:43:26]

https://codereview.chromium.org/775333002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/775333002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:869: single_line: A boolean
indicating if only a single line of output is
As mentioned in the high-level comment, I think this function should return a
single string. Having it do so would make this option unnecessary.

[perezju, 2014-12-05 16:19:43]

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

[perezju, 2014-12-05 16:19:44]

We're planing to change the interface of DeviceUtils.ReadFile.

The main two changes are:
- return a single string (instead of a list of lines)
- if the file is not found throw an exception (instead of returning an empty
list)

skyostil@chromium.org: Please review changes in telemetry
jbudorick@chromium.org: Please review changes in build/android/pylib

In all cases I also tried to make my best guess as to whether the client wants
to throw an exception if the file does not exist, or do something different. But
please do check if my guesses make sense.

I have included (I think) all the current callers of ReadFile. I found them with
this search:
https://code.google.com/p/chromium/codesearch#search/&q=device%5C.ReadFile%5C...

Do let me know if you think that I might be missing something.

[Sami, 2014-12-05 16:38:45]

telemetry lgtm.

[jbudorick, 2014-12-05 16:53:37]

build/android/ lgtm

https://codereview.chromium.org/775333002/diff/20001/build/android/tombstones.py
File build/android/tombstones.py (right):

https://codereview.chromium.org/775333002/diff/20001/build/android/tombstones...
build/android/tombstones.py:66: return device.ReadFile(
If ReadFile is an analogue of read(), perhaps we could do something like
ReadFileLines that's an analogue of readlines()? Just a thought, not necessary
for this CL.

https://codereview.chromium.org/775333002/diff/20001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py
(right):

https://codereview.chromium.org/775333002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py:70:
except device_errors.CommandFailedError:
er... you're still using device_errors, even if you fail to load it. Shouldn't
you either not catch the ImportError, check whether device_errors has been
imported, or just catch any exception?

[perezju, 2014-12-08 10:11:09]

Sami, could you confirm on the issue of importing build/android modules in
telemetry?

https://codereview.chromium.org/775333002/diff/20001/build/android/tombstones.py
File build/android/tombstones.py (right):

https://codereview.chromium.org/775333002/diff/20001/build/android/tombstones...
build/android/tombstones.py:66: return device.ReadFile(
On 2014/12/05 16:53:36, jbudorick wrote:
> If ReadFile is an analogue of read(), perhaps we could do something like
> ReadFileLines that's an analogue of readlines()? Just a thought, not necessary
> for this CL.

That sounds like a good idea. I'll keep that in the doc with DeviceUtils
implementation notes.

https://codereview.chromium.org/775333002/diff/20001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py
(right):

https://codereview.chromium.org/775333002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py:70:
except device_errors.CommandFailedError:
On 2014/12/05 16:53:37, jbudorick wrote:
> er... you're still using device_errors, even if you fail to load it. Shouldn't
> you either not catch the ImportError, check whether device_errors has been
> imported, or just catch any exception?

hmm, yeah, this is a pattern found on some of the telemetry files, see e.g. [1].
As far as I understand, some of these files might get imported on environments
where build/android is not available (so the try/except ImportError is needed
for the file to load), but in those environments the contents are never actually
run.

Sami, could you confirm whether this would be also needed/appropriate here?
Alternatively, should I not catch the import error at the top, or catch any
exception here?

[1]:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

[Sami, 2014-12-08 12:20:19]

https://codereview.chromium.org/775333002/diff/20001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py
(right):

https://codereview.chromium.org/775333002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py:70:
except device_errors.CommandFailedError:
On 2014/12/08 10:11:09, perezju wrote:
> On 2014/12/05 16:53:37, jbudorick wrote:
> > er... you're still using device_errors, even if you fail to load it.
Shouldn't
> > you either not catch the ImportError, check whether device_errors has been
> > imported, or just catch any exception?
> 
> hmm, yeah, this is a pattern found on some of the telemetry files, see e.g.
[1].
> As far as I understand, some of these files might get imported on environments
> where build/android is not available (so the try/except ImportError is needed
> for the file to load), but in those environments the contents are never
actually
> run.
> 
> Sami, could you confirm whether this would be also needed/appropriate here?
> Alternatively, should I not catch the import error at the top, or catch any
> exception here?
> 
> [1]:
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

Right, like Juan explained, these files are imported but not run on platforms
other than Android. If execution actually gets here, we can assume device_errors
is present. We could add an assert but that seems a little redundant. In any
case, we need to catch the ImportError above to make this possible.

[jbudorick, 2014-12-08 14:07:24]

https://codereview.chromium.org/775333002/diff/20001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py
(right):

https://codereview.chromium.org/775333002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py:70:
except device_errors.CommandFailedError:
On 2014/12/08 12:20:18, Sami wrote:
> On 2014/12/08 10:11:09, perezju wrote:
> > On 2014/12/05 16:53:37, jbudorick wrote:
> > > er... you're still using device_errors, even if you fail to load it.
> Shouldn't
> > > you either not catch the ImportError, check whether device_errors has been
> > > imported, or just catch any exception?
> > 
> > hmm, yeah, this is a pattern found on some of the telemetry files, see e.g.
> [1].
> > As far as I understand, some of these files might get imported on
environments
> > where build/android is not available (so the try/except ImportError is
needed
> > for the file to load), but in those environments the contents are never
> actually
> > run.
> > 
> > Sami, could you confirm whether this would be also needed/appropriate here?
> > Alternatively, should I not catch the import error at the top, or catch any
> > exception here?
> > 
> > [1]:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> 
> Right, like Juan explained, these files are imported but not run on platforms
> other than Android. If execution actually gets here, we can assume
device_errors
> is present. We could add an assert but that seems a little redundant. In any
> case, we need to catch the ImportError above to make this possible.

sgtm

[perezju, 2014-12-08 15:07:22]

The CQ bit was checked by perezju@chromium.org

[commit-bot: I haz the power, 2014-12-08 15:07:48]

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

[commit-bot: I haz the power, 2014-12-08 16:04:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-08 16:04:28]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[perezju, 2014-12-09 17:17:17]

Sami, I had to edit a couple more telemetry files to fix some failing unittests.
Can you have a quick look? I left a couple of trybots running. If all looks
fine, I'll try to commit again tomorrow morning.

[Sami, 2014-12-09 17:46:09]

https://codereview.chromium.org/775333002/diff/80001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py
(right):

https://codereview.chromium.org/775333002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py:72:
logging.warning('Temperature file contents: %r', contents)
Why did we need to remove this? Someone may want to log the temperatures for a
reason.

[jbudorick, 2014-12-09 17:49:00]

https://codereview.chromium.org/775333002/diff/80001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py
(right):

https://codereview.chromium.org/775333002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py:72:
logging.warning('Temperature file contents: %r', contents)
On 2014/12/09 17:46:08, Sami wrote:
> Why did we need to remove this? Someone may want to log the temperatures for a
> reason.

Note that this log message wasn't in here to begin with.

https://codereview.chromium.org/775333002/diff/100001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py
(left):

https://codereview.chromium.org/775333002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py:59:
def _GetBoardTemperatureCelsius(self):
(i.e., no logging in the original)

[Sami, 2014-12-09 19:40:52]

Changes in telemetry lgtm.

https://codereview.chromium.org/775333002/diff/80001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py
(right):

https://codereview.chromium.org/775333002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/power_monitor/android_temperature_monitor.py:72:
logging.warning('Temperature file contents: %r', contents)
On 2014/12/09 17:48:59, jbudorick wrote:
> On 2014/12/09 17:46:08, Sami wrote:
> > Why did we need to remove this? Someone may want to log the temperatures for
a
> > reason.
> 
> Note that this log message wasn't in here to begin with.

Ack, serves me right for just looking at just the diff of diffs.

[perezju, 2014-12-10 09:21:02]

The CQ bit was checked by perezju@chromium.org

[commit-bot: I haz the power, 2014-12-10 09:21:28]

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

[commit-bot: I haz the power, 2014-12-10 10:26:48]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-10 10:28:00]

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

[perezju, 2014-12-10 16:07:54]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/796433002/ by perezju@chromium.org.

The reason for reverting is: Broke telemetry android_browser_backend due to
missing client change.