Add 2 config names for builder 'Blimp Android Tester'

Add 2 config names for builder 'Blimp Android Tester'

Add both android and linux builder configs under master 'chromium.fyi'. These are configs for builder 'Blimp Client Engine Integration' for gn double building use.

Related CL = https://codereview.chromium.org/2390753005/

BUG=653175
Committed: https://crrev.com/804b2154c7c47a5c07f07ce426b6498649789785
Cr-Commit-Position: refs/heads/master@{#424342}

[shenghuazhang, 2016-10-06 05:25:53]

Description was changed from

==========
Add 2 config names for builder 'Blimp Android Tester'

Add both android and linux builder configs under master 'chromium.fyi'
- 'Blimp Android Test - Android'
- 'Blimp Android Test - Linux'
These are configs for builder 'Blimp Android Tester' for gn double building use.

BUG=653175
==========

to

==========
Add 2 config names for builder 'Blimp Android Tester'

Add both android and linux builder configs under master 'chromium.fyi'
- 'Blimp Android Test - Android'
- 'Blimp Android Test - Linux'
These are configs for builder 'Blimp Android Tester' for gn double building use.

Related CL = https://codereview.chromium.org/2390753005/

BUG=653175
==========

[shenghuazhang, 2016-10-06 05:32:07]

shenghuazhang@chromium.org changed reviewers:
+ jbudorick@chromium.org

[jbudorick, 2016-10-06 13:31:40]

jbudorick@chromium.org changed reviewers:
+ dpranke@chromium.org

[jbudorick, 2016-10-06 13:31:40]

+Dirk: we're trying to set up a bot that does multiple builds -- one for linux,
one for Android -- and then runs tests using build products from both. Any
thoughts on specifying two mb configurations for a single bot & what that should
look like?

https://codereview.chromium.org/2391343002/diff/1/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/2391343002/diff/1/tools/mb/mb_config.pyl#newc...
tools/mb/mb_config.pyl:87: '//build/args/blimp_client.gn',
These should probably use bot configs in //build/args/bots/ rather than directly
using the client & engine args.

[Dirk Pranke, 2016-10-06 16:55:22]

MB supports a given builder invoking it multiple times w/ different arguments.
The way to do this is to specify a list of config names rather than a single
config name, and then pass in a `phase` argument to the run_mb() call in the
recipe to indicate which one you want to invoke. 

You can see this in use in the "win_pgo" builder and the chromium_pgo recipe
(and pgo recipe_module).

Do this rather than making up fake builder names.

[jbudorick, 2016-10-06 16:56:07]

On 2016/10/06 16:55:22, Dirk Pranke (slow) wrote:
> MB supports a given builder invoking it multiple times w/ different arguments.
> The way to do this is to specify a list of config names rather than a single
> config name, and then pass in a `phase` argument to the run_mb() call in the
> recipe to indicate which one you want to invoke. 
> 
> You can see this in use in the "win_pgo" builder and the chromium_pgo recipe
> (and pgo recipe_module).
> 
> Do this rather than making up fake builder names.

oh, neat, I didn't realize that. Thanks!

[Dirk Pranke, 2016-10-06 17:04:05]

On 2016/10/06 16:56:07, jbudorick wrote:
> oh, neat, I didn't realize that. Thanks!

Yeah, *only* win_pgo currently uses this (the feature was added for PGO) and 
I don't think I ever got around to updating the docs, so I'm not surprised that 
you didn't know about it.

It just occurred to me that it might've been better if the phases
had been strings instead of numbers (and used a dict instead of an array),
to be more self-documenting and match individual steps better.

So, we could consider switching to that ...

[shenghuazhang, 2016-10-06 20:02:32]

Dirk, thanks for the good suggestion!

I modified this to be single builder 'Blimp Client Engine Integration' with 2
config arguments.

Also, I tried to modify the phase as 'dict' instead of 'array', but the
presubmit checking showed "TypeError: unhashable type: 'dict'". Do you think
there is way to bypass this if any?

[Dirk Pranke, 2016-10-06 20:06:41]

On 2016/10/06 20:02:32, shenghuazhang1 wrote:
> Dirk, thanks for the good suggestion!
> 
> I modified this to be single builder 'Blimp Client Engine Integration' with 2
> config arguments.
> 
> Also, I tried to modify the phase as 'dict' instead of 'array', but the
> presubmit checking showed "TypeError: unhashable type: 'dict'". Do you think
> there is way to bypass this if any?

Did you try to modify the code in mb.py that handles the config parsing and
lookup?

You would need to modify:

https://cs.chromium.org/chromium/src/tools/mb/mb.py?rcl=0&l=696

and

https://cs.chromium.org/chromium/src/tools/mb/mb.py?rcl=0&l=81

and possibly some unit tests.

[shenghuazhang, 2016-10-07 02:10:40]

Modified both file mb.py and mb_unittest.py to support phase related code.
Also changed 'win_pgo' builder config to dict.

[Dirk Pranke, 2016-10-07 19:01:59]

Closer! Changing the PGO builders makes life difficult; see the comments below.

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb.py#newcode81
tools/mb/mb.py:81: subp.add_argument('--phase', type=str,
Just delete the type param; str is the default.

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb.py#newcode273
tools/mb/mb.py:273: if isinstance(config, list):
Since we're changing things to be dicts, the list code will be dead, and you can
delete this branch.

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb.py#newcode276
tools/mb/mb.py:276: args = [self.FlattenConfig(c)['gn_args'] for c in
config.values()]
This isn't quite right, you'll lose the config names and the fact that it's a
dict. Instead:

  args = {k: self.FlattenConfig(v)['gn_args'] for k, v in config.items()}

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb.py#newcode374
tools/mb/mb.py:374: if isinstance(config, list):
same thing (this'll be dead code).

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb.py#newcode529
tools/mb/mb.py:529: elif isinstance(config, list):
and here.

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb_config.pyl#...
tools/mb/mb_config.pyl:639: 'pgo_phase_2':
'official_optimize_chrome_pgo_phase_2_x86',
There's also a PGO builder on the official.desktop master (line 402), and
unfortunately that one needs to build across trunk/beta/stable, and the recipe
for it isn't pinned. In other words, we can't easily change the command line it
passes to MB.

I think things will still work if we just change the keys to '0' and '1'; they
aren't good names, but it'll get the job done and we can worry about changing
the names later.

Does that make sense?

[jbudorick, 2016-10-07 19:22:01]

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb_config.pyl#...
tools/mb/mb_config.pyl:87: 'client': 'blimp_chromium_fyi_android_client',
Can these two just directly reference the files?

[Dirk Pranke, 2016-10-07 19:25:29]

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb_config.pyl#...
tools/mb/mb_config.pyl:87: 'client': 'blimp_chromium_fyi_android_client',
On 2016/10/07 19:22:01, jbudorick wrote:
> Can these two just directly reference the files?

Oh, I missed that :(.

I'd be surprised if this even works. I'd expect that if you want to use a file,
you might *have* to list it directly here, like we do in the single-config case.

[shenghuazhang, 2016-10-08 00:21:19]

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb.py#newcode81
tools/mb/mb.py:81: subp.add_argument('--phase', type=str,
On 2016/10/07 19:01:58, Dirk Pranke wrote:
> Just delete the type param; str is the default.

Done.

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb.py#newcode273
tools/mb/mb.py:273: if isinstance(config, list):
On 2016/10/07 19:01:58, Dirk Pranke wrote:
> Since we're changing things to be dicts, the list code will be dead, and you
can
> delete this branch.

Done.

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb.py#newcode276
tools/mb/mb.py:276: args = [self.FlattenConfig(c)['gn_args'] for c in
config.values()]
On 2016/10/07 19:01:59, Dirk Pranke wrote:
> This isn't quite right, you'll lose the config names and the fact that it's a
> dict. Instead:
> 
>   args = {k: self.FlattenConfig(v)['gn_args'] for k, v in config.items()}
> 

Done.

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb.py#newcode374
tools/mb/mb.py:374: if isinstance(config, list):
On 2016/10/07 19:01:59, Dirk Pranke wrote:
> same thing (this'll be dead code).

Done.

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb.py#newcode529
tools/mb/mb.py:529: elif isinstance(config, list):
On 2016/10/07 19:01:58, Dirk Pranke wrote:
> and here.

Done.

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb_config.pyl#...
tools/mb/mb_config.pyl:87: 'client': 'blimp_chromium_fyi_android_client',
On 2016/10/07 19:25:29, Dirk Pranke wrote:
> On 2016/10/07 19:22:01, jbudorick wrote:
> > Can these two just directly reference the files?
> 
> Oh, I missed that :(.
> 
> I'd be surprised if this even works. I'd expect that if you want to use a
file,
> you might *have* to list it directly here, like we do in the single-config
case.

Oh I see. Will replace the configs with the direct file path.

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb_config.pyl#...
tools/mb/mb_config.pyl:639: 'pgo_phase_2':
'official_optimize_chrome_pgo_phase_2_x86',
On 2016/10/07 19:01:59, Dirk Pranke wrote:
> There's also a PGO builder on the official.desktop master (line 402), and
> unfortunately that one needs to build across trunk/beta/stable, and the recipe
> for it isn't pinned. In other words, we can't easily change the command line
it
> passes to MB.
> 
> I think things will still work if we just change the keys to '0' and '1'; they
> aren't good names, but it'll get the job done and we can worry about changing
> the names later.
> 
> Does that make sense?

Yeah, I think change keys to '0' and '1' is a good way for now (if that works),
since things are not as simple as just change the command line in the mb config
here.

https://codereview.chromium.org/2391343002/diff/60001/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/2391343002/diff/60001/tools/mb/mb_config.pyl#...
tools/mb/mb_config.pyl:410: 'win-pgo': {
Have to change these builder configs as well, due to 'list' type of 'config' is
removed.

[Dirk Pranke, 2016-10-11 00:14:05]

lgtm w/ the edits, below. Sorry for all the hassle, and thanks for cleaning this
up!

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/2391343002/diff/40001/tools/mb/mb_config.pyl#...
tools/mb/mb_config.pyl:639: 'pgo_phase_2':
'official_optimize_chrome_pgo_phase_2_x86',
On 2016/10/08 00:21:18, shenghuazhang1 wrote:
> On 2016/10/07 19:01:59, Dirk Pranke wrote:
> > There's also a PGO builder on the official.desktop master (line 402), and
> > unfortunately that one needs to build across trunk/beta/stable, and the
recipe
> > for it isn't pinned. In other words, we can't easily change the command line
> it
> > passes to MB.
> > 
> > I think things will still work if we just change the keys to '0' and '1';
they
> > aren't good names, but it'll get the job done and we can worry about
changing
> > the names later.
> > 
> > Does that make sense?
> 
> Yeah, I think change keys to '0' and '1' is a good way for now (if that
works),
> since things are not as simple as just change the command line in the mb
config
> here.

Ah, sorry, I was wrong about this. The PGO builders are passing in '1' and '2',
not '0' and '1'; the indexes are 1-based, because I thought 0-based looked
stupid at the time :).

https://codereview.chromium.org/2391343002/diff/60001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/2391343002/diff/60001/tools/mb/mb.py#newcode82
tools/mb/mb.py:82: help=('build phase dict for a given build (key in '
I would replace the help with something like help='optional phase name (used
when builders do multiple compiles with different arguments in a single build)'.
I don't think we need to explain that the name is a key to a dict in the help.

https://codereview.chromium.org/2391343002/diff/60001/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/2391343002/diff/60001/tools/mb/mb_config.pyl#...
tools/mb/mb_config.pyl:108: '1': 'official_optimize_chrome_pgo_phase_2_x86',
Whoops, these need to be '1' and '2' instead, here and below (see my other
comment).

[shenghuazhang, 2016-10-11 01:09:27]

https://codereview.chromium.org/2391343002/diff/60001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/2391343002/diff/60001/tools/mb/mb.py#newcode82
tools/mb/mb.py:82: help=('build phase dict for a given build (key in '
On 2016/10/11 00:14:05, Dirk Pranke wrote:
> I would replace the help with something like help='optional phase name (used
> when builders do multiple compiles with different arguments in a single
build)'.
> I don't think we need to explain that the name is a key to a dict in the help.

Done.

https://codereview.chromium.org/2391343002/diff/60001/tools/mb/mb_config.pyl
File tools/mb/mb_config.pyl (right):

https://codereview.chromium.org/2391343002/diff/60001/tools/mb/mb_config.pyl#...
tools/mb/mb_config.pyl:108: '1': 'official_optimize_chrome_pgo_phase_2_x86',
On 2016/10/11 00:14:05, Dirk Pranke wrote:
> Whoops, these need to be '1' and '2' instead, here and below (see my other
> comment).

Done.

[shenghuazhang, 2016-10-11 01:09:36]

The CQ bit was checked by shenghuazhang@chromium.org

[shenghuazhang, 2016-10-11 01:09:37]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/2391343002/#ps80001
(title: "Dirk comments.")

[commit-bot: I haz the power, 2016-10-11 01:09:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dirk Pranke, 2016-10-11 01:12:50]

still lgtm, thanks again.

[commit-bot: I haz the power, 2016-10-11 02:07:24]

Description was changed from

==========
Add 2 config names for builder 'Blimp Android Tester'

Add both android and linux builder configs under master 'chromium.fyi'
- 'Blimp Android Test - Android'
- 'Blimp Android Test - Linux'
These are configs for builder 'Blimp Android Tester' for gn double building use.

Related CL = https://codereview.chromium.org/2390753005/

BUG=653175
==========

to

==========
Add 2 config names for builder 'Blimp Android Tester'

Add both android and linux builder configs under master 'chromium.fyi'
- 'Blimp Android Test - Android'
- 'Blimp Android Test - Linux'
These are configs for builder 'Blimp Android Tester' for gn double building use.

Related CL = https://codereview.chromium.org/2390753005/

BUG=653175
==========

[commit-bot: I haz the power, 2016-10-11 02:07:26]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-11 02:19:00]

Description was changed from

==========
Add 2 config names for builder 'Blimp Android Tester'

Add both android and linux builder configs under master 'chromium.fyi'
- 'Blimp Android Test - Android'
- 'Blimp Android Test - Linux'
These are configs for builder 'Blimp Android Tester' for gn double building use.

Related CL = https://codereview.chromium.org/2390753005/

BUG=653175
==========

to

==========
Add 2 config names for builder 'Blimp Android Tester'

Add both android and linux builder configs under master 'chromium.fyi'
- 'Blimp Android Test - Android'
- 'Blimp Android Test - Linux'
These are configs for builder 'Blimp Android Tester' for gn double building use.

Related CL = https://codereview.chromium.org/2390753005/

BUG=653175

Committed: https://crrev.com/804b2154c7c47a5c07f07ce426b6498649789785
Cr-Commit-Position: refs/heads/master@{#424342}
==========

[commit-bot: I haz the power, 2016-10-11 02:19:01]

Patchset 5 (id:??) landed as
https://crrev.com/804b2154c7c47a5c07f07ce426b6498649789785
Cr-Commit-Position: refs/heads/master@{#424342}

[shenghuazhang, 2016-10-11 17:18:34]

Description was changed from

==========
Add 2 config names for builder 'Blimp Android Tester'

Add both android and linux builder configs under master 'chromium.fyi'
- 'Blimp Android Test - Android'
- 'Blimp Android Test - Linux'
These are configs for builder 'Blimp Android Tester' for gn double building use.

Related CL = https://codereview.chromium.org/2390753005/

BUG=653175

Committed: https://crrev.com/804b2154c7c47a5c07f07ce426b6498649789785
Cr-Commit-Position: refs/heads/master@{#424342}
==========

to

==========
Add 2 config names for builder 'Blimp Android Tester'

Add both android and linux builder configs under master 'chromium.fyi'. These
are configs for builder 'Blimp Client Engine Integration' for gn double building
use.

Related CL = https://codereview.chromium.org/2390753005/

BUG=653175

Committed: https://crrev.com/804b2154c7c47a5c07f07ce426b6498649789785
Cr-Commit-Position: refs/heads/master@{#424342}
==========