Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1468053008:
Add LogDog bootstrapping to `annotated_run.py`. (Closed)
DescriptionAdd LogDog bootstrapping to `annotated_run.py`.
In the process, refactor a bit of the internals to:
- Cleanup old stuff.
- Use a singleton execution tempdir instead of several tempdirs.
- Use the RecipeRuntime object for operations.
- Use "argparse" instead of "optparse".
- Use "logging" instead of "print".
BUG=chromium:550673
TEST=`./scripts/slave/unittests/annotated_run_test.py`
Patch Set 1 #
Total comments: 1
Patch Set 2 : Merged into `annotated_run.py`. #
Total comments: 25
Patch Set 3 : Fix a comment. #Patch Set 4 : Strip out argparse, use local "gce.py" (cloned from depot_tools), docstrings. #
Total comments: 32
Patch Set 5 : Fixes? #Messages
Total messages: 27 (7 generated)
dnj@chromium.org changed reviewers: + estaab@chromium.org, iannucci@chromium.org, stip@chromium.org
PTAL. This is the first shot at a launcher wrapper for `annoated_run.py` that loops LogDog into the mix.
https://codereview.chromium.org/1468053008/diff/1/scripts/master/factory/anno... File scripts/master/factory/annotator_commands.py (left): https://codereview.chromium.org/1468053008/diff/1/scripts/master/factory/anno... scripts/master/factory/annotator_commands.py:29: runner = self.PathJoin(self._script_dir, 'annotated_run.py') ugh. Please no. This means we need to restart all the masters. The wrapper behavior should be achievable by just integrating it into annotated_run.py.
estaab@chromium.org changed reviewers: - estaab@chromium.org
Moving myself to cc since I don't know this code well enough (but I'd like to so I'll follow along).
On 2015/11/26 00:37:34, iannucci wrote: > https://codereview.chromium.org/1468053008/diff/1/scripts/master/factory/anno... > File scripts/master/factory/annotator_commands.py (left): > > https://codereview.chromium.org/1468053008/diff/1/scripts/master/factory/anno... > scripts/master/factory/annotator_commands.py:29: runner = > self.PathJoin(self._script_dir, 'annotated_run.py') > ugh. Please no. This means we need to restart all the masters. The wrapper > behavior should be achievable by just integrating it into annotated_run.py. I considered that. I didn't do this because: - Leaving `annotated_run.py` as-is means that the launcher can be iteratively developed and refined with no large-scale risk to our builds so long as the fallback path works. - `annotated_run.py` is already ~500 lines of Python, and integrating the OS-specific logic and CIPD logic would increase that a fair amount. - Restarting all masters is not unreasonable, since this is master-by-master opt-in anyways. I'm not really opposed to `annotated_run.py` integration, but those reasons seemed sufficient to move this in a separate program.
You can also just call it from annotated_run. I'm really not in favor of modifying the matter factory. We already have an entry point on the slave. IMO there is no advantage to renaming the entry point. If this CL introduces a bug, you will also need another restart to revert it. Opt in per master (and/or builder) should occur on the slave side like we did with bot update, not via there matter side, and definitely not via a mechanism that drops all the builds to flip the switch. I'm very strongly opposed to modifying the master to implement this. On Thu, Nov 26, 2015, 00:12 <dnj@google.com> wrote: > On 2015/11/26 00:37:34, iannucci wrote: > > > https://codereview.chromium.org/1468053008/diff/1/scripts/master/factory/anno... > > File scripts/master/factory/annotator_commands.py (left): > > > > https://codereview.chromium.org/1468053008/diff/1/scripts/master/factory/anno... > > scripts/master/factory/annotator_commands.py:29: runner = > > self.PathJoin(self._script_dir, 'annotated_run.py') > > ugh. Please no. This means we need to restart all the masters. The > wrapper > > behavior should be achievable by just integrating it into > > annotated_run.py. > > I considered that. I didn't do this because: > - Leaving `annotated_run.py` as-is means that the launcher can be > iteratively > developed and refined with no large-scale risk to our builds so long as the > fallback path works. > - `annotated_run.py` is already ~500 lines of Python, and integrating the > OS-specific logic and CIPD logic would increase that a fair amount. > - Restarting all masters is not unreasonable, since this is > master-by-master > opt-in anyways. > > I'm not really opposed to `annotated_run.py` integration, but those reasons > seemed sufficient to move this in a separate program. > > https://codereview.chromium.org/1468053008/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/11/26 15:37:34, iannucci wrote: > You can also just call it from annotated_run. I'm really not in favor of > modifying the matter factory. We already have an entry point on the slave. > IMO there is no advantage to renaming the entry point. > > If this CL introduces a bug, you will also need another restart to revert > it. > > Opt in per master (and/or builder) should occur on the slave side like we > did with bot update, not via there matter side, and definitely not via a > mechanism that drops all the builds to flip the switch. > > I'm very strongly opposed to modifying the master to implement this. Opt in is still per slave here. But I understand - I'll go ahead and merge all of this into `annotated_run.py`.
Description was changed from ========== Add LogDog annotation launcher. Add annotation launcher script. Switch AnnotationFactory to run through `launcher.py` instead of `annotated_run.py` directly. BUG=chromium:550673 TEST=local - `./scripts/slave/launcher.py -vv -d -- fake args` ========== to ========== Add LogDog bootstrapping to `annotated_run.py`. In the process, refactor a bit of the internals to: - Cleanup old stuff. - Use a singleton execution tempdir instead of several tempdirs. - Use the RecipeRuntime object for operations. - Use "argparse" instead of "optparse". - Use "logging" instead of "print". BUG=chromium:550673 TEST=`./scripts/slave/unittests/annotated_run_test.py` ==========
Patchset #2 (id:20001) has been deleted
dnj@google.com changed reviewers: + dnj@google.com, pgervais@chromium.org
Okay updated to merge into `annotated_run.py`. PTAL. +prgervais@ b/c this changes some things about the monitoring event code that he wrote.
(Pinging, since I submitted this over holiday).
iannucci@chromium.org changed reviewers: + luqui@chromium.org
+luqui will comment more, but wanted to CC https://chromiumcodereview.appspot.com/1468053008/diff/40001/environment.cfg.py File environment.cfg.py (right): https://chromiumcodereview.appspot.com/1468053008/diff/40001/environment.cfg.... environment.cfg.py:18: os.path.join(cwd, os.pardir, 'depot_tools'), uh... come again? I have no idea what this does, but it looks bad... https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/common/c... File scripts/common/chromium_utils.py (right): https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/common/c... scripts/common/chromium_utils.py:1451: return json.loads(zlib.decompress(base64.b64decode(value))) why does this have the same docstring as the function above it?
https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... File scripts/slave/annotated_run.py (right): https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:43: LOGDOG_ERROR_RETURNCODES = ( I think the logdog stuff should be in a different FILE, but should be called from annotated_run. That's what I meant before, but maybe I wasn't clear. https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:50: # Whitelist of {master}=>[{builder}|WHITELIST_ALL] whitelisting specific masters is {builder} a set? https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:157: def enter(cls, leak, **kw): what is leak? docstring? https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:163: fields.update(kw) why not put this stuff in __init__? why is there `fields` and also `self._fields`? What are fields? https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:185: def __getattr__(self, key): this seems unnecessarily fancy... why not just use get() everywhere? Or __getitem__ if you must. https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:206: class CIPD(object): separate file? https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:289: def _logdog_bootstrap(rt, opts, cmd): need docstrings for these suckers https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:492: parser = argparse.ArgumentParser( separate CL please https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:541: if os.environ.pop('RUN_SLAVE_UPDATED_SCRIPTS', None) is None: technically this is a semantic change: before it would only be popped if it was set and non-empty. Now it will be popped if it's empty. Not sure if that's desired or not. https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:706: _assert_logdog_whitelisted(rt) why not just have this return a status code? I don't see the benefit of the raise/except switch statement here? edit: oh, I see... _logdog_bootstrap can raise too... mumble :/ https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:708: status = _logdog_bootstrap(rt, opts, cmd) So IIUC, on windows this will now look like annotated_run.py # does update scripts annotated_run.py # checks to see if we're logdogging butler # logdog's annotated_run.py # has logdogging code disabled and actually does work ? *sigh*. What's the mechanism by which the 3rd annotated_run doesn't re-run logdogging code? I'm not seeing it :/
https://chromiumcodereview.appspot.com/1468053008/diff/40001/environment.cfg.py File environment.cfg.py (right): https://chromiumcodereview.appspot.com/1468053008/diff/40001/environment.cfg.... environment.cfg.py:18: os.path.join(cwd, os.pardir, 'depot_tools'), On 2015/12/01 02:12:46, iannucci wrote: > uh... come again? I have no idea what this does, but it looks bad... This adds "depot_tools" to standard paths so its libraries are importable. I can cherry-pick add it to "annotated_run.py", but since "build" assumes a depot_tools checking, this seems reasonable. https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/common/c... File scripts/common/chromium_utils.py (right): https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/common/c... scripts/common/chromium_utils.py:1451: return json.loads(zlib.decompress(base64.b64decode(value))) On 2015/12/01 02:12:46, iannucci wrote: > why does this have the same docstring as the function above it? Copy/paste :/
On 2015/12/01 at 02:39:09, dnj wrote: > https://chromiumcodereview.appspot.com/1468053008/diff/40001/environment.cfg.py > File environment.cfg.py (right): > > https://chromiumcodereview.appspot.com/1468053008/diff/40001/environment.cfg.... > environment.cfg.py:18: os.path.join(cwd, os.pardir, 'depot_tools'), > On 2015/12/01 02:12:46, iannucci wrote: > > uh... come again? I have no idea what this does, but it looks bad... > > This adds "depot_tools" to standard paths so its libraries are importable. I can cherry-pick add it to "annotated_run.py", but since "build" assumes a depot_tools checking, this seems reasonable. > This seems really scary... what uses environment.py? Generally depot_tools shouldn't be considered or used as importable code... what are you using it for? > https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/common/c... > File scripts/common/chromium_utils.py (right): > > https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/common/c... > scripts/common/chromium_utils.py:1451: return json.loads(zlib.decompress(base64.b64decode(value))) > On 2015/12/01 02:12:46, iannucci wrote: > > why does this have the same docstring as the function above it? > > Copy/paste :/
On 2015/12/01 02:41:52, iannucci wrote: > On 2015/12/01 at 02:39:09, dnj wrote: > > > https://chromiumcodereview.appspot.com/1468053008/diff/40001/environment.cfg.py > > File environment.cfg.py (right): > > > > > https://chromiumcodereview.appspot.com/1468053008/diff/40001/environment.cfg.... > > environment.cfg.py:18: os.path.join(cwd, os.pardir, 'depot_tools'), > > On 2015/12/01 02:12:46, iannucci wrote: > > > uh... come again? I have no idea what this does, but it looks bad... > > > > This adds "depot_tools" to standard paths so its libraries are importable. I > can cherry-pick add it to "annotated_run.py", but since "build" assumes a > depot_tools checking, this seems reasonable. > > > > This seems really scary... what uses environment.py? Generally depot_tools > shouldn't be considered or used as importable code... what are you using it for? Basically everything in "build" and "build_internal". I'm using it because I need to import "gerrit_util" for its GCE test implementation. I was under the impression that "depot_tools" is a dependency of "build", so this feels reasonable.
Build and depot tools are codependent in an "omfg this is a huge painful headache" way, not a "this is a reasonable thing" way. How is this code intended to live in the world where we isolate recipes? Is Butler going to be provided on the slave, or is it expected to be isolated with the recipe? On Mon, Nov 30, 2015, 18:53 <dnj@chromium.org> wrote: > On 2015/12/01 02:41:52, iannucci wrote: > > On 2015/12/01 at 02:39:09, dnj wrote: > > > > > > https://chromiumcodereview.appspot.com/1468053008/diff/40001/environment.cfg.py > > > File environment.cfg.py (right): > > > > > > > > > https://chromiumcodereview.appspot.com/1468053008/diff/40001/environment.cfg.... > > > environment.cfg.py:18: os.path.join(cwd, os.pardir, 'depot_tools'), > > > On 2015/12/01 02:12:46, iannucci wrote: > > > > uh... come again? I have no idea what this does, but it looks bad... > > > > > > This adds "depot_tools" to standard paths so its libraries are > > importable. I > > can cherry-pick add it to "annotated_run.py", but since "build" assumes a > > depot_tools checking, this seems reasonable. > > > > > > This seems really scary... what uses environment.py? Generally > depot_tools > > shouldn't be considered or used as importable code... what are you using > > it > for? > > Basically everything in "build" and "build_internal". I'm using it because > I > need to import "gerrit_util" for its GCE test implementation. I was under > the > impression that "depot_tools" is a dependency of "build", so this feels > reasonable. > > https://chromiumcodereview.appspot.com/1468053008/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/12/01 03:03:50, iannucci wrote: > Build and depot tools are codependent in an "omfg this is a huge painful > headache" way, not a "this is a reasonable thing" way. I didn't say it was pretty :/ The alternative is for me to duplicate the code from depot_tools, which seems less ideal given that there _is_ a dependency, but is something I'm not opposed to if we want to pretend there isn't. > How is this code intended to live in the world where we isolate recipes? Is > Butler going to be provided on the slave, or is it expected to be isolated > with the recipe? Presumably we'd isolate all dependencies, "depot_tools" included? But that's fine, I'll go ahead and copy the logic into "build".
https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... File scripts/slave/annotated_run.py (right): https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:50: # Whitelist of {master}=>[{builder}|WHITELIST_ALL] whitelisting specific masters On 2015/12/01 02:38:01, iannucci wrote: > is {builder} a set? I was using that notation to mean "key", but I can update to not intersect with Python. https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:157: def enter(cls, leak, **kw): On 2015/12/01 02:38:01, iannucci wrote: > what is leak? docstring? Done. https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:163: fields.update(kw) On 2015/12/01 02:38:01, iannucci wrote: > why not put this stuff in __init__? why is there `fields` and also > `self._fields`? What are fields? In general, I don't like __init__ doing actual work, so a classmethod is better here. Added doc, renamed to "attrs". https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:185: def __getattr__(self, key): On 2015/12/01 02:38:01, iannucci wrote: > this seems unnecessarily fancy... why not just use get() everywhere? Or > __getitem__ if you must. I dunno, I think this usage looks way better. https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:206: class CIPD(object): On 2015/12/01 02:38:01, iannucci wrote: > separate file? I wasn't intending to make this a first-class library, or annotated_run.py for that matter. Do you really think that's warranted here? https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:289: def _logdog_bootstrap(rt, opts, cmd): On 2015/12/01 02:38:01, iannucci wrote: > need docstrings for these suckers Done. https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:492: parser = argparse.ArgumentParser( On 2015/12/01 02:38:01, iannucci wrote: > separate CL please mmk https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:541: if os.environ.pop('RUN_SLAVE_UPDATED_SCRIPTS', None) is None: On 2015/12/01 02:38:01, iannucci wrote: > technically this is a semantic change: before it would only be popped if it was > set and non-empty. Now it will be popped if it's empty. Not sure if that's > desired or not. Good point, I'll go ahead and revert this. https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:706: _assert_logdog_whitelisted(rt) On 2015/12/01 02:38:01, iannucci wrote: > why not just have this return a status code? I don't see the benefit of the > raise/except switch statement here? > > edit: oh, I see... _logdog_bootstrap can raise too... mumble :/ I chose exceptions here because I'm already surrounding the bootstrap in a failsafe try...finally block, so they fit. https://chromiumcodereview.appspot.com/1468053008/diff/40001/scripts/slave/an... scripts/slave/annotated_run.py:708: status = _logdog_bootstrap(rt, opts, cmd) On 2015/12/01 02:38:01, iannucci wrote: > So IIUC, on windows this will now look like > > annotated_run.py # does update scripts > annotated_run.py # checks to see if we're logdogging > butler # logdog's > annotated_run.py # has logdogging code disabled and actually does work > > ? > > *sigh*. > > What's the mechanism by which the 3rd annotated_run doesn't re-run logdogging > code? I'm not seeing it :/ It should be: annotated_run.py annotated_run.py butler annotee recipes.py If butler/annotee fails or is disabled, it will follow up with the standard: annotated_run.py annotated_run.py recipes.py
Some comments. I'm wondering whether we really need to handle cipd from here instead of puppet. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... File scripts/slave/annotated_run.py (right): https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:79: # XXX: Get this right? XXX == FIXME, TODO? https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:90: 'logdog_cipd_packages': { Why is this handled by annotated_run.py? It seems to be Puppet's job. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:143: raise ValueError('Process exited with non-zero return code (%d)' % (rv,)) Can we raise CalledProcessError here to be consistent with subprocess? https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:148: """RecipeRuntime is the platform-specific runtime enviornment. Typo: enviornment https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:151: combination of plaetform and runtime values used in the setup and execution of Typo here as well. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:166: automatically cleaned up. It returns a RecipeRuntime object containing a It is cleaned up if this script does not crash before getting to it. It might be wise to add a garbage-collecting cron job like the one we have for /var/log/chrome-infra. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:201: def __getattr__(self, key): What is the purpose of returning the superclass attributes before the instance's ones? https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:225: def __init__(self, path, root): Nit: path -> cipd_path https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:235: if os.path.isfile(candidate) and os.access(candidate, os.X_OK): is_executable() instead https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:255: '# Generated at: %s' % (datetime.datetime.now().isoformat(),), This does not show the timezone. You can use this instead: datetime.datetime.utcnow().isoformat('Z') which is unambiguous. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:302: raise LogDogBootstrapError('Could not find service account credentials.') Listing the paths that have been tried will help with debugging. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:317: rt (RecipeRuntime): Recipe runtime enviornment. typo: enviornment https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:606: rv, _ = _run_command(gclient_cmd, cwd=BUILD_ROOT) Forgotten dry_run kwarg? https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:703: level = logging.DEBUG Can we have intermediate log levels as well? https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:735: with open(props_file, 'w') as fh: 'wb' ? https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:762: # TODO(pgervais): Send events from build_data_dir to the endpoint. Please remove this comment, it's obsolete.
Discussed offline:
* will deploy butler/annotee via puppet/cipd
* will refactor config to be passed around as a NamedTuple
* will override logdog binary locations with build_properties e.g. {'infra':
{'logdog': {...}}. The 'infra' property will be used for holding documented
infrastructure service configuration parameters (e.g. our API). Other users will
at least include DM, but will likely include other things as well.
This should reduce the amount of code a fair amount, and should (hopefully) help
with greppability. Additionally, the puppet portion will mesh with our long-term
plan of how we provide butler/annotee to recipes.
I split the refactoring part into this CL: https://codereview.chromium.org/1492613002 This is a follow-on that just adds LogDog bootstrapping. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... File scripts/slave/annotated_run.py (right): https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:79: # XXX: Get this right? On 2015/12/01 18:58:28, pgervais wrote: > XXX == FIXME, TODO? XXX, want to fix this before commit. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:90: 'logdog_cipd_packages': { On 2015/12/01 18:58:28, pgervais wrote: > Why is this handled by annotated_run.py? It seems to be Puppet's job. Done. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:143: raise ValueError('Process exited with non-zero return code (%d)' % (rv,)) On 2015/12/01 18:58:28, pgervais wrote: > Can we raise CalledProcessError here to be consistent with subprocess? Done. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:148: """RecipeRuntime is the platform-specific runtime enviornment. On 2015/12/01 18:58:28, pgervais wrote: > Typo: enviornment Acknowledged. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:151: combination of plaetform and runtime values used in the setup and execution of On 2015/12/01 18:58:28, pgervais wrote: > Typo here as well. Acknowledged. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:166: automatically cleaned up. It returns a RecipeRuntime object containing a On 2015/12/01 18:58:28, pgervais wrote: > It is cleaned up if this script does not crash before getting to it. It might be > wise to add a garbage-collecting cron job like the one we have for > /var/log/chrome-infra. Perma-crash yes, but the layout also helps with that: The layout is: <ROOT>/.recipe_runtime/nonce Each run will have a different nonce, ensuring that they will never conflict, but each run blows away "<ROOT>/.recipe_runtime", cleaning up previous runs. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:201: def __getattr__(self, key): On 2015/12/01 18:58:28, pgervais wrote: > What is the purpose of returning the superclass attributes before the instance's > ones? Rewrote. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:225: def __init__(self, path, root): On 2015/12/01 18:58:28, pgervais wrote: > Nit: path -> cipd_path Acknowledged. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:235: if os.path.isfile(candidate) and os.access(candidate, os.X_OK): On 2015/12/01 18:58:28, pgervais wrote: > is_executable() instead Deprecated. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:255: '# Generated at: %s' % (datetime.datetime.now().isoformat(),), On 2015/12/01 18:58:28, pgervais wrote: > This does not show the timezone. You can use this instead: > > datetime.datetime.utcnow().isoformat('Z') which is unambiguous. Deprecated. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:302: raise LogDogBootstrapError('Could not find service account credentials.') On 2015/12/01 18:58:28, pgervais wrote: > Listing the paths that have been tried will help with debugging. Done. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:317: rt (RecipeRuntime): Recipe runtime enviornment. On 2015/12/01 18:58:28, pgervais wrote: > typo: enviornment Done. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:606: rv, _ = _run_command(gclient_cmd, cwd=BUILD_ROOT) On 2015/12/01 18:58:28, pgervais wrote: > Forgotten dry_run kwarg? This happens before arguments get parsed. The dry run is for the final recipe invocation, so I don't think it's appropriate here. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:703: level = logging.DEBUG On 2015/12/01 18:58:28, pgervais wrote: > Can we have intermediate log levels as well? Such as? I don't think there's anything between INFO and DEBUG. Or are you suggesting I promote all "info"-level logs to WARNING? I don't see any value here, since ATM I am only using "info" and "debug". https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:735: with open(props_file, 'w') as fh: On 2015/12/01 18:58:28, pgervais wrote: > 'wb' ? It's JSON, which is text, not binary. https://chromiumcodereview.appspot.com/1468053008/diff/80001/scripts/slave/an... scripts/slave/annotated_run.py:762: # TODO(pgervais): Send events from build_data_dir to the endpoint. On 2015/12/01 18:58:28, pgervais wrote: > Please remove this comment, it's obsolete. Done.
I'm closing this and re-opening as: https://codereview.chromium.org/1501663002 After being split, all of these patch sets are fairly useless to diff against the current state.
Message was sent while issue was closed.
dnj@chromium.org changed reviewers: + martiniss@chromium.org |
