Cross-repo recipe package system.

Cross-repo recipe package system.

This rolls the package system created in https://codereview.chromium.org/1344583003/ into tools/build, moving core modules to the engine.  See in particular the new entry point scripts/slave/recipes.py and the recipes configuration infra/config/recipes.cfg.

Internal changes: https://chromereviews.googleplex.com/227207013/

TBR for uniform chromium_tests changes.

BUG=459840
TBR=phajdan.jr@chromium.org

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

[luqui, 2015-07-23 20:30:42]

luqui@chromium.org changed reviewers:
+ iannucci@chromium.org

[luqui, 2015-07-23 20:30:43]

Here's what we're reviewing today.

+martiniss fyi.

https://codereview.chromium.org/1241323004/diff/1/scripts/slave/recipe_packag...
File scripts/slave/recipe_package.pyl (right):

https://codereview.chromium.org/1241323004/diff/1/scripts/slave/recipe_packag...
scripts/slave/recipe_package.pyl:6: "repo": "/b/build",
This will refer to the new recipe_engine repo.

[iannucci, 2015-07-26 23:22:42]

Should I review this as-is, or did you need to tweak it pending the notes from
our meeting?

[luqui, 2015-07-27 15:42:14]

The CQ bit was checked by luqui@chromium.org

[luqui, 2015-07-27 15:42:14]

The CQ bit was checked by luqui@chromium.org

[luqui, 2015-07-27 15:42:17]

The CQ bit was unchecked by luqui@chromium.org

[commit-bot: I haz the power, 2015-07-27 15:42:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1241323004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1241323004/20001

[commit-bot: I haz the power, 2015-07-27 15:42:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-27 15:42:21]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[luqui, 2015-07-27 15:42:28]

It needs some tweakage

[luqui, 2015-08-05 01:52:50]

Ok I think she's ready for review now.

I still have to move show_me_the_modules.  Once I get an l-g-t-m, I'll move it
to the new repo and test that everything still works, abandon this change, but
keep recipe_engine vendored in build while we test the new repo on infra.

[luqui, 2015-08-06 20:57:57]

https://codereview.chromium.org/1241323004/diff/80001/scripts/slave/annotated...
File scripts/slave/annotated_run.py (right):

https://codereview.chromium.org/1241323004/diff/80001/scripts/slave/annotated...
scripts/slave/annotated_run.py:27: PACKAGE_PYL = os.path.join(
Oops.  TODO: fix this script and write a test for it %)

[iannucci, 2015-08-06 23:57:13]

https://codereview.chromium.org/1241323004/diff/80001/scripts/slave/recipes.py
File scripts/slave/recipes.py (right):

https://codereview.chromium.org/1241323004/diff/80001/scripts/slave/recipes.p...
scripts/slave/recipes.py:16: if not os.path.exists(ENGINE_DIR):
let's use `git rev-parse --something` to find the root directory and calculate
the abspath for .recipe_engine.

want to start making a README for the new repo:
  * copy this script into your repo (at "path"?)
  * add .recipe_engine to your .gitignore
  * what are all the assumptions about where stuff goes (e.g. for the roller)
  * ...?

let's make all the bootstrap stuff live in a subdirectory, maybe, and then clone
a subtree of that, just to keep the amount of stale code at a minimum. Then we
can have a README in that bootstrap directory describing how changes to that
directory need to propagate differently than the rest of the repo.

Maybe fetch it to HEAD 1/day in this script?

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
File third_party/recipe_engine/package.proto (right):

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.proto:16: required int32 api_version = 1;  //
Version 1
everything is optional :D

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
File third_party/recipe_engine/package.py (right):

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:29: class ProtoFile(object):
document why an object and not just a collection of functions?

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:74: proto_file = ProtoFile(proto_file)
nuke

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:80: 'Recipes config must be in
infra/config/recipes.cfg from root of repo\n'
maybe need to talk to luci-cfg to see if this is the right path... maybe we
don't need to. There debate about making this flexible.

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:83: % (expected_path, proto_path))
can we validate this higher in the stack? like main()? or arg parsing

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:89: recipes_path.replace('/', '\\')
os.sep instead of '\\'

  recipes_path = buf.recipes_path.replace('/', os.sep)

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:113: def checkout(self, context):
need check_checkout (and whatever other methods there are)

maybe a docstring for RepoSpec?

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:139: _run_cmd(['git', 'fetch'])
mebbeh use cwd=

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:191: '%s..origin/%s' % (self.revision,
self.branch),
'origin' is an assumption? or is this a repo managed precisely by us.

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:226: self.repo = repo
s/repo/repo_spec

doc type of deps

doc that repo_spec is used externally

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:285: def updates(self, context):
need docstrings

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:293:
ret_updates.append(RepoUpdate(PackageSpec(
maybe don't need RepoUpdate because it's only used for comparison and we don't
end up comparing them?

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:308: yield update
remove consistent_spec

try:
  ...
  yield update
except ...:
  ...

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:328: def create(cls, proto_file,
fetch=False):
doc: proto_file is the root-repo's `infra/config/recipes.cfg` file

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:330: proto_file = ProtoFile(proto_file)
nuke2faic (always require ProtoFile)

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:334: root_package =
package_deps._create_package(RootRepoSpec(), fetch)
maybe name 'allowFetch'?

just call _create_from_spec directly here?

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:349: 'infra', 'config', 'recipes.cfg')
can this 'infra' 'config' join concept be abstracted everywhere? too many
hardcoded strings?

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:354: def _create_from_spec(self, repo_spec,
package_spec, fetch):
I have sneaking suspicion that repo_spec is only really used for repo_root from
this point on.... looks like it maybe used for revision too. I'm a tad worried
that someone might call a state-affecting method (like 'checkout') on the object
elsewhere in the code, even though I would expect that the only call to checkout
happens at PackageDeps._create_package time.

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:360: not repo_spec ==
self._repos[package_spec.project_id].repo):
!=




possibly?

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:370: self._repos[package_spec.project_id] =
package
need a cycle breaker "currently loading" object per project_id

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:396: def _parse_date(datestr):
don't need this :P

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:412: def _merge2(xs, ys, compare=lambda x,
y: x <= y):
maybe just

return sorted(xs + ys)

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:431: 
not_a_thing = object()

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
File third_party/recipe_engine/recipe_modules/example/__init__.py (right):

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/recipe_modules/example/__init__.py:2:
'recipe_engine/step',
is it possible for us to do the core module moves in a different cl

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
File third_party/recipe_engine/recipes.py (right):

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/recipes.py:1: #!/usr/bin/env python
maybe call this main.py and rename main.py to run.py

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/recipes.py:16: sys.path.insert(0,
os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
!!!!!!!

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/recipes.py:113: def roll(args):
maybe have option to do this in a loop until it can't find any consistent rolls?

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/recipes.py:208: 'show_me_the_modules',
maybe 'doc'? 

doc <module name>
doc <recipe name>
doc --all-modules

?

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/recipes.py:221: package_deps =
package.PackageDeps.create(get_package_config(args))
let's have a `fetch = <condition>` line, and then `package_deps = ....(...,
fetch=fetch)` maybe a bit clearer.

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
File third_party/recipe_engine/unittests/multi_repo_test.py (right):

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/unittests/multi_repo_test.py:25: def
_in_directory(target_dir):
why two copies? :D

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/unittests/multi_repo_test.py:58: def
_recstrify(thing):
don't use?

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/unittests/multi_repo_test.py:197:
self._run_roll(repos['d'], expect_updates=True)
when we allow multi-roll ops, this should probably turn into a number instead of
a bool

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/unittests/multi_repo_test.py:215: def
test_dependent_roll(self):
add example test for adding/deleting a dependency

[luqui, 2015-08-07 19:30:36]

https://codereview.chromium.org/1241323004/diff/80001/infra/config/recipes.cfg
File infra/config/recipes.cfg (right):

https://codereview.chromium.org/1241323004/diff/80001/infra/config/recipes.cf...
infra/config/recipes.cfg:8: branch: "recipe-packages"
Require that branch be master for now.

https://codereview.chromium.org/1241323004/diff/80001/infra/config/recipes.cf...
infra/config/recipes.cfg:9: revision: "recipe-packages"
Ban HEAD and junk

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
File third_party/recipe_engine/package.py (right):

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:13: sys.path.append(
import third_party.google instead

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:139: _run_cmd(['git', 'fetch'])
check & fetch, auto-fetch.

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:293:
ret_updates.append(RepoUpdate(PackageSpec(
Document how rolling work.
Document global coherence and why we don't do it.

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
File third_party/recipe_engine/recipes.py (right):

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/recipes.py:6: recipe_package.pyl file.
Update docstring

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/recipes.py:164: '--bootstrap', action='store_true',
--no-fetch

Don't checkout recipe engine at HEAD.  Instead have copypasta bootstrap check
out pinned recipe engine by parsing the protobuf stupidly and then just forward
to the pinned recipe engine right off.

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
File
third_party/recipe_engine/third_party/python_dateutil-1.5.dist-info/top_level.txt
(right):

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/third_party/python_dateutil-1.5.dist-info/top_level.txt:1:
dateutil
dateutil no longer needed

[luqui, 2015-08-20 22:45:25]

https://codereview.chromium.org/1241323004/diff/80001/scripts/slave/annotated...
File scripts/slave/annotated_run.py (right):

https://codereview.chromium.org/1241323004/diff/80001/scripts/slave/annotated...
scripts/slave/annotated_run.py:27: PACKAGE_PYL = os.path.join(
On 2015/08/06 at 20:57:57, luqui wrote:
> Oops.  TODO: fix this script and write a test for it %)

Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
File third_party/recipe_engine/package.proto (right):

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.proto:16: required int32 api_version = 1;  //
Version 1
On 2015/08/06 at 23:57:12, iannucci wrote:
> everything is optional :D

Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
File third_party/recipe_engine/package.py (right):

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:13: sys.path.append(
On 2015/08/07 at 19:30:36, luqui wrote:
> import third_party.google instead

Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:29: class ProtoFile(object):
On 2015/08/06 at 23:57:12, iannucci wrote:
> document why an object and not just a collection of functions?

Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:74: proto_file = ProtoFile(proto_file)
On 2015/08/06 at 23:57:12, iannucci wrote:
> nuke

Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:80: 'Recipes config must be in
infra/config/recipes.cfg from root of repo\n'
On 2015/08/06 at 23:57:12, iannucci wrote:
> maybe need to talk to luci-cfg to see if this is the right path... maybe we
don't need to. There debate about making this flexible.

Ugh, it is configurable.

Since we don't need this for initial rollout, I'm going to abstract it a little
in preparation but not go all the way.

This is a bigger problem for the bootstrap. The alternatives are:

1) have the bootstrap parse refs.cfg (a protobuf) in order to find the correct
location.
2) add a special variable in the bootstrap where you can say "use this as the
config file instead"

I'm thinking (2) right now.  I know it's ugly, but the extra complexity of doing
it automatically in a bootstrap script which cannot be updated is substantial. 
Doing (2) means you can't copypasta an updated bootstrap script, mostly.  Ughhhh

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:83: % (expected_path, proto_path))
On 2015/08/06 at 23:57:13, iannucci wrote:
> can we validate this higher in the stack? like main()? or arg parsing

Done.

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:89: recipes_path.replace('/', '\\')
On 2015/08/06 at 23:57:12, iannucci wrote:
> os.sep instead of '\\'
> 
>   recipes_path = buf.recipes_path.replace('/', os.sep)

Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:113: def checkout(self, context):
On 2015/08/06 at 23:57:12, iannucci wrote:
> need check_checkout (and whatever other methods there are)
> 
> maybe a docstring for RepoSpec?

Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:139: _run_cmd(['git', 'fetch'])
On 2015/08/06 at 23:57:13, iannucci wrote:
> mebbeh use cwd=

Done

> check & fetch, auto-fetch.

Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:191: '%s..origin/%s' % (self.revision,
self.branch),
On 2015/08/06 at 23:57:12, iannucci wrote:
> 'origin' is an assumption? or is this a repo managed precisely by us.

This repo is managed by us.

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:226: self.repo = repo
On 2015/08/06 at 23:57:12, iannucci wrote:
> s/repo/repo_spec
> 
> doc type of deps
> 
> doc that repo_spec is used externally

Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:285: def updates(self, context):
On 2015/08/06 at 23:57:12, iannucci wrote:
> need docstrings

Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:293:
ret_updates.append(RepoUpdate(PackageSpec(
On 2015/08/07 at 19:30:36, luqui wrote:
> Document how rolling work.
> Document global coherence and why we don't do it.

Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:308: yield update
On 2015/08/06 at 23:57:12, iannucci wrote:
> remove consistent_spec
> 
> try:
>   ...
>   yield update
> except ...:
>   ...


Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:328: def create(cls, proto_file,
fetch=False):
On 2015/08/06 at 23:57:13, iannucci wrote:
> doc: proto_file is the root-repo's `infra/config/recipes.cfg` file

Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:330: proto_file = ProtoFile(proto_file)
On 2015/08/06 at 23:57:12, iannucci wrote:
> nuke2faic (always require ProtoFile)

Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:334: root_package =
package_deps._create_package(RootRepoSpec(), fetch)
On 2015/08/06 at 23:57:12, iannucci wrote:
> maybe name 'allowFetch'?

allow_fetch, Done.

> 
> just call _create_from_spec directly here?

I think this way makes more logical sense actually.

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:349: 'infra', 'config', 'recipes.cfg')
On 2015/08/06 at 23:57:12, iannucci wrote:
> can this 'infra' 'config' join concept be abstracted everywhere? too many
hardcoded strings?

Done mostly

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:360: not repo_spec ==
self._repos[package_spec.project_id].repo):
On 2015/08/06 at 23:57:12, iannucci wrote:
> !=
> 
> 
> 
> 
> possibly?

Done







.

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:370: self._repos[package_spec.project_id] =
package
On 2015/08/06 at 23:57:12, iannucci wrote:
> need a cycle breaker "currently loading" object per project_id

Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:396: def _parse_date(datestr):
On 2015/08/06 at 23:57:13, iannucci wrote:
> don't need this :P

Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:412: def _merge2(xs, ys, compare=lambda x,
y: x <= y):
On 2015/08/06 at 23:57:13, iannucci wrote:
> maybe just
> 
> return sorted(xs + ys)

Will not work, since we need the order in xs and ys to be preserved

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/package.py:431: 
On 2015/08/06 at 23:57:12, iannucci wrote:
> not_a_thing = object()

Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
File third_party/recipe_engine/recipes.py (right):

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/recipes.py:1: #!/usr/bin/env python
On 2015/08/06 at 23:57:13, iannucci wrote:
> maybe call this main.py and rename main.py to run.py

Done.

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/recipes.py:6: recipe_package.pyl file.
On 2015/08/07 at 19:30:36, luqui wrote:
> Update docstring

Done.

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/recipes.py:16: sys.path.insert(0,
os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
On 2015/08/06 at 23:57:13, iannucci wrote:
> !!!!!!!

!!!!!!!

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/recipes.py:113: def roll(args):
On 2015/08/06 at 23:57:13, iannucci wrote:
> maybe have option to do this in a loop until it can't find any consistent
rolls?

On second thought, no.  The reason for one at a time is so that we can run
simulation tests and junk.  We would want to roll as far forward as those
passed, but no farther, so this is not the right place for this logic.

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/recipes.py:164: '--bootstrap', action='store_true',
On 2015/08/07 at 19:30:36, luqui wrote:
> --no-fetch
> 
> Don't checkout recipe engine at HEAD.  Instead have copypasta bootstrap check
out pinned recipe engine by parsing the protobuf stupidly and then just forward
to the pinned recipe engine right off.

Done.

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/recipes.py:208: 'show_me_the_modules',
On 2015/08/06 at 23:57:13, iannucci wrote:
> maybe 'doc'? 

Done

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/recipes.py:221: package_deps =
package.PackageDeps.create(get_package_config(args))
On 2015/08/06 at 23:57:13, iannucci wrote:
> let's have a `fetch = <condition>` line, and then `package_deps = ....(...,
fetch=fetch)` maybe a bit clearer.

Nope, because of error message stuff.

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
File third_party/recipe_engine/unittests/multi_repo_test.py (right):

https://codereview.chromium.org/1241323004/diff/80001/third_party/recipe_engi...
third_party/recipe_engine/unittests/multi_repo_test.py:58: def
_recstrify(thing):
On 2015/08/06 at 23:57:13, iannucci wrote:
> don't use?

Done.

[luqui, 2015-09-14 18:29:08]

https://codereview.chromium.org/1241323004/diff/180001/infra/config/recipes.cfg
File infra/config/recipes.cfg (right):

https://codereview.chromium.org/1241323004/diff/180001/infra/config/recipes.c...
infra/config/recipes.cfg:7: url: "https://github.com/luqui/recipes-py.git"
This must change to googlesource once we're mirrored there.

https://codereview.chromium.org/1241323004/diff/180001/scripts/slave/recipes.py
File scripts/slave/recipes.py (right):

https://codereview.chromium.org/1241323004/diff/180001/scripts/slave/recipes....
scripts/slave/recipes.py:19: ENGINE_REPO =
'https://github.com/luqui/recipes-py.git'
Change to googlesource before land

[iannucci, 2015-09-15 00:10:48]

lgtm % things

definitely need to have a trooper-aware PSA rollout day (tomorrow afternoon
maybe?).

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/annotate...
File scripts/slave/annotated_run.py (right):

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/annotate...
scripts/slave/annotated_run.py:213: # Find out of the recipe we intend to run is
in build_internal's recipes. If
s/of/if

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/recipes.py
File scripts/slave/recipes.py (right):

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/recipes....
scripts/slave/recipes.py:45: m = re.match(r'(\w+)\s*:\s*(.*)', line)
can this file have comments?

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/recipes....
scripts/slave/recipes.py:47: ret.setdefault(m.group(1),
[]).append(parse_atom(m.group(2)))
why not inline parse_atom? It's super small and only used once I think

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/recipes....
scripts/slave/recipes.py:75: [engine_buf] = [
whoa... how did I not know about this particular assignment syntax.... seems
like it might be safer to just call next() on the genexp tho, in case there are
somehow multiple matching things.

  foo = next(x for x in thingee)

is the efficient version of

  foo = [x for x in thingee][0]

Though you're actually using a list comp anyway (don't do that in general... use
genexps), so you're already paying the allocation overhead.

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/recipes/...
File scripts/slave/recipes/blink_trybot.py (right):

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/recipes/...
scripts/slave/recipes/blink_trybot.py:5: from recipe_engine.types import freeze
it occurs to me that 'freeze' isn't really a type...

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/recipes/...
File scripts/slave/recipes/gatekeeper.py (left):

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/recipes/...
scripts/slave/recipes/gatekeeper.py:23: # on the simulating machine.
lol. May want to ping stip about this?

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/unittest...
File scripts/slave/unittests/recipe_lint_test.py (right):

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/unittest...
scripts/slave/unittests/recipe_lint_test.py:24: args.extend(['-w', pattern])
This will work, but be aware that if MODULES_WHITELIST gets huge then it may
fail on windows due to cmdline length limits.

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/unittest...
File scripts/slave/unittests/recipe_simulation_test.py (right):

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/unittest...
scripts/slave/unittests/recipe_simulation_test.py:14: os.execvp(RECIPES_PY,
args)
this may 'splode on windows, not sure.

[luqui, 2015-09-15 19:17:42]

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/annotate...
File scripts/slave/annotated_run.py (right):

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/annotate...
scripts/slave/annotated_run.py:213: # Find out of the recipe we intend to run is
in build_internal's recipes. If
On 2015/09/15 00:10:48, iannucci wrote:
> s/of/if

Done.

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/recipes.py
File scripts/slave/recipes.py (right):

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/recipes....
scripts/slave/recipes.py:45: m = re.match(r'(\w+)\s*:\s*(.*)', line)
On 2015/09/15 00:10:48, iannucci wrote:
> can this file have comments?

It cannot.  For once this makes me happy.

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/recipes....
scripts/slave/recipes.py:47: ret.setdefault(m.group(1),
[]).append(parse_atom(m.group(2)))
On 2015/09/15 00:10:48, iannucci wrote:
> why not inline parse_atom? It's super small and only used once I think

Because it involves an intermediate variable, and mixes together some cleanly
separable functionality with a name.  "Why not split this out into a function?"

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/recipes....
scripts/slave/recipes.py:75: [engine_buf] = [
On 2015/09/15 00:10:48, iannucci wrote:
> whoa... how did I not know about this particular assignment syntax.... seems
> like it might be safer to just call next() on the genexp tho, in case there
are
> somehow multiple matching things.

Seems like it is safer to die here instead of choose arbitrarily in the case of
multiple matching things.  I'm going to pick the 0th element and warn if there
is more than one.

> 
>   foo = next(x for x in thingee)
> 
> is the efficient version of
> 
>   foo = [x for x in thingee][0]
> 
> Though you're actually using a list comp anyway (don't do that in general...
use
> genexps), so you're already paying the allocation overhead.

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/recipes/...
File scripts/slave/recipes/blink_trybot.py (right):

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/recipes/...
scripts/slave/recipes/blink_trybot.py:5: from recipe_engine.types import freeze
On 2015/09/15 00:10:48, iannucci wrote:
> it occurs to me that 'freeze' isn't really a type...

Acknowledged.

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/unittest...
File scripts/slave/unittests/recipe_lint_test.py (right):

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/unittest...
scripts/slave/unittests/recipe_lint_test.py:24: args.extend(['-w', pattern])
On 2015/09/15 00:10:48, iannucci wrote:
> This will work, but be aware that if MODULES_WHITELIST gets huge then it may
> fail on windows due to cmdline length limits.

Acknowledged.

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/unittest...
File scripts/slave/unittests/recipe_simulation_test.py (right):

https://codereview.chromium.org/1241323004/diff/220001/scripts/slave/unittest...
scripts/slave/unittests/recipe_simulation_test.py:14: os.execvp(RECIPES_PY,
args)
On 2015/09/15 00:10:48, iannucci wrote:
> this may 'splode on windows, not sure.

I tested recipes.py bootstrap on Windows which also uses execvp, and it worked
fine.

[luqui, 2015-09-15 19:18:50]

The CQ bit was checked by luqui@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-15 19:19:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1241323004/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1241323004/240001

[commit-bot: I haz the power, 2015-09-15 19:26:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-15 19:26:59]

Dry run: Try jobs failed on following builders:
  build_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/build_presubmit...)

[luqui, 2015-09-15 19:27:51]

The CQ bit was checked by luqui@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-15 19:28:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1241323004/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1241323004/240001

[commit-bot: I haz the power, 2015-09-15 19:35:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-15 19:35:58]

Dry run: Try jobs failed on following builders:
  build_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/build_presubmit...)

[luqui, 2015-09-15 21:00:17]

The CQ bit was checked by luqui@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-15 21:00:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1241323004/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1241323004/260001

[commit-bot: I haz the power, 2015-09-15 21:05:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-15 21:05:04]

Dry run: This issue passed the CQ dry run.

[luqui, 2015-09-16 20:57:39]

The CQ bit was checked by luqui@chromium.org

[luqui, 2015-09-16 20:57:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/1241323004/#ps320001
(title: "Roll to latest recipes-py")

[commit-bot: I haz the power, 2015-09-16 20:57:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1241323004/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1241323004/320001

[commit-bot: I haz the power, 2015-09-16 21:02:33]

Committed patchset #17 (id:320001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=296718

[luqui, 2015-09-16 21:10:03]

A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/1347263002/ by luqui@chromium.org.

The reason for reverting is: World exploded.