Add step generator protocol, remove annotated_checkout, remove script.

Add step generator protocol, remove annotated_checkout, remove script.

This will enable us to support recipes which decide which steps to run at
runtime and provides a testable mechanism for doing so.

I also added some better docs and fixed a few miscellaneous bugs.

annotated_checkout is now implemented in terms of the new generator mechanism,
as is the previous 'script' key.

I added coverage reporting for recipe_util, too, as I was curious. It's at a
whopping 14%. Hopefully I can beef that up soon.

R=agable@chromium.org
BUG=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=201184

[iannucci, 2013-05-17 09:23:03]

+xusydoc

[Mike Stip (use stip instead), 2013-05-18 00:37:21]

some comments, but lgtm

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/annot...
File scripts/slave/annotated_run.py (right):

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/annot...
scripts/slave/annotated_run.py:140: return run_steps(stream,
opts.build_properties, opts.factory_properties)[0]
This is a MakeStepsRetval, right? Shouldn't we use .rectode for clarity? Breaks
80 chars but would make the code easier to read.

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/annot...
scripts/slave/annotated_run.py:198: step_history.last_step = lambda:
step_history[next(reversed(step_history))]
document these

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/annot...
scripts/slave/annotated_run.py:202: def step_generator_wrapper():
small doc saying this demuxes a single step, a list of steps, or a step
generator

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/annot...
scripts/slave/annotated_run.py:223: gensteps = step_or_steps
is there a way you can unify this code with lines 204-211, say with a function?

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/git_s...
File scripts/slave/git_setup.py (right):

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/git_s...
scripts/slave/git_setup.py:46: run_git('remote', 'rm', 'origin', cwd=path)
may want to make this an option in case release scripts need something besides
origin

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/recip...
File scripts/slave/recipes_test/run_presubmit.blink.expected (right):

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/recip...
scripts/slave/recipes_test/run_presubmit.blink.expected:2: {"cmd":
["[DEPOT_TOOLS_ROOT]/gclient", "config", "--spec", "solutions = [{'managed':
True, 'custom_vars': {'webkit_trunk':
'svn://svn-mirror.golo.chromium.org/blink/trunk', 'googlecode_url':
'svn://svn-mirror.golo.chromium.org/%s', 'nacl_trunk':
'http://src.chromium.org/native_client/trunk', 'sourceforge_url':
'svn://svn-mirror.golo.chromium.org/%(repo)s'}, 'url':
'svn://svn-mirror.golo.chromium.org/chrome/trunk/src', 'custom_deps':
{'src/third_party/WebKit': 'svn://svn-mirror.golo.chromium.org/blink/trunk'},
'deps_file': 'DEPS', 'safesync_url': '', 'name': 'src'}]\n"], "name": "gclient
setup", "seed_steps": ["gclient setup", "gclient clean", "gclient sync"]},
really no way to make this rietveld-friendly?

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/unitt...
File scripts/slave/unittests/recipes_test.py (right):

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/unitt...
scripts/slave/unittests/recipes_test.py:148: json.dump(step, f, sort_keys=True)
might be possible to find a json prettifier...

[iannucci, 2013-05-18 04:00:36]

PTAL @ expected format now.

I also moved some stuff around so hopefully it's a bit less labyrinthian.

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/annot...
File scripts/slave/annotated_run.py (right):

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/annot...
scripts/slave/annotated_run.py:140: return run_steps(stream,
opts.build_properties, opts.factory_properties)[0]
On 2013/05/18 00:37:22, Mike Stipicevic wrote:
> This is a MakeStepsRetval, right? Shouldn't we use .rectode for clarity?
Breaks
> 80 chars but would make the code easier to read.

Good catch. Done.

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/annot...
scripts/slave/annotated_run.py:198: step_history.last_step = lambda:
step_history[next(reversed(step_history))]
On 2013/05/18 00:37:22, Mike Stipicevic wrote:
> document these

Done.

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/annot...
scripts/slave/annotated_run.py:202: def step_generator_wrapper():
On 2013/05/18 00:37:22, Mike Stipicevic wrote:
> small doc saying this demuxes a single step, a list of steps, or a step
> generator

Done.

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/annot...
scripts/slave/annotated_run.py:223: gensteps = step_or_steps
On 2013/05/18 00:37:22, Mike Stipicevic wrote:
> is there a way you can unify this code with lines 204-211, say with a
function?

Done. For bonus points, I refactored these nested methods out into proper
top-level methods without relying on closures. Go me.

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/git_s...
File scripts/slave/git_setup.py (right):

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/git_s...
scripts/slave/git_setup.py:46: run_git('remote', 'rm', 'origin', cwd=path)
On 2013/05/18 00:37:22, Mike Stipicevic wrote:
> may want to make this an option in case release scripts need something besides
> origin

Yeah I thought of that, but for now it's all origin. This will be easy to add
once we have a concrete use-case for it.

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/recip...
File scripts/slave/recipes_test/run_presubmit.blink.expected (right):

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/recip...
scripts/slave/recipes_test/run_presubmit.blink.expected:2: {"cmd":
["[DEPOT_TOOLS_ROOT]/gclient", "config", "--spec", "solutions = [{'managed':
True, 'custom_vars': {'webkit_trunk':
'svn://svn-mirror.golo.chromium.org/blink/trunk', 'googlecode_url':
'svn://svn-mirror.golo.chromium.org/%s', 'nacl_trunk':
'http://src.chromium.org/native_client/trunk', 'sourceforge_url':
'svn://svn-mirror.golo.chromium.org/%(repo)s'}, 'url':
'svn://svn-mirror.golo.chromium.org/chrome/trunk/src', 'custom_deps':
{'src/third_party/WebKit': 'svn://svn-mirror.golo.chromium.org/blink/trunk'},
'deps_file': 'DEPS', 'safesync_url': '', 'name': 'src'}]\n"], "name": "gclient
setup", "seed_steps": ["gclient setup", "gclient clean", "gclient sync"]},
On 2013/05/18 00:37:22, Mike Stipicevic wrote:
> really no way to make this rietveld-friendly?

Well... I could leave it like it was, but then it would be basically unreadable
in a normal diff view :/. This actually looks quite decent when viewing the
files in a real viewer.

Part of the ugly here is I totally changed the output format AND caused the
checkout steps to appear in the output, so rietveld thinks I just rewrote
everything.

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/unitt...
File scripts/slave/unittests/recipes_test.py (right):

https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/unitt...
scripts/slave/unittests/recipes_test.py:148: json.dump(step, f, sort_keys=True)
On 2013/05/18 00:37:22, Mike Stipicevic wrote:
> might be possible to find a json prettifier...

Maybe. One thing I toyed with is emitting things in the format of:

[
  {'cmd': [...],
   'other_attr': ..,
   'cool_setting': ...
  },
  {
  ...
  }
]

Do you think that would be a significant improvement?

Actually, screw it, that WOULD be a significant improvement :p. Done.

[iannucci, 2013-05-20 23:13:41]

On 2013/05/18 04:00:36, iannucci wrote:
> PTAL @ expected format now.
> 

ping :-)

> I also moved some stuff around so hopefully it's a bit less labyrinthian.
> 
>
https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/annot...
> File scripts/slave/annotated_run.py (right):
> 
>
https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/annot...
> scripts/slave/annotated_run.py:140: return run_steps(stream,
> opts.build_properties, opts.factory_properties)[0]
> On 2013/05/18 00:37:22, Mike Stipicevic wrote:
> > This is a MakeStepsRetval, right? Shouldn't we use .rectode for clarity?
> Breaks
> > 80 chars but would make the code easier to read.
> 
> Good catch. Done.
> 
>
https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/annot...
> scripts/slave/annotated_run.py:198: step_history.last_step = lambda:
> step_history[next(reversed(step_history))]
> On 2013/05/18 00:37:22, Mike Stipicevic wrote:
> > document these
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/annot...
> scripts/slave/annotated_run.py:202: def step_generator_wrapper():
> On 2013/05/18 00:37:22, Mike Stipicevic wrote:
> > small doc saying this demuxes a single step, a list of steps, or a step
> > generator
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/annot...
> scripts/slave/annotated_run.py:223: gensteps = step_or_steps
> On 2013/05/18 00:37:22, Mike Stipicevic wrote:
> > is there a way you can unify this code with lines 204-211, say with a
> function?
> 
> Done. For bonus points, I refactored these nested methods out into proper
> top-level methods without relying on closures. Go me.
> 
>
https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/git_s...
> File scripts/slave/git_setup.py (right):
> 
>
https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/git_s...
> scripts/slave/git_setup.py:46: run_git('remote', 'rm', 'origin', cwd=path)
> On 2013/05/18 00:37:22, Mike Stipicevic wrote:
> > may want to make this an option in case release scripts need something
besides
> > origin
> 
> Yeah I thought of that, but for now it's all origin. This will be easy to add
> once we have a concrete use-case for it.
> 
>
https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/recip...
> File scripts/slave/recipes_test/run_presubmit.blink.expected (right):
> 
>
https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/recip...
> scripts/slave/recipes_test/run_presubmit.blink.expected:2: {"cmd":
> ["[DEPOT_TOOLS_ROOT]/gclient", "config", "--spec", "solutions = [{'managed':
> True, 'custom_vars': {'webkit_trunk':
> 'svn://svn-mirror.golo.chromium.org/blink/trunk', 'googlecode_url':
> 'svn://svn-mirror.golo.chromium.org/%s', 'nacl_trunk':
> 'http://src.chromium.org/native_client/trunk', 'sourceforge_url':
> 'svn://svn-mirror.golo.chromium.org/%(repo)s'}, 'url':
> 'svn://svn-mirror.golo.chromium.org/chrome/trunk/src', 'custom_deps':
> {'src/third_party/WebKit': 'svn://svn-mirror.golo.chromium.org/blink/trunk'},
> 'deps_file': 'DEPS', 'safesync_url': '', 'name': 'src'}]\n"], "name": "gclient
> setup", "seed_steps": ["gclient setup", "gclient clean", "gclient sync"]},
> On 2013/05/18 00:37:22, Mike Stipicevic wrote:
> > really no way to make this rietveld-friendly?
> 
> Well... I could leave it like it was, but then it would be basically
unreadable
> in a normal diff view :/. This actually looks quite decent when viewing the
> files in a real viewer.
> 
> Part of the ugly here is I totally changed the output format AND caused the
> checkout steps to appear in the output, so rietveld thinks I just rewrote
> everything.
> 
>
https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/unitt...
> File scripts/slave/unittests/recipes_test.py (right):
> 
>
https://chromiumcodereview.appspot.com/15270004/diff/3002/scripts/slave/unitt...
> scripts/slave/unittests/recipes_test.py:148: json.dump(step, f,
sort_keys=True)
> On 2013/05/18 00:37:22, Mike Stipicevic wrote:
> > might be possible to find a json prettifier...
> 
> Maybe. One thing I toyed with is emitting things in the format of:
> 
> [
>   {'cmd': [...],
>    'other_attr': ..,
>    'cool_setting': ...
>   },
>   {
>   ...
>   }
> ]
> 
> Do you think that would be a significant improvement?
> 
> Actually, screw it, that WOULD be a significant improvement :p. Done.

[Mike Stip (use stip instead), 2013-05-21 00:47:44]

lgtm

[commit-bot: I haz the power, 2013-05-21 01:00:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/iannucci@chromium.org/15270004/10001

[commit-bot: I haz the power, 2013-05-21 01:01:50]

Change committed as 201184