Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1492613002:
annotated_run: Cleanup/refactor. (Closed)
Descriptionannotated_run: Cleanup/refactor.
In the process, refactor a bit of the internals to:
- Use a singleton execution tempdir instead of several tempdirs.
- Use a platform-aware Config.
- Use "logging" instead of "print".
BUG=chromium:550673
TEST=`./scripts/slave/unittests/annotated_run_test.py`
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=297811
Patch Set 1 #
Total comments: 8
Patch Set 2 : Code review comments/tweaks. #
Total comments: 1
Patch Set 3 : Bug fixes! #Patch Set 4 : Integrate https://chromiumcodereview.appspot.com/1441013002/ #Patch Set 5 : Use standard paths, fix BuildInternal error. #
Total comments: 1
Patch Set 6 : Fix. #Messages
Total messages: 24 (11 generated)
dnj@chromium.org changed reviewers: + iannucci@chromium.org, pgervais@chromium.org
PTAL
lgtm https://codereview.chromium.org/1492613002/diff/1/scripts/slave/annotated_run.py File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1492613002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:103: LOGGER.info('(Dry Run) Not executing command.') '(Dry Run) But not really!' https://codereview.chromium.org/1492613002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:121: def recipe_tempdir(root=None, leak=False): TODO(crbug.com/361343): Make the recipe engine do this instead of doing it here. Maybe. https://codereview.chromium.org/1492613002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:141: # TODO(pgervais): use infra_libs.rmtree instead. I think we actually want RemoveDirectory from https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/commo... windows is :( https://codereview.chromium.org/1492613002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:430: LOGGER.debug('Using temporary directory: [%s].', tdir) move log line to recipe_tmpdir
dnj@google.com changed reviewers: + dnj@google.com
https://codereview.chromium.org/1492613002/diff/1/scripts/slave/annotated_run.py File scripts/slave/annotated_run.py (right): https://codereview.chromium.org/1492613002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:103: LOGGER.info('(Dry Run) Not executing command.') On 2015/12/02 02:13:34, iannucci wrote: > '(Dry Run) But not really!' I was confused, like "but it returns!", but you're just talking about the log sequence. I'll make this suck less. https://codereview.chromium.org/1492613002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:121: def recipe_tempdir(root=None, leak=False): On 2015/12/02 02:13:34, iannucci wrote: > TODO(crbug.com/361343): Make the recipe engine do this instead of doing it here. > Maybe. We _could_ do that, but this script uses the tempdir for: 1) Dumping master JSON. 2) ts_mon output directory 3) LogDog Butler named pipe (later). 4) LogDog Butler argument JSON (later). I think it is probably warranted regardless. https://codereview.chromium.org/1492613002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:141: # TODO(pgervais): use infra_libs.rmtree instead. On 2015/12/02 02:13:34, iannucci wrote: > I think we actually want RemoveDirectory from > https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/commo... > > windows is :( ew yuk. Done. https://codereview.chromium.org/1492613002/diff/1/scripts/slave/annotated_run... scripts/slave/annotated_run.py:430: LOGGER.debug('Using temporary directory: [%s].', tdir) On 2015/12/02 02:13:34, iannucci wrote: > move log line to recipe_tmpdir Hmm, I just moved it out actually. The idea was that "recipe_tempdir" has no idea _how_ the directory is going to be used, so it just creates it. Having the requester report it allows it to be put into context. Granted somewhat moot b/c it's a one-time call, but I think it's a good practice.
lgtm with one comment. https://chromiumcodereview.appspot.com/1492613002/diff/20001/scripts/slave/an... File scripts/slave/annotated_run.py (right): https://chromiumcodereview.appspot.com/1492613002/diff/20001/scripts/slave/an... scripts/slave/annotated_run.py:58: ('Windows',): {}, While you're at it, could you integrate this CL?https://chromiumcodereview.appspot.com/1441013002/
Patchset #4 (id:60001) has been deleted
I integrated that CL. Can you PTAL and let me know if it looks reasonable?
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
On 2015/12/02 19:42:27, dnj wrote: > I integrated that CL. Can you PTAL and let me know if it looks reasonable? It does. Thanks!
The CQ bit was checked by dnj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iannucci@chromium.org, pgervais@chromium.org Link to the patchset: https://codereview.chromium.org/1492613002/#ps120001 (title: "Integrate https://chromiumcodereview.appspot.com/1441013002/")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492613002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1492613002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build Presubmit on tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Build%20Presubmit/build...)
lgtm https://chromiumcodereview.appspot.com/1492613002/diff/140001/scripts/slave/a... File scripts/slave/annotated_run.py (right): https://chromiumcodereview.appspot.com/1492613002/diff/140001/scripts/slave/a... scripts/slave/annotated_run.py:457: recipe_runner = os.path.join(common.env.Build, move up to 451
The CQ bit was checked by dnj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pgervais@chromium.org, iannucci@chromium.org Link to the patchset: https://codereview.chromium.org/1492613002/#ps160001 (title: "Fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492613002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1492613002/160001
Message was sent while issue was closed.
Description was changed from ========== annotated_run: Cleanup/refactor. In the process, refactor a bit of the internals to: - Use a singleton execution tempdir instead of several tempdirs. - Use a platform-aware Config. - Use "logging" instead of "print". BUG=chromium:550673 TEST=`./scripts/slave/unittests/annotated_run_test.py` ========== to ========== annotated_run: Cleanup/refactor. In the process, refactor a bit of the internals to: - Use a singleton execution tempdir instead of several tempdirs. - Use a platform-aware Config. - Use "logging" instead of "print". BUG=chromium:550673 TEST=`./scripts/slave/unittests/annotated_run_test.py` Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=297811 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=297811
Message was sent while issue was closed.
I suspect this commit had unforeseen consequences. See https://codereview.chromium.org/1500123003/.
Message was sent while issue was closed.
On 2015/12/05 03:24:05, Vadim Sh. wrote: > I suspect this commit had unforeseen consequences. See > https://codereview.chromium.org/1500123003/. It did, but I fixed all of this two days ago. Worst-case a builder had to cycle twice to pick up the change, but things should be okay now. Which builder was still exhibiting errors? |
