Reland "Remove the 'gyp_config' concept from MB."

This re-lands #349043 with some fixes for win32; we weren't consistently
using os.sep everywhere. This patch fixes that and consolidates the code
so that all references to os.sep and os.path.join() happen in the same
places. In addition, this patch updates the unittests to run cleanly on
win32 and test a win32-specific example (to catch the case that broke).

TBR=scottmg@chromium.org
BUG=481692

Review URL: https://codereview.chromium.org/1348153003

Cr-Commit-Position: refs/heads/master@{#349477}

Author: dpranke

tools/mb/docs/user_guide.md

@@ -172,8 +172,6 @@ value of the `mixins` key.
 Each mixin value is itself a dictionary that contains one or more of the
 following keys:
 
-  * `gyp_configs`: a list of the configurations to build, e.g.,
-    ['Release', 'Release_x64'].
   * `gyp_crosscompile`: a boolean; if true, GYP_CROSSCOMPILE=1 is set in
     the environment and passed to GYP.
   * `gyp_defines`: a string containing a list of GYP_DEFINES.
@@ -184,8 +182,8 @@ following keys:
 
 When `mb gen` or `mb analyze` executes, it takes a config name, looks it
 up in the 'configs' dict, and then does a left-to-right expansion of the
-mixins; gyp_defines and gn_args values are concatenated, and type and
-gyp_configs values override each other.
+mixins; gyp_defines and gn_args values are concatenated, and the type values
+override each other.
 
 For example, if you had:
 
@@ -205,7 +203,6 @@ For example, if you had:
     },
     'gn': {'type': 'gn'},
     'gyp_release': {
-      'gyp_config': 'Release'
       'mixins': ['release'],
       'type': 'gyp',
     },

tools/mb/mb.py

@@ -37,7 +37,9 @@ def __init__(self):
     self.chromium_src_dir = p.normpath(d(d(d(p.abspath(__file__)))))
     self.default_config = p.join(self.chromium_src_dir, 'tools', 'mb',
                                  'mb_config.pyl')
+    self.executable = sys.executable
     self.platform = sys.platform
+    self.sep = os.sep
     self.args = argparse.Namespace()
     self.configs = {}
     self.masters = {}
@@ -146,7 +148,7 @@ def CmdLookup(self):
     elif vals['type'] == 'gyp':
       if vals['gyp_crosscompile']:
         self.Print('GYP_CROSSCOMPILE=1')
-      cmd = self.GYPCmd('<path>', vals['gyp_defines'], vals['gyp_config'])
+      cmd = self.GYPCmd('<path>', vals['gyp_defines'])
     else:
       raise MBErr('Unknown meta-build type "%s"' % vals['type'])
 
@@ -285,7 +287,6 @@ def FlattenConfig(self, config):
     vals = {
       'type': None,
       'gn_args': [],
-      'gyp_config': [],
       'gyp_defines': '',
       'gyp_crosscompile': False,
     }
@@ -311,8 +312,6 @@ def FlattenMixins(self, mixins, vals, visited):
           vals['gn_args'] += ' ' + mixin_vals['gn_args']
         else:
           vals['gn_args'] = mixin_vals['gn_args']
-      if 'gyp_config' in mixin_vals:
-        vals['gyp_config'] = mixin_vals['gyp_config']
       if 'gyp_crosscompile' in mixin_vals:
         vals['gyp_crosscompile'] = mixin_vals['gyp_crosscompile']
       if 'gyp_defines' in mixin_vals:
@@ -327,7 +326,7 @@ def FlattenMixins(self, mixins, vals, visited):
   def ClobberIfNeeded(self, vals):
     path = self.args.path[0]
     build_dir = self.ToAbsPath(path)
-    mb_type_path = os.path.join(build_dir, 'mb_type')
+    mb_type_path = self.PathJoin(build_dir, 'mb_type')
     needs_clobber = False
     new_mb_type = vals['type']
     if self.Exists(build_dir):
@@ -364,7 +363,7 @@ def RunGNGen(self, vals):
       # the compile targets to the matching GN labels.
       contents = self.ReadFile(self.args.swarming_targets_file)
       swarming_targets = contents.splitlines()
-      gn_isolate_map = ast.literal_eval(self.ReadFile(os.path.join(
+      gn_isolate_map = ast.literal_eval(self.ReadFile(self.PathJoin(
           self.chromium_src_dir, 'testing', 'buildbot', 'gn_isolate_map.pyl')))
       gn_labels = []
       for target in swarming_targets:
@@ -399,7 +398,7 @@ def RunGNGen(self, vals):
         runtime_deps_target = 'obj/%s.stamp' % label.replace(':', '/')
       else:
         runtime_deps_target = target
-      if sys.platform == 'win32':
+      if self.platform == 'win32':
         deps_path = self.ToAbsPath(path,
                                    runtime_deps_target + '.exe.runtime_deps')
       else:
@@ -426,9 +425,9 @@ def RunGNGen(self, vals):
         {
           'args': [
             '--isolated',
-            self.ToSrcRelPath('%s%s%s.isolated' % (path, os.sep, target)),
+            self.ToSrcRelPath('%s%s%s.isolated' % (path, self.sep, target)),
             '--isolate',
-            self.ToSrcRelPath('%s%s%s.isolate' % (path, os.sep, target)),
+            self.ToSrcRelPath('%s%s%s.isolate' % (path, self.sep, target)),
           ],
           'dir': self.chromium_src_dir,
           'version': 1,
@@ -440,14 +439,12 @@ def RunGNGen(self, vals):
 
   def GNCmd(self, subcommand, path, gn_args=''):
     if self.platform == 'linux2':
-      gn_path = os.path.join(self.chromium_src_dir, 'buildtools', 'linux64',
-                             'gn')
+      subdir = 'linux64'
     elif self.platform == 'darwin':
-      gn_path = os.path.join(self.chromium_src_dir, 'buildtools', 'mac',
-                             'gn')
+      subdir = 'mac'
     else:
-      gn_path = os.path.join(self.chromium_src_dir, 'buildtools', 'win',
-                             'gn.exe')
+      subdir = 'win'
+    gn_path = self.PathJoin(self.chromium_src_dir, 'buildtools', subdir, 'gn')
 
     cmd = [gn_path, subcommand, path]
     gn_args = gn_args.replace("$(goma_dir)", self.args.goma_dir)
@@ -458,12 +455,8 @@ def GNCmd(self, subcommand, path, gn_args=''):
   def RunGYPGen(self, vals):
     path = self.args.path[0]
 
-    output_dir, gyp_config = self.ParseGYPConfigPath(path)
-    if gyp_config != vals['gyp_config']:
-      raise MBErr('The last component of the path (%s) must match the '
-                  'GYP configuration specified in the config (%s), and '
-                  'it does not.' % (gyp_config, vals['gyp_config']))
-    cmd = self.GYPCmd(output_dir, vals['gyp_defines'], config=gyp_config)
+    output_dir = self.ParseGYPConfigPath(path)
+    cmd = self.GYPCmd(output_dir, vals['gyp_defines'])
     env = None
     if vals['gyp_crosscompile']:
       if self.args.verbose:
@@ -474,19 +467,15 @@ def RunGYPGen(self, vals):
     return ret
 
   def RunGYPAnalyze(self, vals):
-    output_dir, gyp_config = self.ParseGYPConfigPath(self.args.path[0])
-    if gyp_config != vals['gyp_config']:
-      raise MBErr('The last component of the path (%s) must match the '
-                  'GYP configuration specified in the config (%s), and '
-                  'it does not.' % (gyp_config, vals['gyp_config']))
+    output_dir = self.ParseGYPConfigPath(self.args.path[0])
     if self.args.verbose:
       inp = self.ReadInputJSON(['files', 'targets'])
       self.Print()
       self.Print('analyze input:')
       self.PrintJSON(inp)
       self.Print()
 
-    cmd = self.GYPCmd(output_dir, vals['gyp_defines'], config=gyp_config)
+    cmd = self.GYPCmd(output_dir, vals['gyp_defines'])
     cmd.extend(['-f', 'analyzer',
                 '-G', 'config_path=%s' % self.args.input_path[0],
                 '-G', 'analyzer_output_path=%s' % self.args.output_path[0]])
@@ -504,15 +493,15 @@ def GetIsolateCommand(self, target, vals, gn_isolate_map):
     # 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 = (sys.platform == 'linux2' and
+    use_x11 = (self.platform == 'linux2' and
                not 'target_os="android"' in vals['gn_args'] and
                not 'use_ozone=true' in vals['gn_args'])
 
     asan = 'is_asan=true' in vals['gn_args']
     msan = 'is_msan=true' in vals['gn_args']
     tsan = 'is_tsan=true' in vals['gn_args']
 
-    executable_suffix = '.exe' if sys.platform == 'win32' else ''
+    executable_suffix = '.exe' if self.platform == 'win32' else ''
 
     test_type = gn_isolate_map[target]['type']
     cmdline = []
@@ -580,38 +569,28 @@ def GetIsolateCommand(self, target, vals, gn_isolate_map):
     return cmdline, extra_files
 
   def ToAbsPath(self, build_path, *comps):
-    return os.path.join(self.chromium_src_dir,
-                        self.ToSrcRelPath(build_path),
-                        *comps)
+    return self.PathJoin(self.chromium_src_dir,
+                         self.ToSrcRelPath(build_path),
+                         *comps)
 
   def ToSrcRelPath(self, path):
     """Returns a relative path from the top of the repo."""
     # TODO: Support normal paths in addition to source-absolute paths.
     assert(path.startswith('//'))
-    return path[2:].replace('/', os.sep)
+    return path[2:].replace('/', self.sep)
 
   def ParseGYPConfigPath(self, path):
     rpath = self.ToSrcRelPath(path)
-    output_dir, _, config = rpath.rpartition('/')
-    self.CheckGYPConfigIsSupported(config, path)
-    return output_dir, config
-
-  def CheckGYPConfigIsSupported(self, config, path):
-    if config not in ('Debug', 'Release'):
-      if (sys.platform in ('win32', 'cygwin') and
-          config not in ('Debug_x64', 'Release_x64')):
-        raise MBErr('Unknown or unsupported config type "%s" in "%s"' %
-                    config, path)
-
-  def GYPCmd(self, output_dir, gyp_defines, config):
+    output_dir, _, _ = rpath.rpartition(self.sep)
+    return output_dir
+
+  def GYPCmd(self, output_dir, gyp_defines):
     gyp_defines = gyp_defines.replace("$(goma_dir)", self.args.goma_dir)
     cmd = [
-        sys.executable,
-        os.path.join('build', 'gyp_chromium'),
+        self.executable,
+        self.PathJoin('build', 'gyp_chromium'),
         '-G',
         'output_dir=' + output_dir,
-        '-G',
-        'config=' + config,
     ]
     for d in shlex.split(gyp_defines):
       cmd += ['-D', d]
@@ -667,7 +646,7 @@ def RunGNAnalyze(self, vals):
       if ret and not 'The input matches no targets' in out:
         self.WriteFailureAndRaise('gn refs returned %d: %s' % (ret, out),
                                   output_path)
-      build_dir = self.ToSrcRelPath(self.args.path[0]) + os.sep
+      build_dir = self.ToSrcRelPath(self.args.path[0]) + self.sep
       for output in out.splitlines():
         build_output = output.replace(build_dir, '')
         if build_output in inp['targets']:
@@ -746,7 +725,7 @@ def WriteJSON(self, obj, path, force_verbose=False):
                  (e, path))
 
   def PrintCmd(self, cmd):
-    if cmd[0] == sys.executable:
+    if cmd[0] == self.executable:
       cmd = ['python'] + cmd[1:]
     self.Print(*[pipes.quote(c) for c in cmd])
 
@@ -794,6 +773,10 @@ def MaybeMakeDirectory(self, path):
       if e.errno != errno.EEXIST:
         raise
 
+  def PathJoin(self, *comps):
+    # This function largely exists so it can be overriden for testing.
+    return os.path.join(*comps)
+
   def ReadFile(self, path):
     # This function largely exists so it can be overriden for testing.
     with open(path) as fp:
@@ -804,7 +787,7 @@ def RemoveFile(self, path):
     os.remove(path)
 
   def RemoveDirectory(self, abs_path):
-    if sys.platform == 'win32':
+    if self.platform == 'win32':
       # In other places in chromium, we often have to retry this command
       # because we're worried about other processes still holding on to
       # file handles, but when MB is invoked, it will be early enough in the

tools/mb/mb_config.pyl

@@ -159,7 +159,6 @@
 
     'debug': {
       'gn_args': 'is_debug=true',
-      'gyp_config': 'Debug',
     },
 
     'debug_bot': {
@@ -254,12 +253,10 @@
     'official': {
       'gn_args': 'is_chrome_branded is_official_build=true is_debug=false is_component_build=false',
       'gyp_defines': 'branding="Chrome" buildtype="Official" component=static_library',
-      'gyp_config': 'Release',
     },
 
     'release': {
       'gn_args': 'is_debug=false',
-      'gyp_config': 'Release',
     },
 
     'release_bot': {

tools/mb/mb_unittest.py

@@ -7,6 +7,7 @@
 
 import json
 import StringIO
+import os
 import sys
 import unittest
 
@@ -16,15 +17,20 @@
 class FakeMBW(mb.MetaBuildWrapper):
   def __init__(self):
     super(FakeMBW, self).__init__()
+
+    # Override vars for test portability.
+    self.chromium_src_dir = '/fake_src'
+    self.default_config = '/fake_src/tools/mb/mb_config.pyl'
+    self.executable = 'python'
+    self.platform = 'linux2'
+    self.sep = '/'
+
     self.files = {}
     self.calls = []
     self.cmds = []
     self.cross_compile = None
     self.out = ''
     self.err = ''
-    self.platform = 'linux2'
-    self.chromium_src_dir = '/fake_src'
-    self.default_config = '/fake_src/tools/mb/mb_config.pyl'
     self.rmdirs = []
 
   def ExpandUser(self, path):
@@ -36,6 +42,9 @@ def Exists(self, path):
   def MaybeMakeDirectory(self, path):
     self.files[path] = True
 
+  def PathJoin(self, *comps):
+    return self.sep.join(comps)
+
   def ReadFile(self, path):
     return self.files[path]
 
@@ -121,11 +130,9 @@ def close(self):
     },
     'rel': {
       'gn_args': 'is_debug=false',
-      'gyp_config': 'Release',
     },
     'debug': {
       'gn_args': 'is_debug=true',
-      'gyp_config': 'Debug',
     },
   },
   'private_configs': ['private'],
@@ -166,7 +173,7 @@ def test_clobber(self):
     # The first time we run this, the build dir doesn't exist, so no clobber.
     self.check(['gen', '-c', 'gn_debug', '//out/Debug'], mbw=mbw, ret=0)
     self.assertEqual(mbw.rmdirs, [])
-    self.assertTrue(mbw.files['/fake_src/out/Debug/mb_type'], 'gn')
+    self.assertEqual(mbw.files['/fake_src/out/Debug/mb_type'], 'gn')
 
     # The second time we run this, the build dir exists and matches, so no
     # clobber.
@@ -294,7 +301,18 @@ def test_gyp_crosscompile(self):
     self.assertTrue(mbw.cross_compile)
 
   def test_gyp_gen(self):
-    self.check(['gen', '-c', 'gyp_rel_bot', '//out/Release'], ret=0)
+    self.check(['gen', '-c', 'gyp_rel_bot', '-g', '/goma', '//out/Release'],
+               ret=0,
+               out=("python build/gyp_chromium -G output_dir=out "
+                    "-D goma=1 -D gomadir=/goma\n"))
+
+    # simulate win32
+    mbw = self.fake_mbw()
+    mbw.sep = '\\'
+    self.check(['gen', '-c', 'gyp_rel_bot', '-g', 'c:\\goma', '//out/Release'],
+               mbw=mbw, ret=0,
+               out=("python 'build\\gyp_chromium' -G output_dir=out "
+                    "-D goma=1 -D 'gomadir=c:\\goma'\n"))
 
   def test_gyp_gen_fails(self):
     mbw = self.fake_mbw()
@@ -304,7 +322,7 @@ def test_gyp_gen_fails(self):
   def test_gyp_lookup_goma_dir_expansion(self):
     self.check(['lookup', '-c', 'gyp_rel_bot', '-g', '/foo'], ret=0,
                out=("python build/gyp_chromium -G 'output_dir=<path>' "
-                    "-G config=Release -D goma=1 -D gomadir=/foo\n"))
+                    "-D goma=1 -D gomadir=/foo\n"))
 
   def test_help(self):
     orig_stdout = sys.stdout