From 8c2cfd36d9ddcd4c1d6cc23b8e1dad2cc317e54c Mon Sep 17 00:00:00 2001 From: dpranke Date: Thu, 17 Sep 2015 13:12:33 -0700 Subject: [PATCH] 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} --- tools/mb/docs/user_guide.md | 7 ++-- tools/mb/mb.py | 87 ++++++++++++++++++--------------------------- tools/mb/mb_config.pyl | 3 -- tools/mb/mb_unittest.py | 34 +++++++++++++----- 4 files changed, 63 insertions(+), 68 deletions(-) diff --git a/tools/mb/docs/user_guide.md b/tools/mb/docs/user_guide.md index e1aba6fd6f02..16a6a7b9ea31 100644 --- a/tools/mb/docs/user_guide.md +++ b/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', }, diff --git a/tools/mb/mb.py b/tools/mb/mb.py index b4f7e770dbbc..10cff5365d7c 100755 --- a/tools/mb/mb.py +++ b/tools/mb/mb.py @@ -37,7 +37,9 @@ class MetaBuildWrapper(object): 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 @@ class MetaBuildWrapper(object): elif vals['type'] == 'gyp': if vals['gyp_crosscompile']: self.Print('GYP_CROSSCOMPILE=1') - cmd = self.GYPCmd('', vals['gyp_defines'], vals['gyp_config']) + cmd = self.GYPCmd('', vals['gyp_defines']) else: raise MBErr('Unknown meta-build type "%s"' % vals['type']) @@ -285,7 +287,6 @@ class MetaBuildWrapper(object): vals = { 'type': None, 'gn_args': [], - 'gyp_config': [], 'gyp_defines': '', 'gyp_crosscompile': False, } @@ -311,8 +312,6 @@ class MetaBuildWrapper(object): 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 @@ class MetaBuildWrapper(object): 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 @@ class MetaBuildWrapper(object): # 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 @@ class MetaBuildWrapper(object): 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 @@ class MetaBuildWrapper(object): { '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 @@ class MetaBuildWrapper(object): 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 @@ class MetaBuildWrapper(object): 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,11 +467,7 @@ class MetaBuildWrapper(object): 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() @@ -486,7 +475,7 @@ class MetaBuildWrapper(object): 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,7 +493,7 @@ 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 = (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']) @@ -512,7 +501,7 @@ class MetaBuildWrapper(object): 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 @@ class MetaBuildWrapper(object): 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 @@ class MetaBuildWrapper(object): 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 @@ class MetaBuildWrapper(object): (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 @@ class MetaBuildWrapper(object): 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 @@ class MetaBuildWrapper(object): 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 diff --git a/tools/mb/mb_config.pyl b/tools/mb/mb_config.pyl index a233b2248fd8..f4c03618daa8 100644 --- a/tools/mb/mb_config.pyl +++ b/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': { diff --git a/tools/mb/mb_unittest.py b/tools/mb/mb_unittest.py index 8c76e5048bed..fad5e76d910a 100755 --- a/tools/mb/mb_unittest.py +++ b/tools/mb/mb_unittest.py @@ -7,6 +7,7 @@ import json import StringIO +import os import sys import unittest @@ -16,15 +17,20 @@ import mb 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 @@ class FakeMBW(mb.MetaBuildWrapper): 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 @@ TEST_CONFIG = """\ }, '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 @@ class UnitTest(unittest.TestCase): # 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 @@ class UnitTest(unittest.TestCase): 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 @@ class UnitTest(unittest.TestCase): 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=' " - "-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 -- 2.11.4.GIT