Flip DeviceUtils.RunShellCommand to use check_return=True by default |
|||
Issue descriptionWe'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)
,
Feb 15 2017
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).
,
Feb 15 2017
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.
,
Feb 21 2017
,
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
,
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
,
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
,
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
,
Mar 28 2017
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 |
|||
Comment 1 by perezju@chromium.org
, Jul 22 2016