Rework iOS recipes to use latest MB approach

scripts/slave/recipe_modules/ios/api.py

Index: scripts/slave/recipe_modules/ios/api.py
diff --git a/scripts/slave/recipe_modules/ios/api.py b/scripts/slave/recipe_modules/ios/api.py
index 409832c37601ad76d9450079c5b14ef06d571233..7ac48eb5d8a5ea45ce14978744922e34fb18a78e 100644
--- a/scripts/slave/recipe_modules/ios/api.py
+++ b/scripts/slave/recipe_modules/ios/api.py
@@ -118,14 +118,14 @@ class iOSApi(recipe_api.RecipeApi):
       ):
         self.__config[key] = parent_config[key]
 
-    # We set some default GYP_DEFINES so developers don't have to set them
-    # manually on every bot. Add them in here.
-    self.__config['GYP_DEFINES']['component'] = 'static_library'
-    self.__config['GYP_DEFINES']['OS'] = 'ios'
+    if self._using_gyp():
+      # We set some default GYP_DEFINES so developers don't have to set them
+      # manually on every bot. Add them in here.
+      self.__config['GYP_DEFINES']['component'] = 'static_library'
+      self.__config['GYP_DEFINES']['OS'] = 'ios'

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

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).

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

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

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

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.

 
     # TODO(crbug.com/552146): Once 'all' works, the default should be ['all'].
     self.__config.setdefault('additional_compile_targets', ['All'])
-    self.__config.setdefault('use_mb', False)
 
     # In order to simplify the code that uses the values of self.__config, here
     # we default to empty values of their respective types, so in other places
@@ -208,8 +208,10 @@ class iOSApi(recipe_api.RecipeApi):
     ], step_test_data=lambda: self.m.json.test_api.output({}))
 
     cfg = self.m.chromium.make_config()
-    cfg.gyp_env.GYP_CROSSCOMPILE = 1
-    cfg.gyp_env.GYP_DEFINES = copy.deepcopy(self.__config['GYP_DEFINES'])
+
+    if self._using_gyp():
+      cfg.gyp_env.GYP_CROSSCOMPILE = 1

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

You don't actually need to set GYP_CROSSCOMPILE=1; OS=ios has the same effect.

+      cfg.gyp_env.GYP_DEFINES = copy.deepcopy(self.__config['GYP_DEFINES'])
     self.m.chromium.c = cfg
 
   def build(self, suffix=None):
@@ -218,22 +220,23 @@ class iOSApi(recipe_api.RecipeApi):
 
     suffix = ' (%s)' % suffix if suffix else ''
 
-    # MB and GN only work if we're doing ninja builds, so we will
-    # ignore the use_mb setting if compiler isn't set to ninja.
-    use_mb = self.__config['use_mb'] and self.compiler == 'ninja'
-    if use_mb:
+    if self._using_mb():
       self.m.chromium.c.project_generator.tool = 'mb'
 
-    # Add the default GYP_DEFINES.
-    gyp_defines = [
-      '%s=%s' % (k, v) for k, v in self.__config['GYP_DEFINES'].iteritems()
-    ]
-
     env = {
-      'GYP_DEFINES': ' '.join(gyp_defines),
       'LANDMINES_VERBOSE': '1',
     }
 
+    if self._using_gyp():
+      gyp_defines = [
+        '%s=%s' % (k, v) for k, v in self.__config['GYP_DEFINES'].iteritems()
+      ]
+      env['GYP_DEFINES'] = ' '.join(gyp_defines)
+
+    if self._using_mb():
+      env['MB_TYPE'] = self.__config['mb_type']
+      env['GN_ARGS'] = self.__config['gn_args']

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

Do you need GN_ARGS in the environment when mb_type is gyp?

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

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.

+
     # Add extra env variables.
     env.update(self.__config['env'])
 
@@ -263,18 +266,23 @@ class iOSApi(recipe_api.RecipeApi):
       cwd = self.m.path['checkout'].join('out', sub_path)
       cmd = ['ninja', '-C', cwd]
 
-    if use_mb:
+    if self._using_mb():
       # if we're using MB to generate build files, make sure we don't
       # invoke GYP directly. We still want the GYP_DEFINES set in the
       # environment, though, so that other hooks can key off of them.
       env['GYP_CHROMIUM_NO_ACTION'] = '1'
 
     step_result = self.m.gclient.runhooks(name='runhooks' + suffix, env=env)
-    step_result.presentation.step_text = (
-      '<br />GYP_DEFINES:<br />%s' % '<br />'.join(gyp_defines)
-    )
+    if self._using_gyp():
+      step_result.presentation.step_text = (
+        '<br />GYP_DEFINES:<br />%s' % '<br />'.join(gyp_defines)
+      )
+    else:
+      step_result.presentation.step_text = (
+        '<br />gn args:<br />%s' % self.__config['gn_args']
+      )
 
-    if use_mb:
+    if self._using_mb():
       self.m.chromium.run_mb(self.m.properties['mastername'],
                              self.m.properties['buildername'],
                              name='generate_build_files' + suffix,
@@ -311,6 +319,14 @@ class iOSApi(recipe_api.RecipeApi):
 
     self.m.step('compile' + suffix, cmd, cwd=cwd)
 
+  def _using_mb(self):
+    # MB and GN only work if we're doing ninja builds, so we will
+    # ignore the use_mb setting if compiler isn't set to ninja.
+    return 'mb_type' in self.__config and self.compiler == 'ninja'

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

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?

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

Sure.
compiler also has to be set to ninja.
I'm not sure what a comment would add to what the code is already saying?

[smut, 2015/11/10 23:25:33]

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.

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

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.

+
+  def _using_gyp(self):
+    return not self._using_mb() or self.__config['mb_type'] == 'gyp'
+
   def test(self, *args):
     """Runs tests as instructed by this bot's build config.