New issue
Advanced search Search tips

Issue 628617 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Feature



Sign in to add a comment

Flip DeviceUtils.RunShellCommand to use check_return=True by default

Project Member Reported by jbudorick@chromium.org, Jul 15 2016

Issue description

We've intended to do this for two+ years. Time to actually do it.

(motivated by https://bugs.chromium.org/p/chromium/issues/detail?id=623087)
 
+1!

returning now from vacation, hopefully next week I can start having a look to assess the amount of work and figure out how to better organize this.
Had a chance to give a quick look to this, and made a small script to check RunShellCommand calls that may need fixing.

Script as well as output running on both chromium and catapult repos:
https://codereview.chromium.org/2693893009

A total of 165 calls are missing check_return or have it set to something other than True.

What would be best course of action? Start slowly flipping it on bunches of, say, 20-30, files at a time?

Aside, I would also like to reduce the total number of calls to RunShellCommand (e.g. calls to 'rm' should use RemovePath instead).
That sounds reasonable enough, particularly if we split by first- or second-level directory.

Switching from RunShellCommand to more specific calls also sgtm but should be done separately.
Owner: perezju@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 5 by bugdroid1@chromium.org, Feb 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/480045526ec7bcf12c9f7726734a6bd592704ce6

commit 480045526ec7bcf12c9f7726734a6bd592704ce6
Author: catapult-deps-roller <catapult-deps-roller@chromium.org>
Date: Mon Feb 27 12:02:19 2017

Roll src/third_party/catapult/ 456b3a552..a75c463e8 (1 commit).

https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git/+log/456b3a55214e..a75c463e8416

$ git log 456b3a552..a75c463e8 --date=short --no-merges --format='%ad %ae %s'
2017-02-27 perezju [Telemetry] Switch RunShellCommand clients to check_return=True

Created with:
  roll-dep src/third_party/catapult
BUG=628617

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=catapult-sheriff@chromium.org

Review-Url: https://codereview.chromium.org/2718033003
Cr-Commit-Position: refs/heads/master@{#453194}

[modify] https://crrev.com/480045526ec7bcf12c9f7726734a6bd592704ce6/DEPS

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0256b97f24a3a7bc58cef9f291db1e3de2528f2a

commit 0256b97f24a3a7bc58cef9f291db1e3de2528f2a
Author: catapult-deps-roller <catapult-deps-roller@chromium.org>
Date: Mon Mar 20 16:50:57 2017

Roll src/third_party/catapult/ 2d86f956d..94b484ab7 (6 commits)

https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git/+log/2d86f956d8ed..94b484ab76c0

$ git log 2d86f956d..94b484ab7 --date=short --no-merges --format='%ad %ae %s'
2017-03-20 perezju [devil] Fix bug in surface_stats_collector.py
2017-03-20 perezju [Telemetry] Use shell=True as appropriate in android platform
2017-03-20 perezju [Telemetry] Use device.GetPids for android_backend.GetPsOutput
2017-03-20 simonhatch Dashboard - Fix retrieving multipart entities from ndb.
2017-03-20 nednguyen [cloud_storage] Throws CredentialError for cloud storage stderr of form ServiceException: 401 Anonymous users does..
2017-03-20 perezju [devil] Fix RunShellCommand usgaes in devil module.

Created with:
  roll-dep src/third_party/catapult
BUG=628617,628617

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=catapult-sheriff@chromium.org

Review-Url: https://codereview.chromium.org/2761833002
Cr-Commit-Position: refs/heads/master@{#458095}

[modify] https://crrev.com/0256b97f24a3a7bc58cef9f291db1e3de2528f2a/DEPS

Project Member

Comment 7 by bugdroid1@chromium.org, Mar 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9252579e4ab9a83ff8a863288bf554aa82877585

commit 9252579e4ab9a83ff8a863288bf554aa82877585
Author: catapult-deps-roller <catapult-deps-roller@chromium.org>
Date: Tue Mar 21 14:16:34 2017

Roll src/third_party/catapult/ 2310cee32..73de30828 (1 commit)

https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git/+log/2310cee32189..73de308284a8

$ git log 2310cee32..73de30828 --date=short --no-merges --format='%ad %ae %s'
2017-03-21 perezju [systrace] Fix device.RunShellCommand usages

Created with:
  roll-dep src/third_party/catapult
BUG=628617

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=catapult-sheriff@chromium.org

Review-Url: https://codereview.chromium.org/2764893002
Cr-Commit-Position: refs/heads/master@{#458400}

[modify] https://crrev.com/9252579e4ab9a83ff8a863288bf554aa82877585/DEPS

Project Member

Comment 8 by bugdroid1@chromium.org, Mar 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/69d8f2a50c2cc0221fd23292238f7fd168e81326

commit 69d8f2a50c2cc0221fd23292238f7fd168e81326
Author: Michael Achenbach <machenbach@chromium.org>
Date: Tue Mar 21 14:30:19 2017

[test] Update Android shell commands in perf runner

BUG=chromium:628617
NOTRY=true

Change-Id: I55923fec5ee34feea85dcc281b00f3d5d22283c5
Reviewed-on: https://chromium-review.googlesource.com/456710
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#43983}
[modify] https://crrev.com/69d8f2a50c2cc0221fd23292238f7fd168e81326/tools/run_perf.py

Owner: jbudorick@chromium.org
John, passing this back to you. Only files remaining with bad usages are:

- build/android/provision_devices.py
- build/android/pylib/content_settings.py
- build/android/pylib/device_settings.py
- build/android/pylib/valgrind_tools.py

Sign in to add a comment