Add Paths as first-class types in configs.

Add Paths as first-class types in configs.

recipe_config_types contains 'data' classes with a flexible representation. Right
now Path is the only one, but there's room for more, if we need them.

During configuration tests, RecipeConfigType objects use their default_handler
to string-ify themselves. During recipe execution, modules can replace the
default_handler with their own implementation, allowing run-time information
to be blended into the static configuration data (as well as mock'd for testing).

Additional changes:
  * Path objects are infinitely concatenateable via the __call__ operator.
    Previously only the special path methods on api.path had this behaviour. Now
    doing __call__ gets you another Path object with the same base, but with more
    path pieces. The object is not stringified until the last possible second.

  * All paths are now part of the path module's configuration. No more hard-coded
    weirdness in path/api.py. This also means that more complex applications
    could extend this configuration to provide more base paths.

  * Checkout is no longer special. It is simply a 'dynamic_path'. Dynamic
    paths are ones whose value must be set at runtime (dependent on user-code),
    but have a well-known name (as opposed to base paths which can be set up at
    module initialization time). All of the checkout related methods have been
    replaced with set_dynamic_path().  A dynamic path must be set to a Path()
    object which is rooted on a non-dynamic path. Since dynamic paths are
    late-binding, it's perfectly possible to store references or sub paths of
    dynamic paths before those paths are actually defined. See _internal_dir in
    the chromium_android module for an example.

R=agable@chromium.org, sivachandra@chromium.org
BUG=

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

[agable, 2013-09-26 21:46:01]

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

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_api.py#n...
scripts/slave/recipe_api.py:47: def make_config_params(self, config_name,
optional=False, **CONFIG_VARS):
Why did these lose their default value of config_name=None? They both still
special-case the value of None.

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_config_t...
File scripts/slave/recipe_config_types.py (right):

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_config_t...
scripts/slave/recipe_config_types.py:13: class RecipeConfigType(object):
docstrings docstrings docstrings.

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_config_t...
scripts/slave/recipe_config_types.py:26: def default_handler(self, *args,
**kwargs):
default_tostring maybe? Not clear what it means to "handle" an object.

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_config_t...
scripts/slave/recipe_config_types.py:34: BASE_RE = re.compile("[a-z][a-z_]*")
This RE seems incredibly restrictive. Also, (due to lack of docstrings) it's not
clear just from reading this that self.base is an alias that will get expanded
to a real base path at the last second.

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_config_t...
scripts/slave/recipe_config_types.py:37: super(Path, self).__init__()
Should pass kwargs to init. Otherwise there's no reason to have them.

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_config_t...
scripts/slave/recipe_config_types.py:43: self.wrapper = kwargs.get('wrapper',
False)
Since there's only one wrapper function, and it is "append .bat if we're on
windows", this needs a better name. Either that, or it needs to be a function
that gets called as part of __str__.

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
File scripts/slave/recipe_modules/android/config.py (right):

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/android/config.py:8: from slave.recipe_config_types
import Path
No empty line above this.

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/android/config.py:24: chromium_in_android_subpath =
Static('/'.join(chromium_in_android_subpath)),
This is super weird and makes me a little uncomfortable. Not entirely sure why
-- or what else there is to do about it since they explicitly require a subpath
-- but it's weird nonetheless. I'll think about it some more.

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

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/chromium/api.py:32: '--src-dir',
self.m.path.checkout,
Did you find/replace for all instances of {m.path|api.path}.[path_method]() to
remove the empty parentheses? Just want to make sure they're really all gone.

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
File scripts/slave/recipe_modules/chromium/config.py (right):

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/chromium/config.py:8: from
slave.recipe_config_types import Path
No empty line above this.

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/chromium/config.py:38: GYP_DEFINES = Dict(equal_fn,
' '.join, (basestring,int,Path)),
Realizing that these are a bit obtuse and it might be nice to pass both
functions as keyword arguments to reduce potential confusion while reading this
file (and other configs).

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/chromium/config.py:143: c.build_dir =
Path('checkout', 'out')
This just looks so much like it's going to construct "checkout/out", not
"[CHECKOUT]/out". Can we come up with a better way of indicating that the first
element is a late-binding alias?

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
File scripts/slave/recipe_modules/chromium_android/config.py (right):

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/chromium_android/config.py:8: from
slave.recipe_config_types import Path
No empty line above this.

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

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/gclient/api.py:151: 'checkout',
cwd(*cfg.solutions[0].name.split(self.m.path.sep)),
So we've lost the ability to save multiple paths into path.checkout and then
select one later. I guess this is mitigated by the json output from gclient
being in step_history, but double check it hasn't negatively affected anyone.

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
File scripts/slave/recipe_modules/path/api.py (left):

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/path/api.py:199: # TODO(iannucci): Custom paths?
Yayyy.

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

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/path/api.py:115: FILTER_METHODS = ('abspath',
'basename', 'exists', 'join', 'split',
Could still call them "OK_METHODS", though, since there's aren't any arg-taking
methods that we don't filter.

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/path/api.py:130: if ':' in startup_cwd[0]:
if startup_cwd[0].endswith(':'):
Also, are windows drives guaranteed to be single letters? You could test for
len(startup_cwd[0]) == 2 then too.

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/path/api.py:135: assert False, 'What is this, I
don\'t even: %r' % startup_cwd
As much as I approve... error strings should probably be *descriptive*.

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/path/api.py:139: self._startup_cwd = ['/', 'BAG OF
CATS']
'FAKE TESTING CWD'? '------------M-A-G-I-C---D-I-V-I-D-E-R----------'? 'FUNGIBLE
BUNNYMONSTER'?

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/path/api.py:181: if default and
self.c.dynamic_paths.get(pathname):
'default' is insufficiently descriptive. Maybe 'overwrite' and flop the logic.

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
File scripts/slave/recipe_modules/path/config.py (right):

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/path/config.py:4: from slave.recipe_config_types
import Path
No empty line above this.

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/path/config.py:21: ['D:\\', 'build', 'slave',
'fake_slave', 'build'],
It's generally E:\\b\build...

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/path/config.py:39: c.base_paths['root'] =
c.CURRENT_WORKING_DIR[:-4]
Magic number! Comment it.

[agable, 2013-09-26 21:58:08]

+ a couple comments from reading the expectations.

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_config_t...
File scripts/slave/recipe_config_types.py (right):

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipe_config_t...
scripts/slave/recipe_config_types.py:52: return 'Path([%s], %r%s)' %
(self.base.upper(), self.pieces, suffix)
Path([ROOT], piece, piece, piece) looks better than PATH([ROOT], (piece, piece,
piece)).

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipes/android...
File scripts/slave/recipes/android/android_builder.expected/try_builder.json
(right):

https://codereview.chromium.org/24737002/diff/1/scripts/slave/recipes/android...
scripts/slave/recipes/android/android_builder.expected/try_builder.json:145:
"[SLAVE_BUILD_ROOT]/src/repo"
I think this is part of a rebase from siva's work, actually...

[iannucci, 2013-09-27 02:08:19]

PALT

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_a...
File scripts/slave/recipe_api.py (right):

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_a...
scripts/slave/recipe_api.py:47: def make_config_params(self, config_name,
optional=False, **CONFIG_VARS):
On 2013/09/26 21:46:02, Aaron Gable wrote:
> Why did these lose their default value of config_name=None? They both still
> special-case the value of None.

Because it's mostly called from make_config and set_config, I figured having
this be explicit (now that make_config and set_config both symmetrically allow
this to be None).

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_c...
File scripts/slave/recipe_config_types.py (right):

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_c...
scripts/slave/recipe_config_types.py:13: class RecipeConfigType(object):
On 2013/09/26 21:46:02, Aaron Gable wrote:
> docstrings docstrings docstrings.

Done.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_c...
scripts/slave/recipe_config_types.py:26: def default_handler(self, *args,
**kwargs):
On 2013/09/26 21:46:02, Aaron Gable wrote:
> default_tostring maybe? Not clear what it means to "handle" an object.

how about default_tostring_fn . Makes sense to talk about 'tostring_fn's,
talking in the docstrings about 'tostring's or 'tostring functions' is
awkwardish.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_c...
scripts/slave/recipe_config_types.py:34: BASE_RE = re.compile("[a-z][a-z_]*")
On 2013/09/26 21:46:02, Aaron Gable wrote:
> This RE seems incredibly restrictive. Also, (due to lack of docstrings) it's
not
> clear just from reading this that self.base is an alias that will get expanded
> to a real base path at the last second. 

docstrings should clear this up more, added comment to make sure.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_c...
scripts/slave/recipe_config_types.py:37: super(Path, self).__init__()
On 2013/09/26 21:46:02, Aaron Gable wrote:
> Should pass kwargs to init. Otherwise there's no reason to have them.

I would love to have wrapper=False at the end here, but... python 2.7 :/

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_c...
scripts/slave/recipe_config_types.py:43: self.wrapper = kwargs.get('wrapper',
False)
On 2013/09/26 21:46:02, Aaron Gable wrote:
> Since there's only one wrapper function, and it is "append .bat if we're on
> windows", this needs a better name. Either that, or it needs to be a function
> that gets called as part of __str__.

Ok, changed this to a more generic fixed-function 'platform_ext'.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/android/config.py (right):

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/android/config.py:8: from slave.recipe_config_types
import Path
On 2013/09/26 21:46:02, Aaron Gable wrote:
> No empty line above this.

Done.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/android/config.py:24: chromium_in_android_subpath =
Static('/'.join(chromium_in_android_subpath)),
On 2013/09/26 21:46:02, Aaron Gable wrote:
> This is super weird and makes me a little uncomfortable. Not entirely sure why
> -- or what else there is to do about it since they explicitly require a
subpath
> -- but it's weird nonetheless. I'll think about it some more.

Yeah that was pretty much my reaction. I decided to punt for now.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium/api.py (right):

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/api.py:32: '--src-dir',
self.m.path.checkout,
On 2013/09/26 21:46:02, Aaron Gable wrote:
> Did you find/replace for all instances of {m.path|api.path}.[path_method]() to
> remove the empty parentheses? Just want to make sure they're really all gone.

I didn't, just the ones I saw... it actually doesn't end up making a difference
in the new system though. I found and killed the stragglers.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium/config.py (right):

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/config.py:8: from
slave.recipe_config_types import Path
On 2013/09/26 21:46:02, Aaron Gable wrote:
> No empty line above this.

Done.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/config.py:38: GYP_DEFINES = Dict(equal_fn,
' '.join, (basestring,int,Path)),
On 2013/09/26 21:46:02, Aaron Gable wrote:
> Realizing that these are a bit obtuse and it might be nice to pass both
> functions as keyword arguments to reduce potential confusion while reading
this
> file (and other configs).

Yes, I agree :). Another CL?

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/config.py:143: c.build_dir =
Path('checkout', 'out')
On 2013/09/26 21:46:02, Aaron Gable wrote:
> This just looks so much like it's going to construct "checkout/out", not
> "[CHECKOUT]/out". Can we come up with a better way of indicating that the
first
> element is a late-binding alias?

All caps? I could also make the pieces argument explicitly a tuple. I'll do
that. Done.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium_android/config.py (right):

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium_android/config.py:8: from
slave.recipe_config_types import Path
On 2013/09/26 21:46:02, Aaron Gable wrote:
> No empty line above this.

Done.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/gclient/api.py (right):

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/gclient/api.py:151: 'checkout',
cwd(*cfg.solutions[0].name.split(self.m.path.sep)),
On 2013/09/26 21:46:02, Aaron Gable wrote:
> So we've lost the ability to save multiple paths into path.checkout and then
> select one later. I guess this is mitigated by the json output from gclient
> being in step_history, but double check it hasn't negatively affected anyone.

I converted the users of this feature over. It was dumb anyway, since in order
to select the checkout, you needed to know what the path to it was.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/path/api.py (right):

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/path/api.py:115: FILTER_METHODS = ('abspath',
'basename', 'exists', 'join', 'split',
On 2013/09/26 21:46:02, Aaron Gable wrote:
> Could still call them "OK_METHODS", though, since there's aren't any
arg-taking
> methods that we don't filter.

Yeah, but I wanted to be extra-clear, and we just happen to be lucky that all of
the methods here are easily filterable.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/path/api.py:130: if ':' in startup_cwd[0]:
On 2013/09/26 21:46:02, Aaron Gable wrote:
> if startup_cwd[0].endswith(':'):

Done

> Also, are windows drives guaranteed to be single letters? You could test for
> len(startup_cwd[0]) == 2 then too.

Yeah technically, but no

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/path/api.py:135: assert False, 'What is this, I
don\'t even: %r' % startup_cwd
On 2013/09/26 21:46:02, Aaron Gable wrote:
> As much as I approve... error strings should probably be *descriptive*.

:(

Done.






:(

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/path/api.py:139: self._startup_cwd = ['/', 'BAG OF
CATS']
On 2013/09/26 21:46:02, Aaron Gable wrote:
> 'FAKE TESTING CWD'? '------------M-A-G-I-C---D-I-V-I-D-E-R----------'?
'FUNGIBLE
> BUNNYMONSTER'?

Done. Though I was very tempted by 'FUNGIBLE BUNNYMONSTER'.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/path/api.py:181: if default and
self.c.dynamic_paths.get(pathname):
On 2013/09/26 21:46:02, Aaron Gable wrote:
> 'default' is insufficiently descriptive. Maybe 'overwrite' and flop the logic.

Done.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/path/config.py (right):

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/path/config.py:4: from slave.recipe_config_types
import Path
On 2013/09/26 21:46:02, Aaron Gable wrote:
> No empty line above this.

Done.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/path/config.py:21: ['D:\\', 'build', 'slave',
'fake_slave', 'build'],
On 2013/09/26 21:46:02, Aaron Gable wrote:
> It's generally E:\\b\build...

Done.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/path/config.py:39: c.base_paths['root'] =
c.CURRENT_WORKING_DIR[:-4]
On 2013/09/26 21:46:02, Aaron Gable wrote:
> Magic number! Comment it.

This IS the comment... the root path is 4 levels up from the CWD. You really
want a comment saying that?

[iannucci, 2013-09-27 02:09:19]

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_c...
File scripts/slave/recipe_config_types.py (right):

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_c...
scripts/slave/recipe_config_types.py:52: return 'Path([%s], %r%s)' %
(self.base.upper(), self.pieces, suffix)
On 2013/09/26 21:58:08, Aaron Gable wrote:
> Path([ROOT], piece, piece, piece) looks better than PATH([ROOT], (piece,
piece,
> piece)).
Since I changed the signature of this class, I decided to leave this alone...
can still do it if you like.

[agable, 2013-09-27 17:48:16]

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium/api.py (right):

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/api.py:32: '--src-dir',
self.m.path.checkout,
On 2013/09/27 02:08:20, iannucci wrote:
> On 2013/09/26 21:46:02, Aaron Gable wrote:
> > Did you find/replace for all instances of {m.path|api.path}.[path_method]()
to
> > remove the empty parentheses? Just want to make sure they're really all
gone.
> 
> I didn't, just the ones I saw... it actually doesn't end up making a
difference
> in the new system though. I found and killed the stragglers.

Thanks. I just wanted to make sure we weren't sending mixed messages for proper
style when you're not adding any new tokens to the path.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium/config.py (right):

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/config.py:38: GYP_DEFINES = Dict(equal_fn,
' '.join, (basestring,int,Path)),
On 2013/09/27 02:08:20, iannucci wrote:
> On 2013/09/26 21:46:02, Aaron Gable wrote:
> > Realizing that these are a bit obtuse and it might be nice to pass both
> > functions as keyword arguments to reduce potential confusion while reading
> this
> > file (and other configs).
> 
> Yes, I agree :). Another CL?

SG.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/config.py:143: c.build_dir =
Path('checkout', 'out')
On 2013/09/27 02:08:20, iannucci wrote:
> On 2013/09/26 21:46:02, Aaron Gable wrote:
> > This just looks so much like it's going to construct "checkout/out", not
> > "[CHECKOUT]/out". Can we come up with a better way of indicating that the
> first
> > element is a late-binding alias?
> 
> All caps? I could also make the pieces argument explicitly a tuple. I'll do
> that. Done.

See comments in recipe_config_types.

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/path/api.py (right):

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/path/api.py:115: FILTER_METHODS = ('abspath',
'basename', 'exists', 'join', 'split',
On 2013/09/27 02:08:20, iannucci wrote:
> On 2013/09/26 21:46:02, Aaron Gable wrote:
> > Could still call them "OK_METHODS", though, since there's aren't any
> arg-taking
> > methods that we don't filter.
> 
> Yeah, but I wanted to be extra-clear, and we just happen to be lucky that all
of
> the methods here are easily filterable.

I would actually prefer that it be called OK_METHODS (or maybe both use ALLOWED_
as their prefix), since FILTER_METHODS sounds like you're filtering the methods,
not filtering the inputs to the methods. And we're not "lucky" that the methods
are easily filterable; we're not going to allow any methods that aren't easily
filterable :)

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/path/config.py (right):

https://chromiumcodereview.appspot.com/24737002/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/path/config.py:21: ['D:\\', 'build', 'slave',
'fake_slave', 'build'],
On 2013/09/27 02:08:20, iannucci wrote:
> On 2013/09/26 21:46:02, Aaron Gable wrote:
> > It's generally E:\\b\build...
> 
> Done.

You're still missing the \b\ part of the path.

https://chromiumcodereview.appspot.com/24737002/diff/15001/scripts/slave/reci...
File scripts/slave/recipe_config_types.py (right):

https://chromiumcodereview.appspot.com/24737002/diff/15001/scripts/slave/reci...
scripts/slave/recipe_config_types.py:32: specific strings using the data in the
Path object.)
nit: your punctuation in/around your parentheses is odd.

https://chromiumcodereview.appspot.com/24737002/diff/15001/scripts/slave/reci...
scripts/slave/recipe_config_types.py:73: def __init__(self, base, pieces=(),
platform_ext=None):
I think I would prefer that BASE_RE assert that base is ALLCAPS, and have pieces
still be *pieces. I don't think our test-path-mocking convention is going to
change, so maybe even require Path('[BASE]', 'path', 'to', 'thing').

https://chromiumcodereview.appspot.com/24737002/diff/15001/scripts/slave/reci...
scripts/slave/recipe_config_types.py:75: assert not any(x in ('..', '.') for x
in pieces)
Also assert that '/' isn't one of the pieces. IIRC,
os.path.join('foo', 'bar', '/', 'baz', 'boom') == '/baz/boom'

https://chromiumcodereview.appspot.com/24737002/diff/15001/scripts/slave/reci...
scripts/slave/recipe_config_types.py:90: return 'Path([%s], %r%s)' %
(self.base.upper(), self.pieces, suffix)
Would still prefer to have this print self.pieces not in a tuple.

https://chromiumcodereview.appspot.com/24737002/diff/15001/scripts/slave/reci...
File scripts/slave/recipes/android/android_builder.expected/try_builder.json
(right):

https://chromiumcodereview.appspot.com/24737002/diff/15001/scripts/slave/reci...
scripts/slave/recipes/android/android_builder.expected/try_builder.json:145:
"[SLAVE_BUILD_ROOT]/src/[SLAVE_BUILD_ROOT]/src/repo"
Um what happened here?

https://chromiumcodereview.appspot.com/24737002/diff/15001/scripts/slave/reci...
File scripts/slave/recipes/android/android_builder.expected/x86_try_builder.json
(right):

https://chromiumcodereview.appspot.com/24737002/diff/15001/scripts/slave/reci...
scripts/slave/recipes/android/android_builder.expected/x86_try_builder.json:146:
"[SLAVE_BUILD_ROOT]/src/[SLAVE_BUILD_ROOT]/src/repo"
And here?

[iannucci, 2013-09-27 19:23:36]

Once more, with feeling.

https://codereview.chromium.org/24737002/diff/15001/scripts/slave/recipe_conf...
File scripts/slave/recipe_config_types.py (right):

https://codereview.chromium.org/24737002/diff/15001/scripts/slave/recipe_conf...
scripts/slave/recipe_config_types.py:32: specific strings using the data in the
Path object.)
On 2013/09/27 17:48:17, Aaron Gable wrote:
> nit: your punctuation in/around your parentheses is odd.

Done.

https://codereview.chromium.org/24737002/diff/15001/scripts/slave/recipe_conf...
scripts/slave/recipe_config_types.py:73: def __init__(self, base, pieces=(),
platform_ext=None):
On 2013/09/27 17:48:17, Aaron Gable wrote:
> I think I would prefer that BASE_RE assert that base is ALLCAPS, and have
pieces
> still be *pieces. I don't think our test-path-mocking convention is going to
> change, so maybe even require Path('[BASE]', 'path', 'to', 'thing').

Done.

https://codereview.chromium.org/24737002/diff/15001/scripts/slave/recipe_conf...
scripts/slave/recipe_config_types.py:75: assert not any(x in ('..', '.') for x
in pieces)
On 2013/09/27 17:48:17, Aaron Gable wrote:
> Also assert that '/' isn't one of the pieces. IIRC,
> os.path.join('foo', 'bar', '/', 'baz', 'boom') == '/baz/boom'

Done.

https://codereview.chromium.org/24737002/diff/15001/scripts/slave/recipe_conf...
scripts/slave/recipe_config_types.py:90: return 'Path([%s], %r%s)' %
(self.base.upper(), self.pieces, suffix)
On 2013/09/27 17:48:17, Aaron Gable wrote:
> Would still prefer to have this print self.pieces not in a tuple.

Done.

https://codereview.chromium.org/24737002/diff/15001/scripts/slave/recipes/and...
File scripts/slave/recipes/android/android_builder.expected/try_builder.json
(right):

https://codereview.chromium.org/24737002/diff/15001/scripts/slave/recipes/and...
scripts/slave/recipes/android/android_builder.expected/try_builder.json:145:
"[SLAVE_BUILD_ROOT]/src/[SLAVE_BUILD_ROOT]/src/repo"
On 2013/09/27 17:48:17, Aaron Gable wrote:
> Um what happened here?

api.path.checkout(self.c._internal_dir)  # _internal_dir is now a Path

Added assert into Path constructor which will flip out if we do this instead of
just blithely pasting stuff together.

https://codereview.chromium.org/24737002/diff/15001/scripts/slave/recipes/and...
File scripts/slave/recipes/android/android_builder.expected/x86_try_builder.json
(right):

https://codereview.chromium.org/24737002/diff/15001/scripts/slave/recipes/and...
scripts/slave/recipes/android/android_builder.expected/x86_try_builder.json:146:
"[SLAVE_BUILD_ROOT]/src/[SLAVE_BUILD_ROOT]/src/repo"
On 2013/09/27 17:48:17, Aaron Gable wrote:
> And here?

Done.

[Siva Chandra, 2013-09-27 19:50:11]

I am an FYI reviewer here I feel. Changes to chromium_android are sweet. Will
take them into the tests API CL after this lands.

[agable, 2013-09-27 20:19:32]

LGTM

Ship it!

https://codereview.chromium.org/24737002/diff/46001/scripts/slave/recipe_conf...
File scripts/slave/recipe_config_types.py (right):

https://codereview.chromium.org/24737002/diff/46001/scripts/slave/recipe_conf...
scripts/slave/recipe_config_types.py:32: specific strings using the data in the
Path object).
I more meant that the parentheses start partway through a sentence, then contain
an entire second sentence :P No biggie.

https://codereview.chromium.org/24737002/diff/46001/scripts/slave/recipe_conf...
scripts/slave/recipe_config_types.py:79: bypass (bool) - Bypass the type
checking and use |base| directly.
Maybe call it _bypass so no one outside of the path module is tempted to use it?

[commit-bot: I haz the power, 2013-09-27 20:35:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/iannucci@chromium.org/24737002/111001

[commit-bot: I haz the power, 2013-09-27 20:36:29]

Presubmit check for 24737002-111001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2013|2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  build/scripts/slave/recipe_config_types.py

Presubmit checks took 43.9s to calculate.

[commit-bot: I haz the power, 2013-09-27 20:39:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/iannucci@chromium.org/24737002/61001

[commit-bot: I haz the power, 2013-09-27 20:41:09]

Change committed as 225770

[iannucci, 2013-09-27 23:21:47]

https://codereview.chromium.org/24737002/diff/46001/scripts/slave/recipe_conf...
File scripts/slave/recipe_config_types.py (right):

https://codereview.chromium.org/24737002/diff/46001/scripts/slave/recipe_conf...
scripts/slave/recipe_config_types.py:32: specific strings using the data in the
Path object).
On 2013/09/27 20:19:33, Aaron Gable wrote:
> I more meant that the parentheses start partway through a sentence, then
contain
> an entire second sentence :P No biggie.

Oh. Durp.

https://codereview.chromium.org/24737002/diff/46001/scripts/slave/recipe_conf...
scripts/slave/recipe_config_types.py:79: bypass (bool) - Bypass the type
checking and use |base| directly.
On 2013/09/27 20:19:33, Aaron Gable wrote:
> Maybe call it _bypass so no one outside of the path module is tempted to use
it?

Done.