Rework iOS recipes to use latest MB approach

scripts/slave/recipe_modules/ios/api.py

 # Copyright 2015 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import copy
 
 from recipe_engine import recipe_api
 
 
 class iOSApi(recipe_api.RecipeApi):
@@ skipping 100 matching lines @@
 
       for key in (
         'xcode version',
         'GYP_DEFINES',
         'compiler',
         'configuration',
         'sdk',
       ):
         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
     # we can iterate over them without having to check if they are in the dict
     # at all.
     self.__config['triggered bots'] = self.__config.get('triggered bots', {})
     self.__config['tests'] = self.__config.get('tests', [])
     self.__config['env'] = self.__config.get('env', {})
 
     # Elements of the "tests" list are dicts. There are two types of elements,
@@ skipping 62 matching lines @@
         'scripts',
         'slave',
         'ios',
         'find_xcode.py',
       ),
       '--json-file', self.m.json.output(),
       '--version', self.__config['xcode version'],
     ], 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):
     """Builds from this bot's build config."""
     assert self.__config is not None
 
     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'])
 
     sub_path = ''
 
     if self.compiler == 'xcodebuild':
       env['GYP_GENERATORS'] = 'xcode'
       env['GYP_GENERATOR_FLAGS'] = 'xcode_project_version=3.2'
       cwd = self.m.path['checkout'].join('xcodebuild')
       cmd = [
         'xcodebuild',
         '-configuration', self.configuration,
         '-project', self.m.path['checkout'].join(
           'build',
           'all.xcodeproj',
         ),
         '-sdk', self.__config['sdk'],
       ]
     elif self.compiler == 'ninja':
       env['GYP_CROSSCOMPILE'] = '1'
       env['GYP_GENERATORS'] = 'ninja'
       sub_path = '%s-%s' % (self.configuration, {
           'simulator': 'iphonesimulator',
           'device': 'iphoneos',
         }[self.platform])
 
       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,
                              build_dir='//out/' + sub_path)
 
     if (self.compiler == 'ninja' and
         self.m.tryserver.is_tryserver and
         'without patch' not in suffix):
       affected_files = self.m.tryserver.get_files_affected_by_patch()
       # The same test may be configured to run on multiple simulators.
@@ skipping 16 matching lines @@
         if test['app'] not in test_targets:
           test['skip'] = True
 
       if requires_compile: # pragma: no cover
         cmd.extend(compile_targets)
       else:
         return
 
     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.
 
     Args:
       *args: Any additional arguments to pass to the test harness.
     """
     assert self.__config is not None
     test_failures = []
     infrastructure_failures = []
 
@@ skipping 130 matching lines @@
         self.configuration,
         'iossim',
       ),
       'ninja': self.m.path.join(
         'src',
          build_dir,
          '%s-%s' % (self.configuration, platform),
          'iossim',
       ),
     }[self.compiler]