Allow module self-reference in loader

Allow module self-reference in loader

See https://chromium-review.googlesource.com/c/426658/ for example where it's useful.

BUG=679756

[Paweł Hajdan Jr., 2017-01-10 12:43:46]

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

[commit-bot: I haz the power, 2017-01-10 12:43:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 12:47:20]

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

[commit-bot: I haz the power, 2017-01-10 12:47:21]

Dry run: This issue passed the CQ dry run.

[Paweł Hajdan Jr., 2017-01-10 12:51:48]

Description was changed from

==========
Allow module self-reference in loader

BUG=none
==========

to

==========
Allow module self-reference in loader

See https://chromium-review.googlesource.com/c/426658/ for example where it's
useful.

BUG=none
==========

[Paweł Hajdan Jr., 2017-01-10 15:51:36]

Description was changed from

==========
Allow module self-reference in loader

See https://chromium-review.googlesource.com/c/426658/ for example where it's
useful.

BUG=none
==========

to

==========
Allow module self-reference in loader

See https://chromium-review.googlesource.com/c/426658/ for example where it's
useful.

BUG=679756
==========

[Paweł Hajdan Jr., 2017-01-10 15:52:52]

phajdan.jr@chromium.org changed reviewers:
+ dpranke@chromium.org, estaab@chromium.org, iannucci@chromium.org

[Paweł Hajdan Jr., 2017-01-10 15:55:13]

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

[commit-bot: I haz the power, 2017-01-10 15:55:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 15:58:30]

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

[commit-bot: I haz the power, 2017-01-10 15:58:31]

Dry run: This issue passed the CQ dry run.

[iannucci, 2017-01-10 19:03:24]

On 2017/01/10 at 15:52:52, phajdan.jr wrote:
> 

This seems unnecessary... why can't you pass self instead of creating a
reference cycle on every module?

[Paweł Hajdan Jr., 2017-01-10 19:14:19]

On 2017/01/10 19:03:24, iannucci wrote:
> This seems unnecessary... why can't you pass self instead of creating a
> reference cycle on every module?

Passing self.m makes usage the same regardless of module the callee wants to
access.

If we pass self, the callee needs to use the arg as-is for chromium_tests, and
arg.m.foo to access a module foo different from chromium_tests.

To some degree this is a pick-your-poison type of thing, with the api-passing
ugliness introduced in https://codereview.chromium.org/339183013 (I had an
in-person discussion with Aaron about it). Back then we didn't have
chromium_tests yet IIRC, and test_utils were supposed to be generic so we were
reluctant to pull chromium-specific deps there.

Alternative solutions are welcome of course. Let me know what would be your
suggestion - and ideally make sure it's feasible.

[iannucci, 2017-01-10 19:15:35]

On 2017/01/10 at 19:03:24, iannucci wrote:
> On 2017/01/10 at 15:52:52, phajdan.jr wrote:
> > 
> 
> This seems unnecessary... why can't you pass self instead of creating a
reference cycle on every module?

Ah, nevermind, I see the difference.

However the CL you pointed to looks wrong to me; I don't think the pattern of
passing the whole api object around is a good one at all. The purpose of
declaring dependencies in the module definition was so that the dependencies of
everything in recipes would be clear (or more-clear). The .m pattern itself
wasn't a great design choice itself. The code in question obscures the
dependencies in a not-so-good way (i.e. everything gets the same api blob which
is the union of all of the deps for every implementation). 

I think that adding this to the recipe engine loader would legitimize and
further encourage this pattern, which I don't think is a good idea. I realize
that you're refactoring the chromium recipes to make them simpler and easier to
work with, and as a transitionary measure, manually injecting self into the .m
collection (and probably JUST for functions that interact with Test objects like
serialized_test_runner()) there is ok. I don't think any other modules besides
the chromium ones exhibit this pattern though, so I don't think adding it to the
core engine is a good idea.