Minor cleanup of some recipe framework code.

Minor cleanup of some recipe framework code.

Changes:
 * Use single naming convention for functions in recipe_loader.py.
 * Doc strings for some obscure corners.
 * Remove I_AM_A_CONFIG_CTX hack, promote confix_ctx from a function that carries
   properties to a regular object of ConfigContext class that also acts as
   a decorator factory.

R=iannucci@chromium.org
BUG=

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

[Vadim Sh., 2014-03-05 06:09:29]

I started deciphering current recipe engine code to figure out how to plug in a
new engine. That's a first bunch of... lets call them 'most unexpected moments'.
All tests pass with 100% coverage, so I assume it's a noop pure refactoring
change (but you never know..).

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_config.py
File scripts/slave/recipe_config.py (right):

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_config....
scripts/slave/recipe_config.py:321: TEST_NAME_FORMAT: A string format (or
function) for naming tests and test
I removed TEST_FILE_FORMAT. It wasn't used anywhere and wasn't hooked up in this
code either. Dead code.

Also, I have no idea what it was supposed to do :)

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_loader.py
File scripts/slave/recipe_loader.py (left):

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_loader....
scripts/slave/recipe_loader.py:24: if hasattr(v, 'I_AM_A_CONFIG_CTX'):
That was WTF moment...

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_loader.py
File scripts/slave/recipe_loader.py (right):

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_loader....
scripts/slave/recipe_loader.py:19: """Finds framework related classes and
functions in a |submod| and adds
That was unclear for me from the first glance.

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_loader....
scripts/slave/recipe_loader.py:127: def create_api(mod_dirs, names,
test_data=DisabledTestData(), required=None,
I really can't stand mixed code styles in a single file... This stuff isn't
supposed to be used by recipes directly, and engine seems to be using mostly
lowercase_letters_with_underscores.

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_loader....
scripts/slave/recipe_loader.py:127: def create_api(mod_dirs, names,
test_data=DisabledTestData(), required=None,
Also this function is a big WTF for me, I still have very vague idea of how it
works.

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_loader....
scripts/slave/recipe_loader.py:141: required: a pair such as ('API', RecipeApi).
I really tried to come up with a better doc string but couldn't :D Still "a
pair" bit is helpful, since from the first glance it's not obvious (I though it
was a list of required stuff).

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_loader....
scripts/slave/recipe_loader.py:178: # (that doesn't need kwargs).
I dislike this asymmetry...

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/step/api.py (right):

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/step/api.py:10: super(StepApi,
self).__init__(**kwargs)
This seems to be more in line with other modules.

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/step_history/api.py (right):

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/step_history/api.py:21: # step_history is
instantiated in annontated_run.py. Recipe engine is
That was non obvious dependency for me. Also added and assert as a type hint.
OrderedDict is crucial since we rely on order in last_step.

[iannucci, 2014-03-05 20:43:37]

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/slave/recipe_...
File scripts/slave/recipe_config.py (right):

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/slave/recipe_...
scripts/slave/recipe_config.py:321: TEST_NAME_FORMAT: A string format (or
function) for naming tests and test
On 2014/03/05 06:09:30, Vadim Sh. wrote:
> I removed TEST_FILE_FORMAT. It wasn't used anywhere and wasn't hooked up in
this
> code either. Dead code.
> 
> Also, I have no idea what it was supposed to do :)

Yep dead code. We used to (until very recently), permute VAR_TEST_MAP and record
every config item's as_jsonish output to a json file (config expectations).
These expectations were sufficiently abstract and noisy that we decided to toss
them (since the recipe expectations are much more useful, and now that we're
going to the one-test-per (mastername, buildername), much more useful for
grepping as well).

We still use VAR_TEST_MAP to go through all permutations, but now we're just
checking for syntax/semantic errors and code coverage.

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/slave/recipe_...
File scripts/slave/recipe_loader.py (left):

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/slave/recipe_...
scripts/slave/recipe_loader.py:24: if hasattr(v, 'I_AM_A_CONFIG_CTX'):
On 2014/03/05 06:09:30, Vadim Sh. wrote:
> That was WTF moment...

:D You're deep into the Old Code here ^_^.

Looking at the modern config_ctx, it's totally dumb not to have it be a class...
it used to be a lot simpler....

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/slave/recipe_...
File scripts/slave/recipe_loader.py (right):

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/slave/recipe_...
scripts/slave/recipe_loader.py:19: """Finds framework related classes and
functions in a |submod| and adds
On 2014/03/05 06:09:30, Vadim Sh. wrote:
> That was unclear for me from the first glance.

Thanks. This docstring lg.

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/slave/recipe_...
scripts/slave/recipe_loader.py:23: |submod| is a recipe module (akin to python
package) with submodules such as
We should find a standard way to document types. Some type declarations in
docstrings are even supported by tools like YouCompleteMe (which uses jedi:
http://jedi.jedidjah.ch/en/latest/docs/features.html#type-hinting).

For that matter, we should really have a proper doc browser...

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/slave/recipe_...
scripts/slave/recipe_loader.py:127: def create_api(mod_dirs, names,
test_data=DisabledTestData(), required=None,
On 2014/03/05 06:09:30, Vadim Sh. wrote:
> I really can't stand mixed code styles in a single file... This stuff isn't
> supposed to be used by recipes directly, and engine seems to be using mostly
> lowercase_letters_with_underscores.

yep. I think this slipped through a codereview

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/slave/recipe_...
scripts/slave/recipe_loader.py:127: def create_api(mod_dirs, names,
test_data=DisabledTestData(), required=None,
On 2014/03/05 06:09:30, Vadim Sh. wrote:
> Also this function is a big WTF for me, I still have very vague idea of how it
> works.

Basically it creates a RecipeApi instance, with all module dependencies (and
test_api's) correctly injected (along with their dependencies, etc.). 

This would probably be more correctly modeled with a recursive function passing
a global dictionary of {mod_name: mod_instance} down through all the layers. I'm
not sure why I didn't do that.

On that note, we should start adding unit tests for this engine stuff (or at
least for the new engine stuff). machenbach did that for his rewrite of the
generator-unfolding code.

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/slave/recipe_...
scripts/slave/recipe_loader.py:178: # (that doesn't need kwargs).
On 2014/03/05 06:09:30, Vadim Sh. wrote:
> I dislike this asymmetry...

Yeah me too :(. There shouldn't be kwargs at all. Again, carry over from days
past. Basically they're used so they show up in the module's __init__. I think
they're only /actually/ used to pass the properties dictionary, but originally
we thought there would be lots of other inputs to various modules.

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/slave/recipe_...
scripts/slave/recipe_loader.py:228: def cached(f):
maybe this should move to recipe_util.py?

Probably should also be named 'cached_unary'

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/slave/recipe_...
File scripts/slave/recipe_modules/step/api.py (right):

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/slave/recipe_...
scripts/slave/recipe_modules/step/api.py:10: super(StepApi,
self).__init__(**kwargs)
On 2014/03/05 06:09:30, Vadim Sh. wrote:
> This seems to be more in line with other modules. 

Yeah, no reason not to.

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/slave/recipe_...
File scripts/slave/recipe_modules/step_history/api.py (right):

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/slave/recipe_...
scripts/slave/recipe_modules/step_history/api.py:21: # step_history is
instantiated in annontated_run.py. Recipe engine is
On 2014/03/05 06:09:30, Vadim Sh. wrote:
> That was non obvious dependency for me. Also added and assert as a type hint.
> OrderedDict is crucial since we rely on order in last_step.

Oh, yeah, this is the other thing that we pass via kwargs in load_module. Can
you make it a kwarg though, since it's meant to be used by name?

Can't wait to get rid of this module...

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/tools/show_me...
File scripts/tools/show_me_the_modules.py (right):

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/tools/show_me...
scripts/tools/show_me_the_modules.py:88: mocks={'path': {}}, properties={},
step_history=collections.OrderedDict())
good catch.

[Vadim Sh., 2014-03-05 21:40:37]

Also made load_recipe always return object of RecipeScript type (former
ModuleObject). It used to return ModuleObject or <module>.

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_loader.py
File scripts/slave/recipe_loader.py (right):

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_loader....
scripts/slave/recipe_loader.py:23: |submod| is a recipe module (akin to python
package) with submodules such as
On 2014/03/05 20:43:37, iannucci wrote:
> We should find a standard way to document types. Some type declarations in
> docstrings are even supported by tools like YouCompleteMe (which uses jedi:
> http://jedi.jedidjah.ch/en/latest/docs/features.html#type-hinting).
> 
> For that matter, we should really have a proper doc browser...
With current level of magicallity doc browser wouldn't be only partially useful.
For example, I don't think it documents inner functions, like that one.

Also I'm kind of on the fence regarding strict type documentation without a way
to enforce the types. I generally prefer to do asserts if specific subtype
matters.

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_loader....
scripts/slave/recipe_loader.py:178: # (that doesn't need kwargs).
On 2014/03/05 20:43:37, iannucci wrote:
> On 2014/03/05 06:09:30, Vadim Sh. wrote:
> > I dislike this asymmetry...
> 
> Yeah me too :(. There shouldn't be kwargs at all. Again, carry over from days
> past. Basically they're used so they show up in the module's __init__. I think
> they're only /actually/ used to pass the properties dictionary, but originally
> we thought there would be lots of other inputs to various modules.

It's used by properties and step_history modules. I was considering to use it to
pass 'engine' instance that implements new recipe engine. I'd like to discuss
this offline.

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_loader....
scripts/slave/recipe_loader.py:228: def cached(f):
On 2014/03/05 20:43:37, iannucci wrote:
> maybe this should move to recipe_util.py?
> 
> Probably should also be named 'cached_unary'

Done.

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/step_history/api.py (right):

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/step_history/api.py:21: # step_history is
instantiated in annontated_run.py. Recipe engine is
On 2014/03/05 20:43:37, iannucci wrote:
> On 2014/03/05 06:09:30, Vadim Sh. wrote:
> > That was non obvious dependency for me. Also added and assert as a type
hint.
> > OrderedDict is crucial since we rely on order in last_step.
> 
> Oh, yeah, this is the other thing that we pass via kwargs in load_module. Can
> you make it a kwarg though, since it's meant to be used by name?

What do you mean? kwargs.get('step_history')? Than same should be done in
'properties' module. Not sure it improves readability.

[Vadim Sh., 2014-03-05 21:40:37]

Also made load_recipe always return object of RecipeScript type (former
ModuleObject). It used to return ModuleObject or <module>.

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_loader.py
File scripts/slave/recipe_loader.py (right):

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_loader....
scripts/slave/recipe_loader.py:23: |submod| is a recipe module (akin to python
package) with submodules such as
On 2014/03/05 20:43:37, iannucci wrote:
> We should find a standard way to document types. Some type declarations in
> docstrings are even supported by tools like YouCompleteMe (which uses jedi:
> http://jedi.jedidjah.ch/en/latest/docs/features.html#type-hinting).
> 
> For that matter, we should really have a proper doc browser...
With current level of magicallity doc browser wouldn't be only partially useful.
For example, I don't think it documents inner functions, like that one.

Also I'm kind of on the fence regarding strict type documentation without a way
to enforce the types. I generally prefer to do asserts if specific subtype
matters.

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_loader....
scripts/slave/recipe_loader.py:178: # (that doesn't need kwargs).
On 2014/03/05 20:43:37, iannucci wrote:
> On 2014/03/05 06:09:30, Vadim Sh. wrote:
> > I dislike this asymmetry...
> 
> Yeah me too :(. There shouldn't be kwargs at all. Again, carry over from days
> past. Basically they're used so they show up in the module's __init__. I think
> they're only /actually/ used to pass the properties dictionary, but originally
> we thought there would be lots of other inputs to various modules.

It's used by properties and step_history modules. I was considering to use it to
pass 'engine' instance that implements new recipe engine. I'd like to discuss
this offline.

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_loader....
scripts/slave/recipe_loader.py:228: def cached(f):
On 2014/03/05 20:43:37, iannucci wrote:
> maybe this should move to recipe_util.py?
> 
> Probably should also be named 'cached_unary'

Done.

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/step_history/api.py (right):

https://codereview.chromium.org/187203005/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/step_history/api.py:21: # step_history is
instantiated in annontated_run.py. Recipe engine is
On 2014/03/05 20:43:37, iannucci wrote:
> On 2014/03/05 06:09:30, Vadim Sh. wrote:
> > That was non obvious dependency for me. Also added and assert as a type
hint.
> > OrderedDict is crucial since we rely on order in last_step.
> 
> Oh, yeah, this is the other thing that we pass via kwargs in load_module. Can
> you make it a kwarg though, since it's meant to be used by name?

What do you mean? kwargs.get('step_history')? Than same should be done in
'properties' module. Not sure it improves readability.

[iannucci, 2014-03-05 21:58:29]

lgtm, thanks for digging through this stuff :)

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/slave/recipe_...
File scripts/slave/recipe_loader.py (right):

https://chromiumcodereview.appspot.com/187203005/diff/1/scripts/slave/recipe_...
scripts/slave/recipe_loader.py:23: |submod| is a recipe module (akin to python
package) with submodules such as
On 2014/03/05 21:40:38, Vadim Sh. wrote:
> On 2014/03/05 20:43:37, iannucci wrote:
> > We should find a standard way to document types. Some type declarations in
> > docstrings are even supported by tools like YouCompleteMe (which uses jedi:
> > http://jedi.jedidjah.ch/en/latest/docs/features.html#type-hinting).
> > 
> > For that matter, we should really have a proper doc browser...
> With current level of magicallity doc browser wouldn't be only partially
useful.
> For example, I don't think it documents inner functions, like that one.
> 

Maybe. I think it would be more useful than you might think. I'd especially like
a way to write markdown and then insert links/snapshots of
sourcecode/docstrings.

> Also I'm kind of on the fence regarding strict type documentation without a
way
> to enforce the types. I generally prefer to do asserts if specific subtype
> matters.

Fair enough. YCM can also read `assert isinstance(x, type)` :)

[Vadim Sh., 2014-03-06 01:08:55]

The CQ bit was checked by vadimsh@chromium.org

[commit-bot: I haz the power, 2014-03-06 01:09:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vadimsh@chromium.org/187203005/40001

[commit-bot: I haz the power, 2014-03-06 01:11:20]

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

[commit-bot: I haz the power, 2014-03-06 01:11:21]

Presubmit check for 187203005-40001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
tests/masters_test.py (51.18s) failed
2 masters failed:
master.chromiumos
master.client.dynamorio
51 masters succeeded, 2 failed, 0 skipped in 51.1s.

=== Error running master.chromiumos === 
Didn't find open port for master.chromiumos

=== Error running master.client.dynamorio === 
2014-03-05 17:09:50-0800 [-] adding IStatusReceiver <WebStatus on port tcp:8259
at 0x2dd2cb0>
2014-03-05 17:09:50-0800 [-] buildbot.status.web.baseweb.RotateLogSite starting
on 8259
2014-03-05 17:09:50-0800 [-] Starting factory
<buildbot.status.web.baseweb.RotateLogSite instance at 0x2b697c02c4d0>
2014-03-05 17:09:50-0800 [-] Setting up http.log rotating 10 files of 1000000
bytes each
2014-03-05 17:09:50-0800 [-] WebStatus using
(/b/commit-queue/workdir/tools/build/masters/master.client.dynamorio/public_html)
2014-03-05 17:09:50-0800 [-] adding IStatusReceiver
<buildbot.status.mail.MailNotifier instance at 0x2dd2ea8>
2014-03-05 17:09:50-0800 [-] removing 0 old schedulers, updating 0, and adding 3
2014-03-05 17:09:54-0800 [-] periodic_stable: missed scheduled build time, so
building immediately
2014-03-05 17:09:55-0800 [-] added buildset 1 to database
2014-03-05 17:09:55-0800 [-] Unhandled Error
	Traceback (most recent call last):
	Failure: twisted.internet.defer.FirstError: FirstError[#1, [Failure instance:
Traceback: <class 'sqlalchemy.exc.OperationalError'>: (OperationalError)
database is locked u'INSERT INTO schedulers (name, state, class_name) VALUES (?,
?, ?)' ('all', '{}', 'buildbot.schedulers.basic.Scheduler')
	/usr/lib/python2.7/threading.py:524:__bootstrap
	/usr/lib/python2.7/threading.py:551:__bootstrap_inner
	/usr/lib/python2.7/threading.py:504:run
	--- <exception caught here> ---
	/b/commit-queue/workdir/tools/build/third_party/twisted_10_2/twisted/python/threadpool.py:207:_worker
	/b/commit-queue/workdir/tools/build/third_party/twisted_10_2/twisted/python/context.py:59:callWithContext
	/b/commit-queue/workdir/tools/build/third_party/twisted_10_2/twisted/python/context.py:37:callWithContext
	/b/commit-queue/workdir/tools/build/third_party/buildbot_8_4p1/buildbot/db/pool.py:112:thd
	/b/commit-queue/workdir/tools/build/third_party/buildbot_8_4p1/buildbot/db/schedulers.py:165:thd
	/b/commit-queue/workdir/tools/build/third_party/sqlalchemy_0_7_1/sqlalchemy/engine/base.py:1358:execute
	/b/commit-queue/workdir/tools/build/third_party/sqlalchemy_0_7_1/sqlalchemy/engine/base.py:1491:_execute_clauseelement
	/b/commit-queue/workdir/tools/build/third_party/sqlalchemy_0_7_1/sqlalchemy/engine/base.py:1599:_execute_context
	/b/commit-queue/workdir/tools/build/third_party/sqlalchemy_0_7_1/sqlalchemy/engine/base.py:1592:_execute_context
	/b/commit-queue/workdir/tools/build/third_party/sqlalchemy_0_7_1/sqlalchemy/engine/default.py:325:do_execute
	]]



Presubmit checks took 110.4s to calculate.

[Vadim Sh., 2014-03-06 01:12:28]

The CQ bit was checked by vadimsh@chromium.org

[commit-bot: I haz the power, 2014-03-06 01:12:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vadimsh@chromium.org/187203005/40001

[commit-bot: I haz the power, 2014-03-06 01:13:59]

Change committed as 255195