Base flags on bootstrapped Chromite branch.

Base flags on bootstrapped Chromite branch.

Currently, Chromite flags are chosen based on the checked-out Chromite
barnch. However, this can be overridden by the "--branch" parameter.
Update the recipe to check for this parameter and, if present, base its
flag choices off that branch instaed of the bootstrapping Chromite
branch.

BUG=chromium:650713
TEST=expectations

Committed: https://chromium.googlesource.com/chromium/tools/build/+/fae60ba54c83abbbfbe84b5533f6a3e8f9b55178

[dnj, 2016-09-27 18:11:38]

dnj@chromium.org changed reviewers:
+ bhthompson@chromium.org, nxia@chromium.org

[dnj, 2016-09-27 18:11:38]

PTAL

[dnj, 2016-09-28 19:25:50]

(ping, I still think this is a good change)

[bhthompson1, 2016-09-28 19:41:07]

bhthompson@google.com changed reviewers:
+ bhthompson@google.com

[bhthompson1, 2016-09-28 19:41:07]

LGTM but nxia may be in a better position to really review.

[nxia1, 2016-09-28 21:45:31]

nxia@google.com changed reviewers:
+ nxia@google.com

[nxia1, 2016-09-28 21:45:31]

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

https://codereview.chromium.org/2374543006/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromite/config.py:126: cgrp.cbb.extra_args =
CBB_EXTRA_ARGS
any reason for not combining this with line 95? we can pass in CBB_EXTRA_ARGS=[]
as the default value and combine them?

https://codereview.chromium.org/2374543006/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromite/config.py:163: branch_flag_idx =
list(c.cbb.extra_args).index('--branch') + 1
what happens when people pass in '--branch=master'? is it converted properly to
'--branch master' somewhere?

[nxia1, 2016-09-28 22:00:04]

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

https://codereview.chromium.org/2374543006/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromite/config.py:165: chromite_branch =
c.cbb.extra_args[branch_flag_idx]
and I'm confused why we can't overwrite c.chromite_branch by the chromite_branch
got from the extra_args.

[dnj, 2016-09-28 22:52:17]

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

https://codereview.chromium.org/2374543006/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromite/config.py:126: cgrp.cbb.extra_args =
CBB_EXTRA_ARGS
On 2016/09/28 21:45:31, nxia1 wrote:
> any reason for not combining this with line 95? we can pass in
CBB_EXTRA_ARGS=[]
> as the default value and combine them?

List doesn't have a default value.

https://codereview.chromium.org/2374543006/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromite/config.py:163: branch_flag_idx =
list(c.cbb.extra_args).index('--branch') + 1
On 2016/09/28 21:45:31, nxia1 wrote:
> what happens when people pass in '--branch=master'? is it converted properly
to
> '--branch master' somewhere? 

Ugh is that something people do? I'll fix.

https://codereview.chromium.org/2374543006/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromite/config.py:165: chromite_branch =
c.cbb.extra_args[branch_flag_idx]
On 2016/09/28 22:00:03, nxia1 wrote:
> and I'm confused why we can't overwrite c.chromite_branch by the
chromite_branch
> got from the extra_args.

c.chrome_branch controls which branch of Chromite is checked out. If a user
specifies "master" but tells it to use a separate branch via "extra_args", I
don't want the recipe to preempt them. There are use cases where this might be
desired.

If a user wants to not bootstrap, they should just specify "cbb_branch" instead
of using extra_args.

[nxia1, 2016-09-29 00:36:45]

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

https://codereview.chromium.org/2374543006/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromite/config.py:165: chromite_branch =
c.cbb.extra_args[branch_flag_idx]
On 2016/09/28 22:52:16, dnj wrote:
> On 2016/09/28 22:00:03, nxia1 wrote:
> > and I'm confused why we can't overwrite c.chromite_branch by the
> chromite_branch
> > got from the extra_args.
> 
> c.chrome_branch controls which branch of Chromite is checked out. If a user
> specifies "master" but tells it to use a separate branch via "extra_args", I
> don't want the recipe to preempt them. There are use cases where this might be
> desired.
> 
> If a user wants to not bootstrap, they should just specify "cbb_branch"
instead
> of using extra_args.


Look I didn't find the cbb_branch settings in tryserver and chromiumOS. There's
cbb_branch settings in chromeos*. is that why we didn't get the right branch
version so always passed in --git-cache-dir?

[nxia1, 2016-09-29 00:37:53]

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

https://codereview.chromium.org/2374543006/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromite/config.py:165: chromite_branch =
c.cbb.extra_args[branch_flag_idx]
On 2016/09/29 00:36:44, nxia1 wrote:
> On 2016/09/28 22:52:16, dnj wrote:
> > On 2016/09/28 22:00:03, nxia1 wrote:
> > > and I'm confused why we can't overwrite c.chromite_branch by the
> > chromite_branch
> > > got from the extra_args.
> > 
> > c.chrome_branch controls which branch of Chromite is checked out. If a user
> > specifies "master" but tells it to use a separate branch via "extra_args", I
> > don't want the recipe to preempt them. There are use cases where this might
be
> > desired.
> > 
> > If a user wants to not bootstrap, they should just specify "cbb_branch"
> instead
> > of using extra_args.
> 
> 
> Look I didn't find the cbb_branch settings in tryserver and chromiumOS.
There's
> cbb_branch settings in chromeos*. is that why we didn't get the right branch
> version so always passed in --git-cache-dir?

Look -> Looks like :)

[dnj, 2016-09-29 00:43:18]

> > Look I didn't find the cbb_branch settings in tryserver and chromiumOS.
> There's
> > cbb_branch settings in chromeos*. is that why we didn't get the right branch
> > version so always passed in --git-cache-dir?
> 
> Look -> Looks like :)

Yeah, which means that we want to check those out as master first, then
bootstrap to a branch. That's the intent, so I don't want to override it in the
recipe.

The reason we weren't making the "--git-cache-dir" decision based on the correct
branch version is because we are currently *only* looking at the "cbb_branch"
value and ignoring the extra_args. This CL fixes that.

[nxia1, 2016-09-29 01:02:33]

On 2016/09/29 00:43:18, dnj wrote:
> > > Look I didn't find the cbb_branch settings in tryserver and chromiumOS.
> > There's
> > > cbb_branch settings in chromeos*. is that why we didn't get the right
branch
> > > version so always passed in --git-cache-dir?
> > 
> > Look -> Looks like :)
> 
> Yeah, which means that we want to check those out as master first, then
> bootstrap to a branch. That's the intent, so I don't want to override it in
the
> recipe.
> 
> The reason we weren't making the "--git-cache-dir" decision based on the
correct
> branch version is because we are currently *only* looking at the "cbb_branch"
> value and ignoring the extra_args. This CL fixes that.

more clear now. thanks!

[nxia1, 2016-09-29 18:00:52]

lgtm

[dnj, 2016-09-29 19:36:17]

The CQ bit was checked by dnj@chromium.org

[dnj, 2016-09-29 19:36:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
bhthompson@google.com
Link to the patchset: https://codereview.chromium.org/2374543006/#ps20001
(title: "Handle alternative branch parameter.")

[commit-bot: I haz the power, 2016-09-29 19:36:24]

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-09-29 19:41:06]

Description was changed from

==========
Base flags on bootstrapped Chromite branch.

Currently, Chromite flags are chosen based on the checked-out Chromite
barnch. However, this can be overridden by the "--branch" parameter.
Update the recipe to check for this parameter and, if present, base its
flag choices off that branch instaed of the bootstrapping Chromite
branch.

BUG=chromium:650713
TEST=expectations
==========

to

==========
Base flags on bootstrapped Chromite branch.

Currently, Chromite flags are chosen based on the checked-out Chromite
barnch. However, this can be overridden by the "--branch" parameter.
Update the recipe to check for this parameter and, if present, base its
flag choices off that branch instaed of the bootstrapping Chromite
branch.

BUG=chromium:650713
TEST=expectations

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

[commit-bot: I haz the power, 2016-09-29 19:41:07]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/tools/build/+/fae60ba54c83abbbfbe8...

[nxia1, 2016-10-31 21:56:55]

https://codereview.chromium.org/2374543006/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromite/config.py (right):

https://codereview.chromium.org/2374543006/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromite/config.py:161: chromite_branch =
c.chromite_branch
any reason we don't overwrite c.chromite_branch with the branch we get from
'--branch/-b'

[dnj, 2016-10-31 22:13:34]

On 2016/10/31 21:56:55, nxia1 wrote:
>
https://codereview.chromium.org/2374543006/diff/20001/scripts/slave/recipe_mo...
> File scripts/slave/recipe_modules/chromite/config.py (right):
> 
>
https://codereview.chromium.org/2374543006/diff/20001/scripts/slave/recipe_mo...
> scripts/slave/recipe_modules/chromite/config.py:161: chromite_branch =
> c.chromite_branch
> any reason we don't overwrite c.chromite_branch with the branch we get from
> '--branch/-b'

Nothing that I can think of.