Make recipe module loader non-recursive

Make recipe module loader non-recursive

Avoid importing python files other than api.py, test_api.py, and *config.py,
to reduce chance of python path manipulation e.g. by resource scripts.

BUG=none

[Paweł Hajdan Jr., 2017-03-09 14:20:45]

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

[commit-bot: I haz the power, 2017-03-09 14:20:55]

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-03-09 14:24:05]

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

[commit-bot: I haz the power, 2017-03-09 14:24:06]

Dry run: This issue passed the CQ dry run.

[Paweł Hajdan Jr., 2017-03-09 14:27:06]

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

[Paweł Hajdan Jr., 2017-03-09 14:27:06]

Please review.

If this looks good but you have suggestions for further improvements, I'd prefer
to land this patch and continue working on this in a separate CL.

[iannucci, 2017-03-09 17:48:53]

I think that this is the right change to the engine, but before we land it we
need to remove all the __init__.py files and make a list of what will be
affected by it. Landing it first will put us into a broken state :(

[iannucci, 2017-03-09 17:49:49]

On 2017/03/09 17:48:53, iannucci wrote:
> I think that this is the right change to the engine, but before we land it we
> need to remove all the __init__.py files and make a list of what will be
> affected by it. Landing it first will put us into a broken state :(

* all the surprising __init__.py files (like the ones in resources) and make a
list of what else will be affected by ceasing to load the modules recursively.

I'll see if I can come up with a list today.

[Paweł Hajdan Jr., 2017-03-09 21:49:06]

If the change looks good, why not approve it?

I believe both landing the CL and tackling the __init__.py files have the same
issue: they can't be easily tested beforehand. Trying to find __init__.py files
first might look like we're taking some steps to avoid issues, but IMO
https://chromium-review.googlesource.com/c/451882/ getting reverted and
corresponding https://bugs.chromium.org/p/chromium/issues/detail?id=699969
indicate even that is not safe.

I'd obviously address any breakage detected after landing this CL. For chromium
CQ we can do an additional check with whitespace CL.

[iannucci1, 2017-03-09 22:08:23]

On 2017/03/09 21:49:06, Paweł Hajdan Jr. wrote:
> If the change looks good, why not approve it?
> 
> I believe both landing the CL and tackling the __init__.py files have the same
> issue: they can't be easily tested beforehand. Trying to find __init__.py
files
> first might look like we're taking some steps to avoid issues, but IMO
> https://chromium-review.googlesource.com/c/451882/ getting reverted and
> corresponding https://bugs.chromium.org/p/chromium/issues/detail?id=699969
> indicate even that is not safe.

Which is even more reason to be careful and root out the affected areas one at a
time 
before landing the behavioral change which causes them to start breaking.

> 
> I'd obviously address any breakage detected after landing this CL. For
chromium
> CQ we can do an additional check with whitespace CL.

That's not obvious: the changes will be hard to detect when they get auto-rolled
out 
to the whole fleet and start breaking in places we're not watching. Each of
these bad
cases should be found and addressed (or at least as much as we can, and the 
potentially affected owners notified).

There's no benefit from landing the breaking change first, I don't think.

[Dirk Pranke, 2017-03-09 22:13:03]

I don't really know this code and hence don't understand the full ramifications
of this change.

Does this affect something like 

https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/test_util...


since that's not one of the approved names? Or does that go through regular
python importing somehow?

[iannucci, 2017-03-09 23:01:10]

On 2017/03/09 22:13:03, Dirk Pranke wrote:
> I don't really know this code and hence don't understand the full
ramifications
> of this change.
> 
> Does this affect something like 
> 
>
https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/test_util...
> 
> 
> since that's not one of the approved names? Or does that go through regular
> python importing somehow?

It would affect something like

  path/to/recipe_modules/module/subdir/__init__.py
  path/to/recipe_modules/module/subdir/script.py
  path/to/recipe_modules/module/subdir/script_unittest.py

Right now when depending on 'module', the loader would see __init__.py and then
import everything in subdir (script.py and script_unittest.py). There is at
least one case where the unittest modifies sys.path and PYTHONPATH during import
time:
https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/test_resu...
.

Some scripts in other parts of the recipe codebase take implicit dependency on
this modification to PYTHONPATH. Additionally, this sys.path manipulation breaks
when not running under buildbot (e.g. when running the script in swarming or a
simple clone of build without the other surrounding repos), which causes the
whole recipe import process to fail if the unittest can't run
common.env.Install().

Because of the implicit dependencies, some scripts today (e.g. crbug.com/699969)
are 'working' by virtue of the fact that the unittest is tweaking PYTHONPATH
when it's taken as a dependency. If we land this loader change right now, these
implicit dependencies will break and builds will start failing because of it.

Note that this change IS A GOOD CHANGE! It would have prevented this mess if we
never did the recursive import thing in the first place. We should definitely
land it, but we need a plan so that landing it is safe (or at least as safe as
we can reasonably make it).