The second solution solution for ToT catapult.

The second solution solution for ToT catapult.

This chage makes the bisect recipe check out catapult unmanagedly (ToT)
next to src/ . I locally tested this by making the recipe run tracing
unittests from this parallel checkout and it seems to work as expected.

R=dtu@chromium.org,iannucci@chromium.org

Committed: https://chromium.googlesource.com/chromium/tools/build/+/8dbc87ce68b4aab11a98c247c4d4bf07ea1d200c

[RobertoCN, 2016-07-22 05:37:59]

Description was changed from

==========
The second solution solution for ToT catapult.

This chage makes the bisect recipe check out catapult unmanagedly (ToT)
next to src/ . I locally tested this by making the recipe run tracing
unittests from this parallel checkout and it seems to work as expected.

R=dtu@chromium.org,iannucci@chromium.org
==========

to

==========
The second solution solution for ToT catapult.

This chage makes the bisect recipe check out catapult unmanagedly (ToT)
next to src/ . I locally tested this by making the recipe run tracing
unittests from this parallel checkout and it seems to work as expected.

R=dtu@chromium.org,iannucci@chromium.org
==========

[RobertoCN, 2016-07-22 05:37:59]

robertocn@chromium.org changed reviewers:
+ eakuefner@chromium.org

[dtu, 2016-07-26 22:58:48]

The chromium_tests recipe module has another mechanism for customizing what
gclient config you want to use. You'd specify it in the config file
recipe_modules/chromium_tests/tryserver_chromium_perf.py.

You can add a new config based on 'chromium_perf' in
depot_tools/recipe_modules/gclient/config.py.

[RobertoCN, 2016-07-27 19:04:02]

On 2016/07/26 22:58:48, dtu wrote:
> The chromium_tests recipe module has another mechanism for customizing what
> gclient config you want to use. You'd specify it in the config file
> recipe_modules/chromium_tests/tryserver_chromium_perf.py.
> 
> You can add a new config based on 'chromium_perf' in
> depot_tools/recipe_modules/gclient/config.py.

There is one config already that we can use; and rather than duplicate it, we
can use it and pop the got_revision_mapping['catapult'] key. See the latest
patch.

[dtu, 2016-07-28 21:18:51]

On 2016/07/27 19:04:02, RobertoCN wrote:
> On 2016/07/26 22:58:48, dtu wrote:
> > The chromium_tests recipe module has another mechanism for customizing what
> > gclient config you want to use. You'd specify it in the config file
> > recipe_modules/chromium_tests/tryserver_chromium_perf.py.
> > 
> > You can add a new config based on 'chromium_perf' in
> > depot_tools/recipe_modules/gclient/config.py.
> 
> There is one config already that we can use; and rather than duplicate it, we
> can use it and pop the got_revision_mapping['catapult'] key. See the latest
> patch.

Hmm, think I still prefer it in gclient/config. Previously there were additional
configs added all over the place with overlaps and duplicates, and keeping them
all in one place makes it easier to figure out what the actual config is doing.

[dtu, 2016-07-28 21:19:21]

On 2016/07/28 21:18:51, dtu wrote:
> On 2016/07/27 19:04:02, RobertoCN wrote:
> > On 2016/07/26 22:58:48, dtu wrote:
> > > The chromium_tests recipe module has another mechanism for customizing
what
> > > gclient config you want to use. You'd specify it in the config file
> > > recipe_modules/chromium_tests/tryserver_chromium_perf.py.
> > > 
> > > You can add a new config based on 'chromium_perf' in
> > > depot_tools/recipe_modules/gclient/config.py.
> > 
> > There is one config already that we can use; and rather than duplicate it,
we
> > can use it and pop the got_revision_mapping['catapult'] key. See the latest
> > patch.
> 
> Hmm, think I still prefer it in gclient/config. Previously there were
additional
> configs added all over the place with overlaps and duplicates, and keeping
them
> all in one place makes it easier to figure out what the actual config is
doing.

Err, I mean adding an additional tryserver_chromium_perf config to
gclient/config.py

[RobertoCN, 2016-08-08 22:33:57]

On 2016/07/28 21:19:21, dtu wrote:
> On 2016/07/28 21:18:51, dtu wrote:
> > On 2016/07/27 19:04:02, RobertoCN wrote:
> > > On 2016/07/26 22:58:48, dtu wrote:
> > > > The chromium_tests recipe module has another mechanism for customizing
> what
> > > > gclient config you want to use. You'd specify it in the config file
> > > > recipe_modules/chromium_tests/tryserver_chromium_perf.py.
> > > > 
> > > > You can add a new config based on 'chromium_perf' in
> > > > depot_tools/recipe_modules/gclient/config.py.
> > > 
> > > There is one config already that we can use; and rather than duplicate it,
> we
> > > can use it and pop the got_revision_mapping['catapult'] key. See the
latest
> > > patch.
> > 
> > Hmm, think I still prefer it in gclient/config. Previously there were
> additional
> > configs added all over the place with overlaps and duplicates, and keeping
> them
> > all in one place makes it easier to figure out what the actual config is
> doing.
> 
> Err, I mean adding an additional tryserver_chromium_perf config to
> gclient/config.py

Robbie advised against adding more configs on depot_tools (where gclient lives)
Instead I am adding a config right where we use it.

PTAL

[dtu, 2016-08-08 22:45:36]

On 2016/08/08 22:33:57, RobertoCN wrote:
> On 2016/07/28 21:19:21, dtu wrote:
> > On 2016/07/28 21:18:51, dtu wrote:
> > > On 2016/07/27 19:04:02, RobertoCN wrote:
> > > > On 2016/07/26 22:58:48, dtu wrote:
> > > > > The chromium_tests recipe module has another mechanism for customizing
> > what
> > > > > gclient config you want to use. You'd specify it in the config file
> > > > > recipe_modules/chromium_tests/tryserver_chromium_perf.py.
> > > > > 
> > > > > You can add a new config based on 'chromium_perf' in
> > > > > depot_tools/recipe_modules/gclient/config.py.
> > > > 
> > > > There is one config already that we can use; and rather than duplicate
it,
> > we
> > > > can use it and pop the got_revision_mapping['catapult'] key. See the
> latest
> > > > patch.
> > > 
> > > Hmm, think I still prefer it in gclient/config. Previously there were
> > additional
> > > configs added all over the place with overlaps and duplicates, and keeping
> > them
> > > all in one place makes it easier to figure out what the actual config is
> > doing.
> > 
> > Err, I mean adding an additional tryserver_chromium_perf config to
> > gclient/config.py
> 
> Robbie advised against adding more configs on depot_tools (where gclient
lives)
> Instead I am adding a config right where we use it.
> 
> PTAL

Oh, why's that?

[iannucci, 2016-08-08 23:00:52]

https://codereview.chromium.org/2172553003/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/tryserver_chromium_perf.py
(right):

https://codereview.chromium.org/2172553003/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/tryserver_chromium_perf.py:10:
CONFIG_CTX = DEPS['gclient'].CONFIG_CTX
this might need to be 'depot_tools/gclient' to be fully-qualified, though if
this works as-is, I'm confused by that.

Typically this would have been housed in a file called `gclient_config.py`

[iannucci, 2016-08-08 23:02:02]

On 2016/08/08 at 22:45:36, dtu wrote:
> On 2016/08/08 22:33:57, RobertoCN wrote:
> > On 2016/07/28 21:19:21, dtu wrote:
> > > On 2016/07/28 21:18:51, dtu wrote:
> > > > On 2016/07/27 19:04:02, RobertoCN wrote:
> > > > > On 2016/07/26 22:58:48, dtu wrote:
> > > > > > The chromium_tests recipe module has another mechanism for
customizing
> > > what
> > > > > > gclient config you want to use. You'd specify it in the config file
> > > > > > recipe_modules/chromium_tests/tryserver_chromium_perf.py.
> > > > > > 
> > > > > > You can add a new config based on 'chromium_perf' in
> > > > > > depot_tools/recipe_modules/gclient/config.py.
> > > > > 
> > > > > There is one config already that we can use; and rather than duplicate
it,
> > > we
> > > > > can use it and pop the got_revision_mapping['catapult'] key. See the
> > latest
> > > > > patch.
> > > > 
> > > > Hmm, think I still prefer it in gclient/config. Previously there were
> > > additional
> > > > configs added all over the place with overlaps and duplicates, and
keeping
> > > them
> > > > all in one place makes it easier to figure out what the actual config is
> > > doing.
> > > 
> > > Err, I mean adding an additional tryserver_chromium_perf config to
> > > gclient/config.py
> > 
> > Robbie advised against adding more configs on depot_tools (where gclient
lives)
> > Instead I am adding a config right where we use it.
> > 
> > PTAL
> 
> Oh, why's that?

Because otherwise depot_tools (nominally an 'upstream' repo) ends up knowing
lots of details about all of its clients (like catapult). Putting the config in
your own module means that it's defined closer to where it's used.

It's a historical accident that gclient knows about its users at all :/

[RobertoCN, 2016-08-08 23:28:26]

On 2016/08/08 23:00:52, iannucci wrote:
>
https://codereview.chromium.org/2172553003/diff/40001/scripts/slave/recipe_mo...
> File scripts/slave/recipe_modules/chromium_tests/tryserver_chromium_perf.py
> (right):
> 
>
https://codereview.chromium.org/2172553003/diff/40001/scripts/slave/recipe_mo...
> scripts/slave/recipe_modules/chromium_tests/tryserver_chromium_perf.py:10:
> CONFIG_CTX = DEPS['gclient'].CONFIG_CTX
> this might need to be 'depot_tools/gclient' to be fully-qualified, though if
> this works as-is, I'm confused by that.
> 
> Typically this would have been housed in a file called `gclient_config.py`
it only works WITHOUT depot_tools/ preceding the name. (At least in
recipe_simulation_test, it gives me a key error if I do prepend it with
depot_tools/)

I thought it better to have the config in the only place where it is used to
avoid adding yet another file to an already quite crowded recipe_module. If you
think we should, I'll be glad to move it to its own file.

[iannucci, 2016-08-09 01:59:31]

On 2016/08/08 at 23:28:26, robertocn wrote:
> On 2016/08/08 23:00:52, iannucci wrote:
> >
https://codereview.chromium.org/2172553003/diff/40001/scripts/slave/recipe_mo...
> > File scripts/slave/recipe_modules/chromium_tests/tryserver_chromium_perf.py
> > (right):
> > 
> >
https://codereview.chromium.org/2172553003/diff/40001/scripts/slave/recipe_mo...
> > scripts/slave/recipe_modules/chromium_tests/tryserver_chromium_perf.py:10:
> > CONFIG_CTX = DEPS['gclient'].CONFIG_CTX
> > this might need to be 'depot_tools/gclient' to be fully-qualified, though if
> > this works as-is, I'm confused by that.
> > 
> > Typically this would have been housed in a file called `gclient_config.py`
> it only works WITHOUT depot_tools/ preceding the name. (At least in
recipe_simulation_test, it gives me a key error if I do prepend it with
depot_tools/)
> 

Hm... that's strange/unfortunate, but I believe you... configs need some serious
love.

> I thought it better to have the config in the only place where it is used to
avoid adding yet another file to an already quite crowded recipe_module. If you
think we should, I'll be glad to move it to its own file.

Same file rationale sgtm :) It was more of an FYI in case you picked a location
at random :).

config stuff lgtm (well as good as configs can look....). can't speak for the
rest of the cl though.

[dtu, 2016-08-10 01:19:27]

On 2016/08/09 01:59:31, iannucci wrote:
> On 2016/08/08 at 23:28:26, robertocn wrote:
> > On 2016/08/08 23:00:52, iannucci wrote:
> > >
>
https://codereview.chromium.org/2172553003/diff/40001/scripts/slave/recipe_mo...
> > > File
scripts/slave/recipe_modules/chromium_tests/tryserver_chromium_perf.py
> > > (right):
> > > 
> > >
>
https://codereview.chromium.org/2172553003/diff/40001/scripts/slave/recipe_mo...
> > > scripts/slave/recipe_modules/chromium_tests/tryserver_chromium_perf.py:10:
> > > CONFIG_CTX = DEPS['gclient'].CONFIG_CTX
> > > this might need to be 'depot_tools/gclient' to be fully-qualified, though
if
> > > this works as-is, I'm confused by that.
> > > 
> > > Typically this would have been housed in a file called `gclient_config.py`
> > it only works WITHOUT depot_tools/ preceding the name. (At least in
> recipe_simulation_test, it gives me a key error if I do prepend it with
> depot_tools/)
> > 
> 
> Hm... that's strange/unfortunate, but I believe you... configs need some
serious
> love.
> 
> > I thought it better to have the config in the only place where it is used to
> avoid adding yet another file to an already quite crowded recipe_module. If
you
> think we should, I'll be glad to move it to its own file.
> 
> Same file rationale sgtm :) It was more of an FYI in case you picked a
location
> at random :).
> 
> config stuff lgtm (well as good as configs can look....). can't speak for the
> rest of the cl though.

Ah, didn't know you could define configs here. This is great.

[dtu, 2016-08-10 01:19:40]

https://codereview.chromium.org/2172553003/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/chromium_perf.py (right):

https://codereview.chromium.org/2172553003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/chromium_perf.py:22: platform,
target_bits, tests, gclient_config='chromium_perf'):
Don't think we need a new parameter, we don't have a case where chromium_config
is different from gclient_config. Maybe just rename chromium_config to
config_name or something like that.

https://codereview.chromium.org/2172553003/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/tryserver_chromium_perf.py
(right):

https://codereview.chromium.org/2172553003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/tryserver_chromium_perf.py:5: from
recipe_engine import config as recipe_config
Unused import? If it's necessary, include a comment explaining it.

https://codereview.chromium.org/2172553003/diff/60001/scripts/slave/recipes/b...
File scripts/slave/recipes/bisection/desktop_bisect.py (right):

https://codereview.chromium.org/2172553003/diff/60001/scripts/slave/recipes/b...
scripts/slave/recipes/bisection/desktop_bisect.py:32:
api.path.c.dynamic_paths['catapult'] = api.path['slave_build'].join(
What does this do?

[RobertoCN, 2016-08-10 22:59:54]

PTAL

https://codereview.chromium.org/2172553003/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/chromium_perf.py (right):

https://codereview.chromium.org/2172553003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/chromium_perf.py:22: platform,
target_bits, tests, gclient_config='chromium_perf'):
On 2016/08/10 01:19:39, dtu wrote:
> Don't think we need a new parameter, we don't have a case where
chromium_config
> is different from gclient_config. Maybe just rename chromium_config to
> config_name or something like that.

Done.

https://codereview.chromium.org/2172553003/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/tryserver_chromium_perf.py
(right):

https://codereview.chromium.org/2172553003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/tryserver_chromium_perf.py:5: from
recipe_engine import config as recipe_config
On 2016/08/10 01:19:39, dtu wrote:
> Unused import? If it's necessary, include a comment explaining it.

Done.

https://codereview.chromium.org/2172553003/diff/60001/scripts/slave/recipes/b...
File scripts/slave/recipes/bisection/desktop_bisect.py (right):

https://codereview.chromium.org/2172553003/diff/60001/scripts/slave/recipes/b...
scripts/slave/recipes/bisection/desktop_bisect.py:32:
api.path.c.dynamic_paths['catapult'] = api.path['slave_build'].join(
On 2016/08/10 01:19:40, dtu wrote:
> What does this do?

it makes it so that we can do api.path['catapult'].join('tracing', 'tracing',
'yaddayadda') and that we have a single place to change it (per recipe) if
needed. For example, it recently changed (through this last rebase) to getting
this path from AutoBisectApi.working_dir.

[dtu, 2016-08-10 23:03:21]

lgtm

[RobertoCN, 2016-08-10 23:21:19]

The CQ bit was checked by robertocn@chromium.org

[RobertoCN, 2016-08-10 23:21:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2172553003/#ps80001
(title: "Addressing feedback and rebasing")

[commit-bot: I haz the power, 2016-08-10 23:21:30]

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, 2016-08-10 23:25:52]

Description was changed from

==========
The second solution solution for ToT catapult.

This chage makes the bisect recipe check out catapult unmanagedly (ToT)
next to src/ . I locally tested this by making the recipe run tracing
unittests from this parallel checkout and it seems to work as expected.

R=dtu@chromium.org,iannucci@chromium.org
==========

to

==========
The second solution solution for ToT catapult.

This chage makes the bisect recipe check out catapult unmanagedly (ToT)
next to src/ . I locally tested this by making the recipe run tracing
unittests from this parallel checkout and it seems to work as expected.

R=dtu@chromium.org,iannucci@chromium.org

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/8dbc87ce68b4aab11a98...
==========

[commit-bot: I haz the power, 2016-08-10 23:25:54]

Committed patchset #7 (id:80001) as
https://chromium.googlesource.com/chromium/tools/build/+/8dbc87ce68b4aab11a98...