Rework iOS recipes to use latest MB approach

Rework iOS recipes to use the latest MB approach.

My initial approach to integrating the iOS recipes w/ MB
and GN called for MB to ignore the GYP_DEFINES set in the
bot config files and to only look at the configuration
information in mb_config.pyl.

Subsequent conversation w/ smut@ led us to try
instead leaving the information in the bot config
files and making MB aware of them instead, so that users
would not have to configure the bots in two different
places.

R=smut@google.com
BUG=517216
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=297631

[Dirk Pranke, 2015-11-10 02:03:31]

This patch depends on the changes in patchset #9 of
https://codereview.chromium.org/1411183010/ in order for the 'passthrough' mode
to MB to work.

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

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/ios/api.py:125: self.__config['GYP_DEFINES']['OS']
= 'ios'
note that I'm not generally a fan of this. I can understand the desire to not
have to repeat yourself across config files, but having parts
of the GYP_DEFINES coming from build-side recipes and parts coming from src-side
config files doesn't seem good, either.

Ideally we would figure out how to share things and avoid the repetition and
still keep stuff completely src-side (like MB does via mixins).

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/ios/api.py:213: cfg.gyp_env.GYP_CROSSCOMPILE = 1
You don't actually need to set GYP_CROSSCOMPILE=1; OS=ios has the same effect.

[smut, 2015-11-10 22:01:18]

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

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/ios/api.py:125: self.__config['GYP_DEFINES']['OS']
= 'ios'
On 2015/11/10 02:03:31, Dirk Pranke wrote:
> note that I'm not generally a fan of this. I can understand the desire to not
> have to repeat yourself across config files, but having parts
> of the GYP_DEFINES coming from build-side recipes and parts coming from
src-side
> config files doesn't seem good, either.
> 
> Ideally we would figure out how to share things and avoid the repetition and
> still keep stuff completely src-side (like MB does via mixins).

These are immutable for iOS. I guess I can just put them in the src-side configs
if you prefer.

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/ios/api.py:238: env['GN_ARGS'] =
self.__config['gn_args']
Do you need GN_ARGS in the environment when mb_type is gyp?

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/ios/api.py:325: return 'mb_type' in self.__config
and self.compiler == 'ninja'
How about moving these to line 60 and making them @properties?

I also find this somewhat confusing. I guess the modes of operation here are:

gyp + gclient runhooks + xcodebuild
gyp + gclient runhooks + ninja
gyp + mb + ninja
gn + mb + ninja

Is that correct?

Then we are using mb if mb_type is specified at all, regardless of if it's gyp
or gn. And below we are using gyp either because mb_type is not specified at
all, or because it is and it's specified as gyp.

How about a comment to this effect?

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipes/ios/t...
File scripts/slave/recipes/ios/try.py (right):

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipes/ios/t...
scripts/slave/recipes/ios/try.py:213: 'gn_args': 'target_os="ios"
ios_enable_code_signing=false',
Can this be a dict like gyp_defines?

[Dirk Pranke, 2015-11-10 22:34:15]

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

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/ios/api.py:125: self.__config['GYP_DEFINES']['OS']
= 'ios'
On 2015/11/10 22:01:18, smut wrote:
> On 2015/11/10 02:03:31, Dirk Pranke wrote:
> > note that I'm not generally a fan of this. I can understand the desire to
not
> > have to repeat yourself across config files, but having parts
> > of the GYP_DEFINES coming from build-side recipes and parts coming from
> src-side
> > config files doesn't seem good, either.
> > 
> > Ideally we would figure out how to share things and avoid the repetition and
> > still keep stuff completely src-side (like MB does via mixins).
> 
> These are immutable for iOS. I guess I can just put them in the src-side
configs
> if you prefer.

As long as we're doing this approach for just the iOS bots it doesn't matter
that much, but I still think it would be better if they were in the configs just
to
have things explicitly only be in one place.

If we started using src-side configs for non-iOS bots I would more strongly 
everything to be in the configs.

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/ios/api.py:238: env['GN_ARGS'] =
self.__config['gn_args']
On 2015/11/10 22:01:18, smut wrote:
> Do you need GN_ARGS in the environment when mb_type is gyp?

As noted in the src-side CL, no, we don't technically need GN_ARGS to be set.
It's a toss-up as to whether it should be or not.

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/ios/api.py:325: return 'mb_type' in self.__config
and self.compiler == 'ninja'
On 2015/11/10 22:01:18, smut wrote:
> How about moving these to line 60 and making them @properties?

Sure.

> I also find this somewhat confusing. I guess the modes of operation here are:
> 
> gyp + gclient runhooks + xcodebuild
> gyp + gclient runhooks + ninja
> gyp + mb + ninja
> gn + mb + ninja
> 
> Is that correct?
> 
> Then we are using mb if mb_type is specified at all, regardless of if it's gyp
> or gn.

compiler also has to be set to ninja.

> And below we are using gyp either because mb_type is not specified at
> all, or because it is and it's specified as gyp.
> 
> How about a comment to this effect?

I'm not sure what a comment would add to what the code is already saying?

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipes/ios/t...
File scripts/slave/recipes/ios/try.py (right):

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipes/ios/t...
scripts/slave/recipes/ios/try.py:213: 'gn_args': 'target_os="ios"
ios_enable_code_signing=false',
On 2015/11/10 22:01:18, smut wrote:
> Can this be a dict like gyp_defines?

I actually don't think modeling gyp_defines as a dict is a good pattern to
follow.

It's not how GYP (or GN) actually gets them, and modeling them as either Python
dicts or JSON dicts introduces type and quoting uncertainties that you have to
write code to account for. In particular, GN uses C++-style syntax, not python
syntax, so you have to use "true" instead of "True" and '"ios"' instead of
'ios'.

You also end up potentially hiding (or duplicating) the work that GN and GYP
will do automatically to de-dup arguments.

But, if I can't convince you that treating these things as strings instead of
dicts is the right way to go, we should probably make the gn args a dict.

[smut, 2015-11-10 23:25:34]

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

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/ios/api.py:325: return 'mb_type' in self.__config
and self.compiler == 'ninja'
On 2015/11/10 22:34:15, Dirk Pranke wrote:
> On 2015/11/10 22:01:18, smut wrote:
> > How about moving these to line 60 and making them @properties?
> 
> Sure.
> 
> > I also find this somewhat confusing. I guess the modes of operation here
are:
> > 
> > gyp + gclient runhooks + xcodebuild
> > gyp + gclient runhooks + ninja
> > gyp + mb + ninja
> > gn + mb + ninja
> > 
> > Is that correct?
> > 
> > Then we are using mb if mb_type is specified at all, regardless of if it's
gyp
> > or gn.
> 
> compiler also has to be set to ninja.
> 
> > And below we are using gyp either because mb_type is not specified at
> > all, or because it is and it's specified as gyp.
> > 
> > How about a comment to this effect?
> 
> I'm not sure what a comment would add to what the code is already saying?

Just so it's summarized in one place. Right now you have to read this and the
build method. Also your current comment references a use_mb setting, but you
have eliminated all reference to that key in the config.

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipes/ios/t...
File scripts/slave/recipes/ios/try.py (right):

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipes/ios/t...
scripts/slave/recipes/ios/try.py:213: 'gn_args': 'target_os="ios"
ios_enable_code_signing=false',
On 2015/11/10 22:34:15, Dirk Pranke wrote:
> On 2015/11/10 22:01:18, smut wrote:
> > Can this be a dict like gyp_defines?
> 
> I actually don't think modeling gyp_defines as a dict is a good pattern to
> follow.
> 
> It's not how GYP (or GN) actually gets them, and modeling them as either
Python
> dicts or JSON dicts introduces type and quoting uncertainties that you have to
> write code to account for. In particular, GN uses C++-style syntax, not python
> syntax, so you have to use "true" instead of "True" and '"ios"' instead of
> 'ios'.
> 
> You also end up potentially hiding (or duplicating) the work that GN and GYP
> will do automatically to de-dup arguments.
> 
> But, if I can't convince you that treating these things as strings instead of
> dicts is the right way to go, we should probably make the gn args a dict.

It's hard to parse the diff when someone updates one really long one-line string
with another. I prefer these values to be enumerated alphabetically so the diff
is really easy to see. That's really where my choice of dict came in. I didn't
realize at the time that it could be problematic.

How about a list of strings? gn_args = [
  'ios_enable_code_signing=false',
  'target_os="ios"',
]

Just to ease legibility of the config itself and especially diffs in CLs.

[Dirk Pranke, 2015-11-10 23:59:37]

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

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/ios/api.py:325: return 'mb_type' in self.__config
and self.compiler == 'ninja'
On 2015/11/10 23:25:33, smut wrote:
> On 2015/11/10 22:34:15, Dirk Pranke wrote:
> > I'm not sure what a comment would add to what the code is already saying?
> 
> Just so it's summarized in one place. Right now you have to read this and the
> build method. Also your current comment references a use_mb setting, but you
> have eliminated all reference to that key in the config.

True, I need to change 'use_mb' to 'mb_type' in the comment. I'll see if I can
clean it up otherwise as well.

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipes/ios/t...
File scripts/slave/recipes/ios/try.py (right):

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipes/ios/t...
scripts/slave/recipes/ios/try.py:213: 'gn_args': 'target_os="ios"
ios_enable_code_signing=false',
On 2015/11/10 23:25:34, smut wrote:
> It's hard to parse the diff when someone updates one really long one-line
string
> with another. 

True, that's a good point. 

GN actually takes the space-separated string of args and writes them to a
newline-separated list of args in out/Release/args.gn .

So there's some precedent that suggests that multi-line is good, though we'd
have to rework MB to take a multiline string if we actually wanted to pass it
through as is (or write the args.gn file directly and tell MB to just use that).

I will think about this some more.

[smut, 2015-11-11 00:08:09]

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipes/ios/t...
File scripts/slave/recipes/ios/try.py (right):

https://codereview.chromium.org/1410743010/diff/1/scripts/slave/recipes/ios/t...
scripts/slave/recipes/ios/try.py:213: 'gn_args': 'target_os="ios"
ios_enable_code_signing=false',
On 2015/11/10 23:59:37, Dirk Pranke wrote:
> On 2015/11/10 23:25:34, smut wrote:
> > It's hard to parse the diff when someone updates one really long one-line
> string
> > with another. 
> 
> True, that's a good point. 
> 
> GN actually takes the space-separated string of args and writes them to a
> newline-separated list of args in out/Release/args.gn .
> 
> So there's some precedent that suggests that multi-line is good, though we'd
> have to rework MB to take a multiline string if we actually wanted to pass it
> through as is (or write the args.gn file directly and tell MB to just use
that).
> 
> I will think about this some more.

Just add ' '.join(self.__config['gn_args']) in ios/api.py for minimal work. It
already does that for gyp defines.

[Dirk Pranke, 2015-11-11 00:21:29]

On 2015/11/11 00:08:09, smut wrote:
> Just add ' '.join(self.__config['gn_args']) in ios/api.py for minimal work. It
> already does that for gyp defines.

Yes, I understood that part :). I was thinking more about what I would want the 
config file format to be (dict vs. single string vs list of strings), not how
one would
implement this.

I think list of strings for both GN and GYP is perhaps the best tradeoff. Are
you open
to changing GYP to be a list of strings?

[smut, 2015-11-11 00:33:06]

On 2015/11/11 00:21:29, Dirk Pranke wrote:
> On 2015/11/11 00:08:09, smut wrote:
> > Just add ' '.join(self.__config['gn_args']) in ios/api.py for minimal work.
It
> > already does that for gyp defines.
> 
> Yes, I understood that part :). I was thinking more about what I would want
the 
> config file format to be (dict vs. single string vs list of strings), not how
> one would
> implement this.
> 
> I think list of strings for both GN and GYP is perhaps the best tradeoff. Are
> you open
> to changing GYP to be a list of strings?

I guess so.

[Dirk Pranke, 2015-11-18 04:05:00]

Description was changed from

==========
Rework iOS recipes to pass args through to MB when needed.

My initial approach to integrating the iOS recipes w/ MB
and GN called for MB to ignore the GYP_DEFINES set in the
bot config files and to only look at the configuration information
in mb_config.pyl.

Subsequent conversation w/ smut@ led us to try instead leaving
the information in the bot config files and passing it through
to MB, so that users would not have to configure GYP_DEFINES
in two different places.

R=smut@google.com
BUG=517216
==========

to

==========
Rework iOS recipes to use the latest MB approach.

My initial approach to integrating the iOS recipes w/ MB
and GN called for MB to ignore the GYP_DEFINES set in the
bot config files and to only look at the configuration
information in mb_config.pyl.

Subsequent conversation w/ smut@ led us to try
instead leaving the information in the bot config
files and making MB aware of them instead, so that users
would not have to configure the bots in two different
places.

R=smut@google.com
BUG=517216
==========

[Dirk Pranke, 2015-11-18 04:28:34]

Please take another look? 

See the updated src-side CL in https://codereview.chromium.org/1411183010/ as
well, though this CL should land before that one will work.

https://codereview.chromium.org/1410743010/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/ios/api.py (right):

https://codereview.chromium.org/1410743010/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/ios/api.py:300: build_dir='//out/' + sub_path)
We could present the GYP_DEFINES and/or gn_args and mb_type values in the step
text for this step as well.

Only logging the GYP_DEFINES after runhooks might be somewhat misleading for the
bots that use MB and/or GN.

[Dirk Pranke, 2015-11-19 20:44:02]

ping?

[smut, 2015-11-19 20:58:27]

On 2015/11/19 20:44:02, Dirk Pranke wrote:
> ping?

lgtm

[smut, 2015-11-19 20:58:50]

https://codereview.chromium.org/1410743010/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/ios/api.py (right):

https://codereview.chromium.org/1410743010/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/ios/api.py:300: build_dir='//out/' + sub_path)
On 2015/11/18 04:28:34, Dirk Pranke wrote:
> We could present the GYP_DEFINES and/or gn_args and mb_type values in the step
> text for this step as well.
> 
> Only logging the GYP_DEFINES after runhooks might be somewhat misleading for
the
> bots that use MB and/or GN.

Sure.

[Dirk Pranke, 2015-11-20 00:01:18]

I will adjust the step text in a follow-up patch.

[Dirk Pranke, 2015-11-20 00:01:22]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2015-11-20 00:01:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1410743010/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1410743010/40001

[commit-bot: I haz the power, 2015-11-20 00:30:00]

Committed patchset #3 (id:40001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=297631