Implement multi-phase build support in MB.

The PGO builders need to run multiple compiles in a single build,
each with different arguments. This was not previously possible
with MB, but this CL adds support for specifying a list of configs
per builder, and adds a --phase command line arg to indicate which
phase of the build should be generated.

It is an error to specify --phase on a builder that isn't
expected to have multiple phases. Also phases are 1-based (phase 1,
phase 2, etc.).

Also, this CL cleans up some TODOs that I no longer plan to fix.

R=sebmarchand@chromium.org
BUG=595947

Review-Url: https://codereview.chromium.org/2160353003
Cr-Commit-Position: refs/heads/master@{#406955}

tools/mb/docs/design_spec.md

@@ -46,7 +46,7 @@ with spaces and passed to GYP, or a dict that will be similarly converted),
 'mb_type' field that says whether to use GN or GYP. Bot config files
 require the full list of settings to be given explicitly.
 
-If no mathcing bot config file is found, `mb` looks in the
+If no matching bot config file is found, `mb` looks in the
 `//tools/mb/mb_config.pyl` config file to determine whether to use GYP or GN
 for a particular build directory, and what set of flags (`GYP_DEFINES` or `gn
 args`) to use.
@@ -424,16 +424,3 @@ config file change, however.
   [CR tool](https://code.google.com/p/chromium/wiki/CRUserManual). Not
   only is it only intended to replace the gyp\_chromium part of `'gclient
   runhooks'`, it is not really meant as a developer-facing tool.
-
-### Open issues
-
-* Some common flags (goma\_dir being the obvious one) may need to be
-  specified via the user, and it's unclear how to integrate this with
-  the concept of build\_configs.
-
-  Right now, MB has hard-coded support for a few flags (i.e., you can
-  pass the --goma-dir flag, and it will know to expand "${goma\_dir}" in
-  the string before calling out to the tool. We may want to generalize
-  this to a common key/value approach (perhaps then meeting the
-  ChromeOS non-goal, above), or we may want to keep this very strictly
-  limited for simplicity.

tools/mb/docs/user_guide.md

@@ -112,7 +112,9 @@ a directory, then runs GYP or GN as appropriate:
 ```
 
 Either the `-c/--config` flag or the `-m/--master` and `-b/--builder` flags
-must be specified so that `mb` can figure out which config to use.
+must be specified so that `mb` can figure out which config to use. The
+`--phase` flag must also be used with builders that have multiple
+build/compile steps (and only with those builders).
 
 By default, MB will look for a bot config file under `//ios/build/bots` (see
 [design_spec.md](the design spec) for details of how the bot config files
@@ -148,7 +150,8 @@ Prints what command will be run by `mb gen` (like `mb gen -n` but does
 not require you to specify a path).
 
 The `-b/--builder`, `-c/--config`, `-f/--config-file`, `-m/--master`,
-`-q/--quiet`, and `-v/--verbose` flags work as documented for `mb gen`.
+`--phase`, `-q/--quiet`, and `-v/--verbose` flags work as documented for
+`mb gen`.
 
 ### `mb validate`
 
@@ -200,10 +203,14 @@ expression: a dictionary with three main keys, `masters`, `configs` and
 
 The `masters` key contains a nested series of dicts containing mappings
 of master -> builder -> config . This allows us to isolate the buildbot
-recipes from the actual details of the configs.
-
-The `configs` key points to a dictionary of named build
-configurations.
+recipes from the actual details of the configs. The config should either
+be a single string value representing a key in the `configs` dictionary,
+or a list of strings, each of which is a key in the `configs` dictionary;
+the latter case is for builders that do multiple compiles with different
+arguments in a single build, and must *only* be used for such builders
+(where a --phase argument must be supplied in each lookup or gen call).
+
+The `configs` key points to a dictionary of named build configurations.
 
 There should be an key in this dict for every supported configuration
 of Chromium, meaning every configuration we have a bot for, and every

tools/mb/mb.py

@@ -78,6 +78,9 @@ class MetaBuildWrapper(object):
                         help='master name to look up config from')
       subp.add_argument('-c', '--config',
                         help='configuration to analyze')
+      subp.add_argument('--phase', type=int,
+                        help=('build phase for a given build '
+                              '(int in [1, 2, ...))'))
       subp.add_argument('-f', '--config-file', metavar='PATH',
                         default=self.default_config,
                         help='path to config file '
@@ -328,7 +331,11 @@ class MetaBuildWrapper(object):
     all_configs = {}
     for master in self.masters:
       for config in self.masters[master].values():
-        all_configs[config] = master
+        if isinstance(config, list):
+          for c in config:
+            all_configs[c] = master
+        else:
+          all_configs[config] = master
 
     # Check that every referenced args file or config actually exists.
     for config, loc in all_configs.items():
@@ -475,10 +482,15 @@ class MetaBuildWrapper(object):
         config = self.masters[master][builder]
         if config == 'tbd':
           tbd.add(builder)
+        elif isinstance(config, list):
+          vals = self.FlattenConfig(config[0])
+          if vals['type'] == 'gyp':
+            gyp.add(builder)
+          else:
+            done.add(builder)
         elif config.startswith('//'):
           done.add(builder)
         else:
-          # TODO(dpranke): Check if MB is actually running?
           vals = self.FlattenConfig(config)
           if vals['type'] == 'gyp':
             gyp.add(builder)
@@ -522,12 +534,6 @@ class MetaBuildWrapper(object):
         self.RunGNGen(vals)
       return vals
 
-    # TODO: We can only get the config for GN build dirs, not GYP build dirs.
-    # GN stores the args that were used in args.gn in the build dir,
-    # but GYP doesn't store them anywhere. We should consider modifying
-    # gyp_chromium to record the arguments it runs with in a similar
-    # manner.
-
     mb_type_path = self.PathJoin(self.ToAbsPath(build_dir), 'mb_type')
     if not self.Exists(mb_type_path):
       toolchain_path = self.PathJoin(self.ToAbsPath(build_dir),
@@ -656,12 +662,24 @@ class MetaBuildWrapper(object):
       raise MBErr('Builder name "%s"  not found under masters[%s] in "%s"' %
                   (self.args.builder, self.args.master, self.args.config_file))
 
-    return self.masters[self.args.master][self.args.builder]
+    config = self.masters[self.args.master][self.args.builder]
+    if isinstance(config, list):
+      if self.args.phase is None:
+        raise MBErr('Must specify a build --phase for %s on %s' %
+                    (self.args.builder, self.args.master))
+      phase = int(self.args.phase)
+      if phase < 1 or phase > len(config):
+        raise MBErr('Phase %d out of bounds for %s on %s' %
+                    (phase, self.args.builder, self.args.master))
+      return config[phase-1]
+
+    if self.args.phase is not None:
+      raise MBErr('Must not specify a build --phase for %s on %s' %
+                  (self.args.builder, self.args.master))
+    return config
 
   def FlattenConfig(self, config):
     mixins = self.configs[config]
-    # TODO(dpranke): We really should provide a constructor for the
-    # default set of values.
     vals = {
       'args_file': '',
       'cros_passthrough': False,
@@ -680,8 +698,6 @@ class MetaBuildWrapper(object):
       if m not in self.mixins:
         raise MBErr('Unknown mixin "%s"' % m)
 
-      # TODO: check for cycles in mixins.
-
       visited.append(m)
 
       mixin_vals = self.mixins[m]
@@ -976,7 +992,6 @@ class MetaBuildWrapper(object):
 
     # This needs to mirror the settings in //build/config/ui.gni:
     # use_x11 = is_linux && !use_ozone.
-    # TODO(dpranke): Figure out how to keep this in sync better.
     use_x11 = (self.platform == 'linux2' and
                not android and
                not 'use_ozone=true' in vals['gn_args'])

tools/mb/mb_config.pyl

@@ -110,8 +110,14 @@
       'Chromium Mac 10.9 Goma Canary (clobber)': 'gn_release_bot',
       'Chromium Mac 10.9 Goma Canary (dbg)': 'gn_debug_bot',
       'Chromium Mac 10.9 Goma Canary (dbg)(clobber)': 'gn_debug_bot',
-      'Chromium Win PGO Builder': 'gyp_official_winpgo',
-      'Chromium Win x64 PGO Builder': 'gyp_official_winpgo_x64',
+      'Chromium Win PGO Builder': [
+        'gyp_official_chrome_pgo_phase_1_x86',
+        'gyp_official_chrome_pgo_phase_2_x86'
+      ],
+      'Chromium Win x64 PGO Builder': [
+        'gyp_official_chrome_pgo_phase_1_x64',
+        'gyp_official_chrome_pgo_phase_2_x64',
+      ],
       'Chromium Windows Analyze': 'gn_windows_analyze',
       'CrWin7Goma': 'gn_release_bot_minimal_symbols_x86',
       'CrWin7Goma(clbr)': 'gn_shared_release_bot_minimal_symbols_x86',
@@ -623,9 +629,15 @@
       # but it's not clear if that's actually used anywhere.
       'win': 'gyp_official',
       'win-asan': 'gyp_official_syzyasan',
-      'win-pgo': 'gyp_official_winpgo',
+      'win-pgo': [
+        'gyp_official_chrome_pgo_phase_1_x86',
+        'gyp_official_chrome_pgo_phase_2_x86',
+      ],
       'win64': 'gyp_official_x64',
-      'win64-pgo': 'gyp_official_winpgo_x64',
+      'win64-pgo': [
+        'gyp_official_chrome_pgo_phase_1_x64',
+        'gyp_official_chrome_pgo_phase_2_x64',
+      ],
     },
 
     'official.desktop.continuous': {
@@ -900,7 +912,10 @@
       'win_nacl_sdk_build': 'gyp_release_bot_minimal_symbols_x86',
       'win_optional_gpu_tests_rel':
         'swarming_gpu_tests_deqp_gles_gn_release_trybot_x86',
-      'win_pgo': 'gyp_official_winpgo',
+      'win_pgo': [
+        'gyp_official_chrome_pgo_phase_1_x86',
+        'gyp_official_chrome_pgo_phase_2_x86',
+      ],
       'win_upload_clang': 'gn_release_bot',
       'win_chrome_official': 'gn_official_goma_minimal_symbols_x86',
       'win_x64_chromium_variable_builder': 'findit',
@@ -1344,14 +1359,20 @@
       'gyp', 'official', 'six_concurrent_links',
     ],
 
-    # TODO(crbug.com/595947) - figure out how to handle PGO, which needs
-    # to invoke GYP/GN twice, with two different sets of flags, apparently.
-    'gyp_official_winpgo': [
-      'gyp', 'error',
+    'gyp_official_chrome_pgo_phase_1_x64': [
+      'gyp', 'official', 'chrome_pgo_phase_1', 'x64',
+    ],
+
+    'gyp_official_chrome_pgo_phase_2_x64': [
+      'gyp', 'official', 'chrome_pgo_phase_2', 'x64',
+    ],
+
+    'gyp_official_chrome_pgo_phase_1_x86': [
+      'gyp', 'official', 'chrome_pgo_phase_1', 'x86',
     ],
 
-    'gyp_official_winpgo_x64': [
-      'gyp', 'error', 'x64',
+    'gyp_official_chrome_pgo_phase_2_x86': [
+      'gyp', 'official', 'chrome_pgo_phase_2', 'x86',
     ],
 
     'gyp_official_x64': [
@@ -1942,6 +1963,16 @@
       'gyp_defines': 'cfi_diag=1',
     },
 
+    'chrome_pgo_phase_1': {
+      'gn_args': 'chrome_pgo_phase=1',
+      'gyp_defines': 'chrome_pgo_phase=1',
+    },
+
+    'chrome_pgo_phase_2': {
+      'gn_args': 'chrome_pgo_phase=2',
+      'gyp_defines': 'chrome_pgo_phase=2',
+    },
+
     'chrome_with_codecs': {
       'mixins': ['ffmpeg_branding_chrome', 'proprietary_codecs'],
     },

tools/mb/mb_unittest.py

@@ -105,13 +105,6 @@ class FakeFile(object):
 
 TEST_CONFIG = """\
 {
-  'configs': {
-    'gyp_rel_bot': ['gyp', 'rel', 'goma'],
-    'gn_debug_goma': ['gn', 'debug', 'goma'],
-    'gyp_debug': ['gyp', 'debug', 'fake_feature1'],
-    'gn_rel_bot': ['gn', 'rel', 'goma'],
-    'gyp_crosscompile': ['gyp', 'crosscompile'],
-  },
   'masters': {
     'chromium': {},
     'fake_master': {
@@ -121,8 +114,18 @@ TEST_CONFIG = """\
       'fake_gn_debug_builder': 'gn_debug_goma',
       'fake_gyp_builder': 'gyp_debug',
       'fake_gn_args_bot': '//build/args/bots/fake_master/fake_gn_args_bot.gn',
+      'fake_multi_phase': ['gn_phase_1', 'gn_phase_2'],
     },
   },
+  'configs': {
+    'gyp_rel_bot': ['gyp', 'rel', 'goma'],
+    'gn_debug_goma': ['gn', 'debug', 'goma'],
+    'gyp_debug': ['gyp', 'debug', 'fake_feature1'],
+    'gn_rel_bot': ['gn', 'rel', 'goma'],
+    'gyp_crosscompile': ['gyp', 'crosscompile'],
+    'gn_phase_1': ['gn', 'phase_1'],
+    'gn_phase_2': ['gn', 'phase_2'],
+  },
   'mixins': {
     'crosscompile': {
       'gyp_crosscompile': True,
@@ -137,6 +140,14 @@ TEST_CONFIG = """\
       'gn_args': 'use_goma=true',
       'gyp_defines': 'goma=1',
     },
+    'phase_1': {
+      'gn_args': 'phase=1',
+      'gyp_args': 'phase=1',
+    },
+    'phase_2': {
+      'gn_args': 'phase=2',
+      'gyp_args': 'phase=2',
+    },
     'rel': {
       'gn_args': 'is_debug=false',
     },
@@ -489,6 +500,58 @@ class UnitTest(unittest.TestCase):
     finally:
       sys.stdout = orig_stdout
 
+  def test_multiple_phases(self):
+    # Check that not passing a --phase to a multi-phase builder fails.
+    mbw = self.check(['lookup', '-m', 'fake_master', '-b', 'fake_multi_phase'],
+                     ret=1)
+    self.assertIn('Must specify a build --phase', mbw.out)
+
+    # Check that passing a --phase to a single-phase builder fails.
+    mbw = self.check(['lookup', '-m', 'fake_master', '-b', 'fake_gn_builder',
+                      '--phase', '1'],
+                     ret=1)
+    self.assertIn('Must not specify a build --phase', mbw.out)
+
+    # Check different ranges; 0 and 3 are out of bounds, 1 and 2 should work.
+    mbw = self.check(['lookup', '-m', 'fake_master', '-b', 'fake_multi_phase',
+                      '--phase', '0'], ret=1)
+    self.assertIn('Phase 0 out of bounds', mbw.out)
+
+    mbw = self.check(['lookup', '-m', 'fake_master', '-b', 'fake_multi_phase',
+                      '--phase', '1'], ret=0)
+    self.assertIn('phase = 1', mbw.out)
+
+    mbw = self.check(['lookup', '-m', 'fake_master', '-b', 'fake_multi_phase',
+                      '--phase', '2'], ret=0)
+    self.assertIn('phase = 2', mbw.out)
+
+    mbw = self.check(['lookup', '-m', 'fake_master', '-b', 'fake_multi_phase',
+                      '--phase', '3'], ret=1)
+    self.assertIn('Phase 3 out of bounds', mbw.out)
+
+  def test_validate(self):
+    mbw = self.fake_mbw()
+    self.check(['validate'], mbw=mbw, ret=0)
+
+  def test_bad_validate(self):
+    mbw = self.fake_mbw()
+    mbw.files[mbw.default_config] = TEST_BAD_CONFIG
+    self.check(['validate'], mbw=mbw, ret=1)
+
+  def test_gyp_env_hacks(self):
+    mbw = self.fake_mbw()
+    mbw.files[mbw.default_config] = GYP_HACKS_CONFIG
+    self.check(['lookup', '-c', 'fake_config'], mbw=mbw,
+               ret=0,
+               out=("GYP_DEFINES='foo=bar baz=1'\n"
+                    "GYP_LINK_CONCURRENCY=1\n"
+                    "LLVM_FORCE_HEAD_REVISION=1\n"
+                    "python build/gyp_chromium -G output_dir=_path_\n"))
+
+
+if __name__ == '__main__':
+  unittest.main()
+
   def test_validate(self):
     mbw = self.fake_mbw()
     self.check(['validate'], mbw=mbw, ret=0)