Add simple depends_on API.

Add simple depends_on API.

BUG=550688
Committed: https://github.com/luci/recipes-py/commit/68b89f6aaec205975d9a741f64cbfa50a9efb8c1

[estaab, 2015-11-10 02:55:12]

estaab@chromium.org changed reviewers:
+ estaab@chromium.org

[estaab, 2015-11-10 02:55:13]

https://codereview.chromium.org/1421843006/diff/1/recipes/top.py
File recipes/top.py (right):

https://codereview.chromium.org/1421843006/diff/1/recipes/top.py#newcode17
recipes/top.py:17: res = api.depend_on('bottom', {'number': to_pass})
Since to_pass is just a plain Property() does that mean it doesn't have a type?
Is it valid to pass this directly into bottom which takes an int property type?

https://codereview.chromium.org/1421843006/diff/1/recipes/top.py#newcode18
recipes/top.py:18: return RETURN_SCHEMA(**res)
Would "return {'result': res['result']}" also be valid here?

[martiniss, 2015-11-13 00:17:12]

martiniss@chromium.org changed reviewers:
+ iannucci@chromium.org, luqui@chromium.org, phajdan.jr@chromium.org

[martiniss, 2015-11-13 00:17:12]

PTAL

Here's a better implementation. Still rather raw, but it does what I want to.
Look at top.py; you can run this by doing 
"./recipes.py --package infra/config/recipes.cfg run top to_pass=3"

Need to clean stuff up still.

https://codereview.chromium.org/1421843006/diff/1/recipes/top.py
File recipes/top.py (right):

https://codereview.chromium.org/1421843006/diff/1/recipes/top.py#newcode17
recipes/top.py:17: res = api.depend_on('bottom', {'number': to_pass})
On 2015/11/10 at 02:55:12, estaab wrote:
> Since to_pass is just a plain Property() does that mean it doesn't have a
type? Is it valid to pass this directly into bottom which takes an int property
type?

Technically yes, and naive simulation tests won't catch this. We would need to
add static analysis to our python programs, which I guess we could do.

More advanced simulation tests would catch it though, because the recipe api
would realize that you're passing in a bad type.

https://codereview.chromium.org/1421843006/diff/1/recipes/top.py#newcode18
recipes/top.py:18: return RETURN_SCHEMA(**res)
On 2015/11/10 at 02:55:12, estaab wrote:
> Would "return {'result': res['result']}" also be valid here?

Yes.

[martiniss, 2015-11-13 02:14:23]

Now with testing!!

Check out recipes/top.py. It uses the testing api thing
(https://docs.google.com/document/d/1mhlGG7qibTgunTZIq4oom4eDFxR-aylesEwU52bh2...)
!

[Paweł Hajdan Jr., 2015-11-13 12:30:00]

Nice work. Mostly minor comments.

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/recipe_te...
File recipe_engine/recipe_test_api.py (right):

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/recipe_te...
recipe_engine/recipe_test_api.py:178:
ret.depend_on_data.update(self.depend_on_data)
Would it make sense to use combineify here for consistency with mod_data and
step_data?

If not, consider adding a comment explaining why.

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/recipe_te...
recipe_engine/recipe_test_api.py:206: assert recipe not in self.depend_on_data,
(
If I understand this correctly, this uses recipe as a key, which means it
wouldn't be possible to have several invocations of the same recipe with
different properties - is that right?

I'd suggest changing the key from recipe to (recipe, properties). WDYT?

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/recipe_te...
recipe_engine/recipe_test_api.py:207: "Already gave test data for %s" % recipe)
nit: Prefer single quotes ' .

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/run.py#ne...
recipe_engine/run.py:953: return self._test_data.depend_on_data[recipe][1]
It's not obvious to me why [1] is needed at the end. What's going on here?

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/run.py#ne...
recipe_engine/run.py:954: elif True: # is_local
nit: "elif True" looks weird. Why not "else" with a TODO?

Actually with "return" above "else" shouldn't be needed and you could just
deindent the entire block.

https://codereview.chromium.org/1421843006/diff/80001/recipes/top.py
File recipes/top.py (right):

https://codereview.chromium.org/1421843006/diff/80001/recipes/top.py#newcode24
recipes/top.py:24: api.depend_on('bottom', {'number': 3}, {'number_times_two':
6})
Can we test cases of invoking the same recipe with different properties?

[Sergiy Byelozyorov, 2015-11-13 13:14:54]

sergiyb@chromium.org changed reviewers:
+ sergiyb@chromium.org

[Sergiy Byelozyorov, 2015-11-13 13:14:55]

drive-by, just some suggestions that can be ignored
I feel that Pawel is raising much more important questions

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/loader.py
File recipe_engine/loader.py (right):

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/loader.py...
recipe_engine/loader.py:133: class RecipeUniverse(object):
can you please document what this does or add a TODO here?

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/run.py#ne...
recipe_engine/run.py:959: cmd = ['python', recipes_py_loc,
please use sys.executable here

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/run.py#ne...
recipe_engine/run.py:964: rval = subprocess.call(cmd)
So currently this would run recipe engine with another recipe locally and block
caller? Please consider adding a comment explaining that and also explain what
this will do in DM world.

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/run.py#ne...
recipe_engine/run.py:965: return json.load(f)
I wonder if we should also return rval here... perhaps by adding it to the
resulting dict, e.g. something along

result = json.load(f)
result['recipe_return_code'] = rval
return result

[martiniss, 2015-11-13 23:19:03]

Minor fixes. Still need to do some more work to fix everything.

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/loader.py
File recipe_engine/loader.py (right):

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/loader.py...
recipe_engine/loader.py:133: class RecipeUniverse(object):
On 2015/11/13 at 13:14:55, Sergiy Byelozyorov wrote:
> can you please document what this does or add a TODO here?

luqui@ wrote this, would be a better person to document it.

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/recipe_te...
File recipe_engine/recipe_test_api.py (right):

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/recipe_te...
recipe_engine/recipe_test_api.py:178:
ret.depend_on_data.update(self.depend_on_data)
On 2015/11/13 at 12:30:00, Paweł Hajdan Jr. wrote:
> Would it make sense to use combineify here for consistency with mod_data and
step_data?
> 
> If not, consider adding a comment explaining why.

That works alright. Fixed.

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/recipe_te...
recipe_engine/recipe_test_api.py:206: assert recipe not in self.depend_on_data,
(
On 2015/11/13 at 12:30:00, Paweł Hajdan Jr. wrote:
> If I understand this correctly, this uses recipe as a key, which means it
wouldn't be possible to have several invocations of the same recipe with
different properties - is that right?
> 
> I'd suggest changing the key from recipe to (recipe, properties). WDYT?

Changed to recipe, properties.

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/recipe_te...
recipe_engine/recipe_test_api.py:207: "Already gave test data for %s" % recipe)
On 2015/11/13 at 12:30:00, Paweł Hajdan Jr. wrote:
> nit: Prefer single quotes ' .

Fixed.

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/run.py#ne...
recipe_engine/run.py:953: return self._test_data.depend_on_data[recipe][1]
On 2015/11/13 at 12:30:00, Paweł Hajdan Jr. wrote:
> It's not obvious to me why [1] is needed at the end. What's going on here?

Changed.

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/run.py#ne...
recipe_engine/run.py:954: elif True: # is_local
On 2015/11/13 at 12:30:00, Paweł Hajdan Jr. wrote:
> nit: "elif True" looks weird. Why not "else" with a TODO?
> 
> Actually with "return" above "else" shouldn't be needed and you could just
deindent the entire block.

Dedented, and added a todo to add DM stuff.

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/run.py#ne...
recipe_engine/run.py:959: cmd = ['python', recipes_py_loc,
On 2015/11/13 at 13:14:55, Sergiy Byelozyorov wrote:
> please use sys.executable here

Fixed.

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/run.py#ne...
recipe_engine/run.py:965: return json.load(f)
On 2015/11/13 at 13:14:55, Sergiy Byelozyorov wrote:
> I wonder if we should also return rval here... perhaps by adding it to the
resulting dict, e.g. something along
> 
> result = json.load(f)
> result['recipe_return_code'] = rval
> return result

I talked to estaab, and we agreed that what depend_on returns is not just the
result, but also a status_code of some kind. Not exactly sure what this will
look like, but this will change at some point. Shouldn't ignore the return code
I imagine.

https://codereview.chromium.org/1421843006/diff/80001/recipes/top.py
File recipes/top.py (right):

https://codereview.chromium.org/1421843006/diff/80001/recipes/top.py#newcode24
recipes/top.py:24: api.depend_on('bottom', {'number': 3}, {'number_times_two':
6})
On 2015/11/13 at 12:30:00, Paweł Hajdan Jr. wrote:
> Can we test cases of invoking the same recipe with different properties?

Sure, that should work. Not testing that for now, but I'll add some more test
recipes in the future.

[luqui, 2015-11-16 20:48:00]

Nice work overall.

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/loader.py
File recipe_engine/loader.py (right):

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/loader.p...
recipe_engine/loader.py:137: self._config_file = config_file
I don't see where ._config_file is used.

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_a...
File recipe_engine/recipe_api.py (right):

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_a...
recipe_engine/recipe_api.py:7: import json
I think all of these new imports are unused.

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_t...
File recipe_engine/recipe_test_api.py (right):

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_t...
recipe_engine/recipe_test_api.py:163: self.depend_on_data = {}
This is a little worrisome to me.  Why isn't depend_on just a special kind of
step?

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_t...
recipe_engine/recipe_test_api.py:217: 'depend_on_data': self.depend_on_data
ubernit: ultimate comma

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py#n...
recipe_engine/run.py:768: self._test_data, universe or self._universe)
Hmm, isn't test_data mutable, in order for example for the 'unconsumed test
data' check?

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py#n...
recipe_engine/run.py:959: recipe_script = self._universe.load_recipe(recipe)
This precludes one use case, which is that the recipe we depend on was checked
out by a previous step in the recipe, which will requite a more lenient test
mechanism.  This is well designed though, so we can cross that bridge when we
come to it.

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py#n...
recipe_engine/run.py:964: return
self._test_data.depend_on_data[types.freeze((recipe, properties),)]
line wrap

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py#n...
recipe_engine/run.py:974: '--result-file=%s' % f.name, recipe]
The convention for this argument has been --output-json AFAICT.   I did like
"result" there, though.  Maybe --output-result-json

https://codereview.chromium.org/1421843006/diff/100001/recipes/bottom.py
File recipes/bottom.py (right):

https://codereview.chromium.org/1421843006/diff/100001/recipes/bottom.py#newc...
recipes/bottom.py:1: from recipe_engine.recipe_api import Property
Seems like these should be in recipes/engine_tests with the others.

recipes/ has recipes which are actually intended to be run for a useful purpose
(which in recipes-py is a bit silly since there will likely never be any, but
you get the idea anyway?)

[Sergiy Byelozyorov, 2015-11-17 11:19:40]

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/1421843006/diff/80001/recipe_engine/run.py#ne...
recipe_engine/run.py:965: return json.load(f)
On 2015/11/13 23:19:03, martiniss wrote:
> On 2015/11/13 at 13:14:55, Sergiy Byelozyorov wrote:
> > I wonder if we should also return rval here... perhaps by adding it to the
> resulting dict, e.g. something along
> > 
> > result = json.load(f)
> > result['recipe_return_code'] = rval
> > return result
> 
> I talked to estaab, and we agreed that what depend_on returns is not just the
> result, but also a status_code of some kind. Not exactly sure what this will
> look like, but this will change at some point. Shouldn't ignore the return
code
> I imagine.

Please add a TODO if you are planning to make changes here later.

[estaab, 2015-11-17 23:07:16]

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py#n...
recipe_engine/run.py:977: rval = subprocess.call(cmd)
We should probably throw an exception when rval isn't 0 since we control the
process running the recipe and should always expect it to be 0, right?

[martiniss, 2015-11-18 01:00:00]

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/loader.py
File recipe_engine/loader.py (right):

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/loader.p...
recipe_engine/loader.py:137: self._config_file = config_file
On 2015/11/16 at 20:47:59, luqui wrote:
> I don't see where ._config_file is used.

Used for something else. Not needed, so I deleted.

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_a...
File recipe_engine/recipe_api.py (right):

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_a...
recipe_engine/recipe_api.py:7: import json
On 2015/11/16 at 20:47:59, luqui wrote:
> I think all of these new imports are unused.

Correct!

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_t...
File recipe_engine/recipe_test_api.py (right):

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_t...
recipe_engine/recipe_test_api.py:163: self.depend_on_data = {}
On 2015/11/16 at 20:48:00, luqui wrote:
> This is a little worrisome to me.  Why isn't depend_on just a special kind of
step?

Because it's not a step? I have to mock the output based on the key of (name,
properties). It seems strange to stick that into a string as the step name.

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_t...
recipe_engine/recipe_test_api.py:217: 'depend_on_data': self.depend_on_data
On 2015/11/16 at 20:48:00, luqui wrote:
> ubernit: ultimate comma

Done.

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py#n...
recipe_engine/run.py:768: self._test_data, universe or self._universe)
On 2015/11/16 at 20:48:00, luqui wrote:
> Hmm, isn't test_data mutable, in order for example for the 'unconsumed test
data' check?

Deleted this method. Not needed anymore.

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py#n...
recipe_engine/run.py:974: '--result-file=%s' % f.name, recipe]
On 2015/11/16 at 20:48:00, luqui wrote:
> The convention for this argument has been --output-json AFAICT.   I did like
"result" there, though.  Maybe --output-result-json

Fixed.

https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py#n...
recipe_engine/run.py:977: rval = subprocess.call(cmd)
On 2015/11/17 at 23:07:16, estaab wrote:
> We should probably throw an exception when rval isn't 0 since we control the
process running the recipe and should always expect it to be 0, right?

Good idea. Raising an InfraFailure.

https://codereview.chromium.org/1421843006/diff/100001/recipes/bottom.py
File recipes/bottom.py (right):

https://codereview.chromium.org/1421843006/diff/100001/recipes/bottom.py#newc...
recipes/bottom.py:1: from recipe_engine.recipe_api import Property
On 2015/11/16 at 20:48:00, luqui wrote:
> Seems like these should be in recipes/engine_tests with the others.
> 
> recipes/ has recipes which are actually intended to be run for a useful
purpose (which in recipes-py is a bit silly since there will likely never be
any, but you get the idea anyway?)

Sure, that's fine. Moved.

[estaab, 2015-11-18 01:05:48]

On 2015/11/18 01:00:00, martiniss wrote:
> https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/loader.py
> File recipe_engine/loader.py (right):
> 
>
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/loader.p...
> recipe_engine/loader.py:137: self._config_file = config_file
> On 2015/11/16 at 20:47:59, luqui wrote:
> > I don't see where ._config_file is used.
> 
> Used for something else. Not needed, so I deleted.
> 
>
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_a...
> File recipe_engine/recipe_api.py (right):
> 
>
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_a...
> recipe_engine/recipe_api.py:7: import json
> On 2015/11/16 at 20:47:59, luqui wrote:
> > I think all of these new imports are unused.
> 
> Correct!
> 
>
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_t...
> File recipe_engine/recipe_test_api.py (right):
> 
>
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_t...
> recipe_engine/recipe_test_api.py:163: self.depend_on_data = {}
> On 2015/11/16 at 20:48:00, luqui wrote:
> > This is a little worrisome to me.  Why isn't depend_on just a special kind
of
> step?
> 
> Because it's not a step? I have to mock the output based on the key of (name,
> properties). It seems strange to stick that into a string as the step name.
> 
>
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_t...
> recipe_engine/recipe_test_api.py:217: 'depend_on_data': self.depend_on_data
> On 2015/11/16 at 20:48:00, luqui wrote:
> > ubernit: ultimate comma
> 
> Done.
> 
> https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py
> File recipe_engine/run.py (right):
> 
>
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py#n...
> recipe_engine/run.py:768: self._test_data, universe or self._universe)
> On 2015/11/16 at 20:48:00, luqui wrote:
> > Hmm, isn't test_data mutable, in order for example for the 'unconsumed test
> data' check?
> 
> Deleted this method. Not needed anymore.
> 
>
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py#n...
> recipe_engine/run.py:974: '--result-file=%s' % f.name, recipe]
> On 2015/11/16 at 20:48:00, luqui wrote:
> > The convention for this argument has been --output-json AFAICT.   I did like
> "result" there, though.  Maybe --output-result-json
> 
> Fixed.
> 
>
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py#n...
> recipe_engine/run.py:977: rval = subprocess.call(cmd)
> On 2015/11/17 at 23:07:16, estaab wrote:
> > We should probably throw an exception when rval isn't 0 since we control the
> process running the recipe and should always expect it to be 0, right?
> 
> Good idea. Raising an InfraFailure.
> 
> https://codereview.chromium.org/1421843006/diff/100001/recipes/bottom.py
> File recipes/bottom.py (right):
> 
>
https://codereview.chromium.org/1421843006/diff/100001/recipes/bottom.py#newc...
> recipes/bottom.py:1: from recipe_engine.recipe_api import Property
> On 2015/11/16 at 20:48:00, luqui wrote:
> > Seems like these should be in recipes/engine_tests with the others.
> > 
> > recipes/ has recipes which are actually intended to be run for a useful
> purpose (which in recipes-py is a bit silly since there will likely never be
> any, but you get the idea anyway?)
> 
> Sure, that's fine. Moved.

Question, don't let it hold up committing this if others give an LG though. How
will these sequential child recipes show up in annotations and in the buildbot
build view?

[Paweł Hajdan Jr., 2015-11-18 16:56:03]

https://codereview.chromium.org/1421843006/diff/120001/recipe_engine/recipe_t...
File recipe_engine/recipe_test_api.py (right):

https://codereview.chromium.org/1421843006/diff/120001/recipe_engine/recipe_t...
recipe_engine/recipe_test_api.py:207: 'Already gave test data for %s' % recipe)
Shouldn't this print the entire tuple in error message, not just the recipe
part?

https://codereview.chromium.org/1421843006/diff/120001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/1421843006/diff/120001/recipe_engine/run.py#n...
recipe_engine/run.py:961: 
nit: Remove redundant empty line.

https://codereview.chromium.org/1421843006/diff/120001/recipe_engine/run.py#n...
recipe_engine/run.py:976: raise recipe_api.InfraFailure(
This isn't necessarily an infra failure, right? Dependent recipes can fail for
legitimate non-infra reasons. Or is such a failure reported in some other way?
In that case please clarify in a comment.

[martiniss, 2015-11-18 17:21:53]

On 2015/11/18 at 01:05:48, estaab wrote:
> On 2015/11/18 01:00:00, martiniss wrote:
> >
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/loader.py
> > File recipe_engine/loader.py (right):
> > 
> >
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/loader.p...
> > recipe_engine/loader.py:137: self._config_file = config_file
> > On 2015/11/16 at 20:47:59, luqui wrote:
> > > I don't see where ._config_file is used.
> > 
> > Used for something else. Not needed, so I deleted.
> > 
> >
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_a...
> > File recipe_engine/recipe_api.py (right):
> > 
> >
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_a...
> > recipe_engine/recipe_api.py:7: import json
> > On 2015/11/16 at 20:47:59, luqui wrote:
> > > I think all of these new imports are unused.
> > 
> > Correct!
> > 
> >
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_t...
> > File recipe_engine/recipe_test_api.py (right):
> > 
> >
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_t...
> > recipe_engine/recipe_test_api.py:163: self.depend_on_data = {}
> > On 2015/11/16 at 20:48:00, luqui wrote:
> > > This is a little worrisome to me.  Why isn't depend_on just a special kind
of
> > step?
> > 
> > Because it's not a step? I have to mock the output based on the key of
(name,
> > properties). It seems strange to stick that into a string as the step name.
> > 
> >
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/recipe_t...
> > recipe_engine/recipe_test_api.py:217: 'depend_on_data': self.depend_on_data
> > On 2015/11/16 at 20:48:00, luqui wrote:
> > > ubernit: ultimate comma
> > 
> > Done.
> > 
> > https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py
> > File recipe_engine/run.py (right):
> > 
> >
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py#n...
> > recipe_engine/run.py:768: self._test_data, universe or self._universe)
> > On 2015/11/16 at 20:48:00, luqui wrote:
> > > Hmm, isn't test_data mutable, in order for example for the 'unconsumed
test
> > data' check?
> > 
> > Deleted this method. Not needed anymore.
> > 
> >
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py#n...
> > recipe_engine/run.py:974: '--result-file=%s' % f.name, recipe]
> > On 2015/11/16 at 20:48:00, luqui wrote:
> > > The convention for this argument has been --output-json AFAICT.   I did
like
> > "result" there, though.  Maybe --output-result-json
> > 
> > Fixed.
> > 
> >
https://codereview.chromium.org/1421843006/diff/100001/recipe_engine/run.py#n...
> > recipe_engine/run.py:977: rval = subprocess.call(cmd)
> > On 2015/11/17 at 23:07:16, estaab wrote:
> > > We should probably throw an exception when rval isn't 0 since we control
the
> > process running the recipe and should always expect it to be 0, right?
> > 
> > Good idea. Raising an InfraFailure.
> > 
> > https://codereview.chromium.org/1421843006/diff/100001/recipes/bottom.py
> > File recipes/bottom.py (right):
> > 
> >
https://codereview.chromium.org/1421843006/diff/100001/recipes/bottom.py#newc...
> > recipes/bottom.py:1: from recipe_engine.recipe_api import Property
> > On 2015/11/16 at 20:48:00, luqui wrote:
> > > Seems like these should be in recipes/engine_tests with the others.
> > > 
> > > recipes/ has recipes which are actually intended to be run for a useful
> > purpose (which in recipes-py is a bit silly since there will likely never be
> > any, but you get the idea anyway?)
> > 
> > Sure, that's fine. Moved.
> 
> Question, don't let it hold up committing this if others give an LG though.
How will these sequential child recipes show up in annotations and in the
buildbot build view?

I'm not sure, but I believe that's basically annotee/logdog's job to handle
displaying it. I'll talk to dnj and make sure he'll be able to parse the output
as is right now. 

Here's the output of running 
'./recipes.py --package infra/config/recipes.cfg run engine_tests/top to_pass=3'
https://x20web.corp.google.com/users/ma/martiniss/recipe_out

[estaab, 2015-11-18 17:24:29]

lgtm

I think we'll need to annotate the start and end of a child recipe run at some
point. This will be easy to change, though.

[martiniss, 2015-11-18 22:03:30]

PTAL

erik re-look at this patch. Made some changes to the architecture to always
check properties. It'll give a nicer error message now for any case.

https://codereview.chromium.org/1421843006/diff/120001/recipe_engine/recipe_t...
File recipe_engine/recipe_test_api.py (right):

https://codereview.chromium.org/1421843006/diff/120001/recipe_engine/recipe_t...
recipe_engine/recipe_test_api.py:207: 'Already gave test data for %s' % recipe)
On 2015/11/18 at 16:56:03, Paweł Hajdan Jr. wrote:
> Shouldn't this print the entire tuple in error message, not just the recipe
part?

Good catch. Fixed.

https://codereview.chromium.org/1421843006/diff/120001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/1421843006/diff/120001/recipe_engine/run.py#n...
recipe_engine/run.py:961: 
On 2015/11/18 at 16:56:03, Paweł Hajdan Jr. wrote:
> nit: Remove redundant empty line.

Fixed.

https://codereview.chromium.org/1421843006/diff/120001/recipe_engine/run.py#n...
recipe_engine/run.py:976: raise recipe_api.InfraFailure(
On 2015/11/18 at 16:56:03, Paweł Hajdan Jr. wrote:
> This isn't necessarily an infra failure, right? Dependent recipes can fail for
legitimate non-infra reasons. Or is such a failure reported in some other way?
In that case please clarify in a comment.

True. I changed this to be a step failure. 

This recipe could return 1 if the actual recipe failed; compile failed because
the patch was bad, for example.

[Paweł Hajdan Jr., 2015-11-19 15:12:23]

LGTM w/nit

https://codereview.chromium.org/1421843006/diff/140001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/1421843006/diff/140001/recipe_engine/run.py#n...
recipe_engine/run.py:962: 
nit: Remove redundant empty line.

[estaab, 2015-11-19 23:08:18]

estaab@chromium.org changed required reviewers:
+ iannucci@chromium.org

[estaab, 2015-11-19 23:08:19]

lgtm

Robbie should look at this and give the final lgtm.

https://codereview.chromium.org/1421843006/diff/140001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/1421843006/diff/140001/recipe_engine/run.py#n...
recipe_engine/run.py:963: func = lambda *args, **kwargs: (
maybe a more descriptive name, like run_recipe or something?

[martiniss, 2015-11-20 00:08:51]

Robbie and luke can you take a look? 

Had to fiddle with a couple recipes.py functions, and the threshold.

https://codereview.chromium.org/1421843006/diff/140001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/1421843006/diff/140001/recipe_engine/run.py#n...
recipe_engine/run.py:962: 
On 2015/11/19 at 15:12:23, Paweł Hajdan Jr. wrote:
> nit: Remove redundant empty line.

Fixed.

https://codereview.chromium.org/1421843006/diff/140001/recipe_engine/run.py#n...
recipe_engine/run.py:963: func = lambda *args, **kwargs: (
On 2015/11/19 at 23:08:19, estaab wrote:
> maybe a more descriptive name, like run_recipe or something?

run_recipe it is!

[luqui, 2015-11-20 01:07:02]

Structually this lgtm.  A few concerns still.

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/recipe_a...
File recipe_engine/recipe_api.py (right):

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/recipe_a...
recipe_engine/recipe_api.py:457: def depend_on(self, recipe, properties,
**kwargs):
It just occurred to me -- what if a module wants to depend on a recipe?  Should
this be part of RecipeApiPlain instead?

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/recipe_a...
recipe_engine/recipe_api.py:460: def depend_on_test(self, recipe, properties,
value, **kwargs):
Unused function?

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/unittest...
File recipe_engine/unittests/loader_test.py (right):

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/unittest...
recipe_engine/unittests/loader_test.py:131: loader._invoke_with_properties =
mock_inner_invoke
Doesn't mock provide a cleaner way of doing things like this?  I'm not that
familiar, I just have a feeling.  (mock is vendored in already)

[iannucci, 2015-11-20 02:00:19]

lgtm for sure!

This will be a great base to build on.

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/recipe_a...
File recipe_engine/recipe_api.py (right):

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/recipe_a...
recipe_engine/recipe_api.py:457: def depend_on(self, recipe, properties,
**kwargs):
On 2015/11/20 at 01:07:02, luqui wrote:
> It just occurred to me -- what if a module wants to depend on a recipe? 
Should this be part of RecipeApiPlain instead?

Yes, we should definitely think about this use case

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/recipe_t...
File recipe_engine/recipe_test_api.py (right):

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/recipe_t...
recipe_engine/recipe_test_api.py:204: def depend_on(self, recipe, properties,
result):
can we check that result conforms to the RETURN_SCHEMA of recipe?

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/recipe_t...
recipe_engine/recipe_test_api.py:208: self.depend_on_data[tup] = freeze(result)
Is there a way to distinguish the case where we re-try the same job
(recipe+properties) intentionally? e.g. when we retry to detect potential flake?

What about depending on multiple runs of the same job in parallel (preemptive
flake detection)?

Or will these flows require the 'advanced' API and are out of scope for this CL?

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/run.py#n...
recipe_engine/run.py:954: recipe_script = self._universe.load_recipe(recipe)
probably also something for the 'advanced' API, but not all dependencies will
happen within the same version of the same toolset. For example, when bisecting,
we'll want to chose the toolset that corresponds with the current version of the
code that we're operating on.

I think in this case we'd require the depender to supply the swarming dimensions
instead of calculating them automatically, too.

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/run.py#n...
recipe_engine/run.py:967: # For now, we just run the recipe locally.
assert distributor == None?

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/run.py#n...
recipe_engine/run.py:986: # This does type checking for properties
presumably distributor calculation would happen here?

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/unittest...
File recipe_engine/unittests/loader_test.py (right):

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/unittest...
recipe_engine/unittests/loader_test.py:131: loader._invoke_with_properties =
mock_inner_invoke
On 2015/11/20 at 01:07:02, luqui wrote:
> Doesn't mock provide a cleaner way of doing things like this?  I'm not that
familiar, I just have a feeling.  (mock is vendored in already)

(yes it does)

[martiniss, 2015-11-23 22:05:08]

More tests!

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/recipe_a...
File recipe_engine/recipe_api.py (right):

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/recipe_a...
recipe_engine/recipe_api.py:457: def depend_on(self, recipe, properties,
**kwargs):
On 2015/11/20 at 02:00:18, iannucci wrote:
> On 2015/11/20 at 01:07:02, luqui wrote:
> > It just occurred to me -- what if a module wants to depend on a recipe? 
Should this be part of RecipeApiPlain instead?
> 
> Yes, we should definitely think about this use case

I don't want to have this be a feature for now. If we really want this in the
future, we can easily move the code.

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/recipe_a...
recipe_engine/recipe_api.py:460: def depend_on_test(self, recipe, properties,
value, **kwargs):
On 2015/11/20 at 01:07:02, luqui wrote:
> Unused function?

Yes, whoops. Good catch.

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/recipe_t...
File recipe_engine/recipe_test_api.py (right):

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/recipe_t...
recipe_engine/recipe_test_api.py:204: def depend_on(self, recipe, properties,
result):
On 2015/11/20 at 02:00:19, iannucci wrote:
> can we check that result conforms to the RETURN_SCHEMA of recipe?

Yes. Did this in run.py since this file doesn't know how to load the recipe.

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/recipe_t...
recipe_engine/recipe_test_api.py:208: self.depend_on_data[tup] = freeze(result)
On 2015/11/20 at 02:00:19, iannucci wrote:
> Is there a way to distinguish the case where we re-try the same job
(recipe+properties) intentionally? e.g. when we retry to detect potential flake?
> 
> What about depending on multiple runs of the same job in parallel (preemptive
flake detection)?
> 
> Or will these flows require the 'advanced' API and are out of scope for this
CL?

Retrying the same job may partially be supported by this API. It will be much
more fully supported by the more advanced API.

One thing we could do is to de-dup the recipe "names" in a similar way that we
de-duplicate step names. If we see a recipe twice, then we just add a " (1)" to
the end of it. Not totally sure if that will work, but I'll think about it more
carefully in the future.

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/run.py#n...
recipe_engine/run.py:954: recipe_script = self._universe.load_recipe(recipe)
On 2015/11/20 at 02:00:19, iannucci wrote:
> probably also something for the 'advanced' API, but not all dependencies will
happen within the same version of the same toolset. For example, when bisecting,
we'll want to chose the toolset that corresponds with the current version of the
code that we're operating on.
> 
> I think in this case we'd require the depender to supply the swarming
dimensions instead of calculating them automatically, too.

True. Will require additional thinking. But we can just change the universe we
load from. Might also stick this inside of the distributor code, wherever that
lives.

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/run.py#n...
recipe_engine/run.py:967: # For now, we just run the recipe locally.
On 2015/11/20 at 02:00:19, iannucci wrote:
> assert distributor == None?

Good idea.

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/run.py#n...
recipe_engine/run.py:986: # This does type checking for properties
On 2015/11/20 at 02:00:19, iannucci wrote:
> presumably distributor calculation would happen here?

What do you mean calculation? 

I believe the distributor would just define a recipe_run essentially. So maybe
return a function or something. Which I would then pass in here, which would get
called after the properties being passed to the recipe are validated by the
engine.

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/unittest...
File recipe_engine/unittests/loader_test.py (right):

https://codereview.chromium.org/1421843006/diff/160001/recipe_engine/unittest...
recipe_engine/unittests/loader_test.py:131: loader._invoke_with_properties =
mock_inner_invoke
On 2015/11/20 at 02:00:19, iannucci wrote:
> On 2015/11/20 at 01:07:02, luqui wrote:
> > Doesn't mock provide a cleaner way of doing things like this?  I'm not that
familiar, I just have a feeling.  (mock is vendored in already)
> 
> (yes it does)

much wow.

Had to fix it, since we don't actually use it anywhere :P

[martiniss, 2015-11-23 23:33:28]

Ok, will commit at 4 PM PST. Speak now or forever hold your peace!

[martiniss, 2015-11-24 00:00:01]

Description was changed from

==========
Add simple depends_on API.

BUG=
==========

to

==========
Add simple depends_on API.

BUG=550688
==========

[martiniss, 2015-11-24 00:00:07]

The CQ bit was checked by martiniss@chromium.org

[martiniss, 2015-11-24 00:00:07]

The patchset sent to the CQ was uploaded after l-g-t-m from estaab@chromium.org,
phajdan.jr@chromium.org, luqui@chromium.org, iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/1421843006/#ps220001
(title: "Remove old expectation, move tests to their own folder.")

[commit-bot: I haz the power, 2015-11-24 00:00:12]

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

[commit-bot: I haz the power, 2015-11-24 00:01:25]

Committed patchset #12 (id:220001) as
https://github.com/luci/recipes-py/commit/68b89f6aaec205975d9a741f64cbfa50a9e...