[recipe_engine] Fix a couple path-related bugs.

[recipe_engine] Fix a couple path-related bugs.

Previously if a recipe never ended up importing the 'recipe_engine/path' module,
attempts to use any Path object would crash. The resason for this is that the
path module and the engine are convolutedly interconnected by the to_string_fn
in config_types. The to_string_fn essentially must have a reference to the
PathApi object, and so the set_tostring_fn is called from PathApi's __init__
method. If the recipe never ends up loading this module transitively, but then
acquires a Path object (e.g. via api.resource()), attempting to stringify the
Path will crash the recipe. This poor api boundary needs to be fixed, but the
refactor looked pretty severe when I attempted it, so we paper over the crash
for now by essentially always loading the path module when creating the api for
the recipe. If the path module is already in the recipe's DEPS, the additional
load is a no-op. This extra module does not get attached to the recipe's api
object, so for the recipe to actually utilize the PathApi module, it still has
to import it via DEPS.

Also fix the appearance of simulated resource paths to be more in line with
the existing RECIPE_MODULE notation:
  RECIPE[repo_name/recipe_name].resources/ becomes
  RECIPE[repo_name::recipe_name].resources/
And
  RECIPE[repo_name/../recipe_modules/module_name/example].resource/ becomes
  RECIPE[repo_name::module_name:example].resources/

Lastly, add a small optimization to Path.join when the arguments are
empty.

R=martiniss@chromium.org, phajdan.jr@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2669033007
Committed: https://github.com/luci/recipes-py/commit/37a1da981d744246d14a2c2e6fac554703ea4b1b

[martiniss, 2017-02-02 21:12:18]

lgtm

https://codereview.chromium.org/2669033007/diff/20001/recipe_engine/loader.py
File recipe_engine/loader.py (right):

https://codereview.chromium.org/2669033007/diff/20001/recipe_engine/loader.py...
recipe_engine/loader.py:586: # Provide a fake module to the ScriptApi so that
recipes can use:
should this comment be moved? It doesn't seem to apply to the name, that I can
tell.

https://codereview.chromium.org/2669033007/diff/20001/recipe_engine/loader.py...
recipe_engine/loader.py:594: rel_to = toplevel_package.recipe_dirs[0]
I don't like all the [0] we have everywhere with module_dirs and recipe_dirs,
which we should technically iterate over, but never actually do.

[iannucci, 2017-02-02 22:29:13]

https://codereview.chromium.org/2669033007/diff/20001/recipe_engine/loader.py
File recipe_engine/loader.py (right):

https://codereview.chromium.org/2669033007/diff/20001/recipe_engine/loader.py...
recipe_engine/loader.py:586: # Provide a fake module to the ScriptApi so that
recipes can use:
On 2017/02/02 21:12:18, martiniss wrote:
> should this comment be moved? It doesn't seem to apply to the name, that I can
> tell.

no, it still applies; this whole section is in order to construct fakeModule

https://codereview.chromium.org/2669033007/diff/20001/recipe_engine/loader.py...
recipe_engine/loader.py:594: rel_to = toplevel_package.recipe_dirs[0]
On 2017/02/02 21:12:18, martiniss wrote:
> I don't like all the [0] we have everywhere with module_dirs and recipe_dirs,
> which we should technically iterate over, but never actually do.

agree. I'll take a stab at cleaning this up in a subsequent CL.

[iannucci, 2017-02-02 22:29:19]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2017-02-02 22:29:33]

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-02-02 22:33:39]

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

[commit-bot: I haz the power, 2017-02-02 22:33:40]

Try jobs failed on following builders:
  Recipes-py Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3419c41972443810)

[martiniss, 2017-02-02 23:44:56]

This doesn't break any tests, right? It's a bit weird that changing something
from a str to a PathRepoSpec doesn't break anything, but cool

lgtm

https://codereview.chromium.org/2669033007/diff/40001/recipe_engine/package.py
File recipe_engine/package.py (right):

https://codereview.chromium.org/2669033007/diff/40001/recipe_engine/package.p...
recipe_engine/package.py:514: return PathRepoSpec(url[7:])
nit: change this to 

return PathRepoSpec(url[len("file://"):])

[iannucci, 2017-02-02 23:48:37]

On 2017/02/02 23:44:56, martiniss wrote:
> This doesn't break any tests, right? It's a bit weird that changing something
> from a str to a PathRepoSpec doesn't break anything, but cool
> 

The method was returning some derivative of RepoSpec before.

> lgtm
> 
> https://codereview.chromium.org/2669033007/diff/40001/recipe_engine/package.py
> File recipe_engine/package.py (right):
> 
>
https://codereview.chromium.org/2669033007/diff/40001/recipe_engine/package.p...
> recipe_engine/package.py:514: return PathRepoSpec(url[7:])
> nit: change this to 
> 
> return PathRepoSpec(url[len("file://"):])

[iannucci, 2017-02-02 23:58:08]

bleah, one more look pls (I thought all the tests were passing but my shell
betrayed me)

https://codereview.chromium.org/2669033007/diff/40001/recipe_engine/package.py
File recipe_engine/package.py (right):

https://codereview.chromium.org/2669033007/diff/40001/recipe_engine/package.p...
recipe_engine/package.py:514: return PathRepoSpec(url[7:])
On 2017/02/02 23:44:56, martiniss wrote:
> nit: change this to 
> 
> return PathRepoSpec(url[len("file://"):])

Done.

[iannucci, 2017-02-03 00:00:05]

https://codereview.chromium.org/2669033007/diff/60001/recipe_engine/doc.py
File recipe_engine/doc.py (right):

https://codereview.chromium.org/2669033007/diff/60001/recipe_engine/doc.py#ne...
recipe_engine/doc.py:105: None, {}, universe_view))
So... this has always been wrong and just miraculously worked before. The engine
is supposed to take the universe_view, not the universe...

https://codereview.chromium.org/2669033007/diff/60001/recipe_engine/package.py
File recipe_engine/package.py (right):

https://codereview.chromium.org/2669033007/diff/60001/recipe_engine/package.p...
recipe_engine/package.py:335: url="file://"+self.path)
These are used by the autoroller tests.

[martiniss, 2017-02-03 00:01:46]

lgtm

[iannucci, 2017-02-03 00:08:56]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2017-02-03 00:09:04]

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-02-03 00:11:54]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486080536594260,
"parent_rev": "a7a17ae6545a9ff04b71b3655ff4d08a0de32926", "commit_rev":
"37a1da981d744246d14a2c2e6fac554703ea4b1b"}

[commit-bot: I haz the power, 2017-02-03 00:11:57]

Description was changed from

==========
[recipe_engine] Fix a couple path-related bugs.

Previously if a recipe never ended up importing the 'recipe_engine/path' module,
attempts to use any Path object would crash. The resason for this is that the
path module and the engine are convolutedly interconnected by the to_string_fn
in config_types. The to_string_fn essentially must have a reference to the
PathApi object, and so the set_tostring_fn is called from PathApi's __init__
method. If the recipe never ends up loading this module transitively, but then
acquires a Path object (e.g. via api.resource()), attempting to stringify the
Path will crash the recipe. This poor api boundary needs to be fixed, but the
refactor looked pretty severe when I attempted it, so we paper over the crash
for now by essentially always loading the path module when creating the api for
the recipe. If the path module is already in the recipe's DEPS, the additional
load is a no-op. This extra module does not get attached to the recipe's api
object, so for the recipe to actually utilize the PathApi module, it still has
to import it via DEPS.

Also fix the appearance of simulated resource paths to be more in line with
the existing RECIPE_MODULE notation:
  RECIPE[repo_name/recipe_name].resources/ becomes
  RECIPE[repo_name::recipe_name].resources/
And
  RECIPE[repo_name/../recipe_modules/module_name/example].resource/ becomes
  RECIPE[repo_name::module_name:example].resources/

Lastly, add a small optimization to Path.join when the arguments are
empty.

R=martiniss@chromium.org, phajdan.jr@chromium.org
BUG=
==========

to

==========
[recipe_engine] Fix a couple path-related bugs.

Previously if a recipe never ended up importing the 'recipe_engine/path' module,
attempts to use any Path object would crash. The resason for this is that the
path module and the engine are convolutedly interconnected by the to_string_fn
in config_types. The to_string_fn essentially must have a reference to the
PathApi object, and so the set_tostring_fn is called from PathApi's __init__
method. If the recipe never ends up loading this module transitively, but then
acquires a Path object (e.g. via api.resource()), attempting to stringify the
Path will crash the recipe. This poor api boundary needs to be fixed, but the
refactor looked pretty severe when I attempted it, so we paper over the crash
for now by essentially always loading the path module when creating the api for
the recipe. If the path module is already in the recipe's DEPS, the additional
load is a no-op. This extra module does not get attached to the recipe's api
object, so for the recipe to actually utilize the PathApi module, it still has
to import it via DEPS.

Also fix the appearance of simulated resource paths to be more in line with
the existing RECIPE_MODULE notation:
  RECIPE[repo_name/recipe_name].resources/ becomes
  RECIPE[repo_name::recipe_name].resources/
And
  RECIPE[repo_name/../recipe_modules/module_name/example].resource/ becomes
  RECIPE[repo_name::module_name:example].resources/

Lastly, add a small optimization to Path.join when the arguments are
empty.

R=martiniss@chromium.org, phajdan.jr@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2669033007
Committed:
https://github.com/luci/recipes-py/commit/37a1da981d744246d14a2c2e6fac554703e...
==========

[commit-bot: I haz the power, 2017-02-03 00:12:00]

Committed patchset #4 (id:60001) as
https://github.com/luci/recipes-py/commit/37a1da981d744246d14a2c2e6fac554703e...