Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(139)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2130863002: Check environment (Closed)

Created:
4 years, 5 months ago by nicholaslin
Modified:
4 years, 5 months ago
CC:
chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Check environment BUG=623789

Patch Set 1 #

Patch Set 2 : Trying slave #

Patch Set 3 : First try, logdog args #

Patch Set 4 : Check swarming env #

Patch Set 5 : Add device info to logdog. #

Total comments: 9

Patch Set 6 : Move most of logdog logic outside test_runner harness. #

Patch Set 7 : Delete unnecessary line breaks. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+19 lines, -1 line) Patch
M build/android/pylib/local/device/local_device_environment.py View 1 2 3 4 5 6 4 chunks +12 lines, -1 line 0 comments Download
M build/android/test_runner.py View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M tools/mb/mb.py View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 4 (1 generated)
nicholaslin
Adding logdog streaming for logcats. These changes will depend on passing the new --logdog-output-dir flag ...
4 years, 5 months ago (2016-07-08 02:33:29 UTC) #2
jbudorick
I'm concerned with how entangled this is w/ logdog. I don't think this should know ...
4 years, 5 months ago (2016-07-08 02:50:41 UTC) #3
nicholaslin
4 years, 5 months ago (2016-07-09 01:07:47 UTC) #4 
On 2016/07/08 02:50:41, jbudorick wrote:
> I'm concerned with how entangled this is w/ logdog. I don't think this should
> know so much about logdog.
> 
> Also, please give this CL a more descriptive title and indicate that it's
> android-specific.
> 
>
https://codereview.chromium.org/2130863002/diff/80001/build/android/pylib/loc...
> File build/android/pylib/local/device/local_device_environment.py (right):
> 
>
https://codereview.chromium.org/2130863002/diff/80001/build/android/pylib/loc...
> build/android/pylib/local/device/local_device_environment.py:123: 
> ?
> 
>
https://codereview.chromium.org/2130863002/diff/80001/build/android/pylib/loc...
> build/android/pylib/local/device/local_device_environment.py:132: #Might be
> redundant with usage of logdog
> Remove this.
> 
>
https://codereview.chromium.org/2130863002/diff/80001/build/android/pylib/loc...
> build/android/pylib/local/device/local_device_environment.py:145: for m in
> self._logcat_monitors:
> Can we run this in parallel?
>
https://codesearch.chromium.org/chromium/src/third_party/catapult/devil/devil...
> ?
> 
>
https://codereview.chromium.org/2130863002/diff/80001/build/android/pylib/loc...
> build/android/pylib/local/device/local_device_environment.py:146:
device_serial
> = (m._adb).__str__()
> 1) The linter should have been unhappy about this, particularly the protected
> member access. If you need to grab the AdbWrapper instance from the
> LogcatMonitor instead of the DeviceUtils instance (which, AFAICT, you do),
then
> let's add a property to LogcatMonitor that exposes _adb.
> 2) Use AdbWrapper.GetDeviceSerial() instead of __str__() if you want the
serial.
> 
>
https://codereview.chromium.org/2130863002/diff/80001/build/android/pylib/loc...
> build/android/pylib/local/device/local_device_environment.py:147:
> add_device_args = ['sed', '-i', '-e',
> Do this in-process rather than calling out to sed.
> 
>
https://codereview.chromium.org/2130863002/diff/80001/build/android/pylib/loc...
> build/android/pylib/local/device/local_device_environment.py:150:
> subprocess.check_call(add_device_args)
> Use cmd_helper instead of subprocess:
>
https://codesearch.chromium.org/chromium/src/third_party/catapult/devil/devil...
> 
>
https://codereview.chromium.org/2130863002/diff/80001/build/android/pylib/loc...
> build/android/pylib/local/device/local_device_environment.py:155: # will be
used
> once recipes are intergrated
> 1) Commented-out code shouldn't be here.
> 2) What is this doing...? This seems way too specific.
> 
>
https://codereview.chromium.org/2130863002/diff/80001/build/android/pylib/loc...
> build/android/pylib/local/device/local_device_environment.py:160: logdog_args
=
> [butler_location, '-log-level', 'debug', '-project',
> This knows way too much about logdog and how to run it. If we're running this
at
> the end, why can't a post-processing step in the recipe -- i.e., outside the
> test runner -- handle this?
> 
>
https://codereview.chromium.org/2130863002/diff/80001/build/android/pylib/loc...
> build/android/pylib/local/device/local_device_environment.py:186: print
'Logcats
> are located at {0}'.format(url_constructor(project, prefix, name))
> No prints, please. If you want to log something, use logging (this would be
> logging.info).

Working on taking logdog out of test_runner. If everything works the only
changes that should be made will be:

1. either changing the logcat-output-dir flag to logcat-output-file or adding an
extra logdog-file flag for streaming (test_runner)
2. adding in the device serials to the logcats (local_device_monitor,
devil/logcat_monitor)

Powered by Google App Engine
This is Rietveld 408576698