Rework how we invoke GYP in MB.

Previously we would figure out which GYP_DEFINES we needed by
concatentating strings together from the mixins, and then we
would split the resulting string via shlex.split(), and attempt
to pass each individual arg via -D command line flags to gyp_chromium.

This seems to be a fragile process fraught with error.

This patch attempts to make things more robust by setting GYP_DEFINES
in the environment instead, like the bots normally do. In theory
this should trigger the exact same code paths in gyp_chromium we see
today when gyp_chromium is invoked as part of runhooks, and should be
safer.

R=scottmg@chromium.org
BUG=

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

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

3 files changed, 112 insertions, 41 deletions

diff --git a/tools/mb/mb.py b/tools/mb/mb.py
index bcb9cb2..12f942d 100755
--- a/tools/mb/mb.py
+++ b/tools/mb/mb.py
@@ -18,7 +18,7 @@ import json
 import os
 import pipes
 import pprint
-import shlex
+import re
 import shutil
 import sys
 import subprocess
@@ -144,15 +144,16 @@ class MetaBuildWrapper(object):
   def CmdLookup(self):
     vals = self.GetConfig()
     if vals['type'] == 'gn':
-      cmd = self.GNCmd('gen', '<path>', vals['gn_args'])
+      cmd = self.GNCmd('gen', '_path_', vals['gn_args'])
+      env = None
     elif vals['type'] == 'gyp':
       if vals['gyp_crosscompile']:
         self.Print('GYP_CROSSCOMPILE=1')
-      cmd = self.GYPCmd('<path>', vals['gyp_defines'])
+      cmd, env = self.GYPCmd('_path_', vals['gyp_defines'])
     else:
       raise MBErr('Unknown meta-build type "%s"' % vals['type'])
 
-    self.PrintCmd(cmd)
+    self.PrintCmd(cmd, env)
     return 0
 
   def CmdHelp(self):
@@ -344,6 +345,9 @@ class MetaBuildWrapper(object):
         self.Print("%s/mb_type missing, clobbering to be safe" % path)
         needs_clobber = True
 
+    if self.args.dryrun:
+      return
+
     if needs_clobber:
       self.RemoveDirectory(build_dir)
 
@@ -458,12 +462,8 @@ class MetaBuildWrapper(object):
     path = self.args.path[0]
 
     output_dir = self.ParseGYPConfigPath(path)
-    cmd = self.GYPCmd(output_dir, vals['gyp_defines'])
-    env = None
+    cmd, env = self.GYPCmd(output_dir, vals['gyp_defines'])
     if vals['gyp_crosscompile']:
-      if self.args.verbose:
-        self.Print('Setting GYP_CROSSCOMPILE=1 in the environment')
-      env = os.environ.copy()
       env['GYP_CROSSCOMPILE'] = '1'
     ret, _, _ = self.Run(cmd, env=env)
     return ret
@@ -477,11 +477,11 @@ class MetaBuildWrapper(object):
       self.PrintJSON(inp)
       self.Print()
 
-    cmd = self.GYPCmd(output_dir, vals['gyp_defines'])
+    cmd, env = 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]])
-    ret, _, _ = self.Run(cmd)
+    ret, _, _ = self.Run(cmd, env=env)
     if not ret and self.args.verbose:
       outp = json.loads(self.ReadFile(self.args.output_path[0]))
       self.Print()
@@ -587,16 +587,24 @@ class MetaBuildWrapper(object):
     return output_dir
 
   def GYPCmd(self, output_dir, gyp_defines):
-    gyp_defines = gyp_defines.replace("$(goma_dir)", self.args.goma_dir)
+    goma_dir = self.args.goma_dir
+
+    # GYP uses shlex.split() to split the gyp defines into separate arguments,
+    # so we can support backslashes and and spaces in arguments by quoting
+    # them, even on Windows, where this normally wouldn't work.
+    if '\\' in goma_dir or ' ' in goma_dir:
+      goma_dir = "'%s'" % goma_dir
+    gyp_defines = gyp_defines.replace("$(goma_dir)", goma_dir)
+
     cmd = [
         self.executable,
         self.PathJoin('build', 'gyp_chromium'),
         '-G',
         'output_dir=' + output_dir,
     ]
-    for d in shlex.split(gyp_defines):
-      cmd += ['-D', d]
-    return cmd
+    env = os.environ.copy()
+    env['GYP_DEFINES'] = gyp_defines
+    return cmd, env
 
   def RunGNAnalyze(self, vals):
     # analyze runs before 'gn gen' now, so we need to run gn gen
@@ -726,10 +734,26 @@ class MetaBuildWrapper(object):
       raise MBErr('Error %s writing to the output path "%s"' %
                  (e, path))
 
-  def PrintCmd(self, cmd):
+  def PrintCmd(self, cmd, env):
+    if self.platform == 'win32':
+      env_prefix = 'set '
+      env_quoter = QuoteForSet
+      shell_quoter = QuoteForCmd
+    else:
+      env_prefix = ''
+      env_quoter = pipes.quote
+      shell_quoter = pipes.quote
+
+    def print_env(var):
+      if env and var in env:
+        self.Print('%s%s=%s' % (env_prefix, var, env_quoter(env[var])))
+
+    print_env('GYP_CROSSCOMPILE')
+    print_env('GYP_DEFINES')
+
     if cmd[0] == self.executable:
       cmd = ['python'] + cmd[1:]
-    self.Print(*[pipes.quote(c) for c in cmd])
+    self.Print(*[shell_quoter(arg) for arg in cmd])
 
   def PrintJSON(self, obj):
     self.Print(json.dumps(obj, indent=2, sort_keys=True))
@@ -741,7 +765,7 @@ class MetaBuildWrapper(object):
   def Run(self, cmd, env=None, force_verbose=True):
     # This function largely exists so it can be overridden for testing.
     if self.args.dryrun or self.args.verbose or force_verbose:
-      self.PrintCmd(cmd)
+      self.PrintCmd(cmd, env)
     if self.args.dryrun:
       return 0, '', ''
 
@@ -815,6 +839,35 @@ class MBErr(Exception):
   pass
 
 
+# See http://goo.gl/l5NPDW and http://goo.gl/4Diozm for the painful
+# details of this next section, which handles escaping command lines
+# so that they can be copied and pasted into a cmd window.
+UNSAFE_FOR_SET = set('^<>&|')
+UNSAFE_FOR_CMD = UNSAFE_FOR_SET.union(set('()%'))
+ALL_META_CHARS = UNSAFE_FOR_CMD.union(set('"'))
+
+
+def QuoteForSet(arg):
+  if any(a in UNSAFE_FOR_SET for a in arg):
+    arg = ''.join('^' + a if a in UNSAFE_FOR_SET else a for a in arg)
+  return arg
+
+
+def QuoteForCmd(arg):
+  # First, escape the arg so that CommandLineToArgvW will parse it properly.
+  # From //tools/gyp/pylib/gyp/msvs_emulation.py:23.
+  if arg == '' or ' ' in arg or '"' in arg:
+    quote_re = re.compile(r'(\\*)"')
+    arg = '"%s"' % (quote_re.sub(lambda mo: 2 * mo.group(1) + '\\"', arg))
+
+  # Then check to see if the arg contains any metacharacters other than
+  # double quotes; if it does, quote everything (including the double
+  # quotes) for safety.
+  if any(a in UNSAFE_FOR_CMD for a in arg):
+    arg = ''.join('^' + a if a in ALL_META_CHARS else a for a in arg)
+  return arg
+
+
 if __name__ == '__main__':
   try:
     sys.exit(main(sys.argv[1:]))
diff --git a/tools/mb/mb_config.pyl b/tools/mb/mb_config.pyl
index d42ba1c..5df53ba 100644
--- a/tools/mb/mb_config.pyl
+++ b/tools/mb/mb_config.pyl
@@ -212,6 +212,8 @@
     'gn': {'type': 'gn'},
 
     'goma': {
+      # The MB code will properly escape goma_dir if necessary in the GYP
+      # code path; the GN code path needs no escaping.
       'gn_args': 'use_goma=true goma_dir="$(goma_dir)"',
       'gyp_defines': 'use_goma=1 gomadir=$(goma_dir)',
     },
@@ -254,7 +256,7 @@
 
     'official': {
       'gn_args': 'is_chrome_branded=true is_official_build=true is_debug=false is_component_build=false',
-      'gyp_defines': 'branding="Chrome" buildtype="Official" component=static_library',
+      'gyp_defines': 'branding=Chrome buildtype=Official component=static_library',
     },
 
     'release': {
diff --git a/tools/mb/mb_unittest.py b/tools/mb/mb_unittest.py
index fad5e76..64f5312 100755
--- a/tools/mb/mb_unittest.py
+++ b/tools/mb/mb_unittest.py
@@ -15,15 +15,22 @@ import mb
 
 
 class FakeMBW(mb.MetaBuildWrapper):
-  def __init__(self):
+  def __init__(self, win32=False):
     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 = '/'
+    if win32:
+      self.chromium_src_dir = 'c:\\fake_src'
+      self.default_config = 'c:\\fake_src\\tools\\mb\\mb_config.pyl'
+      self.platform = 'win32'
+      self.executable = 'c:\\python\\python.exe'
+      self.sep = '\\'
+    else:
+      self.chromium_src_dir = '/fake_src'
+      self.default_config = '/fake_src/tools/mb/mb_config.pyl'
+      self.executable = '/usr/bin/python'
+      self.platform = 'linux2'
+      self.sep = '/'
 
     self.files = {}
     self.calls = []
@@ -99,7 +106,7 @@ TEST_CONFIG = """\
   'common_dev_configs': ['gn_debug'],
   'configs': {
     'gyp_rel_bot': ['gyp', 'rel', 'goma'],
-    'gn_debug': ['gn', 'debug'],
+    'gn_debug': ['gn', 'debug', 'goma'],
     'gyp_debug': ['gyp', 'debug'],
     'gn_rel_bot': ['gn', 'rel', 'goma'],
     'private': ['gyp', 'rel', 'fake_feature1'],
@@ -126,7 +133,7 @@ TEST_CONFIG = """\
     'gn': {'type': 'gn'},
     'goma': {
       'gn_args': 'use_goma=true goma_dir="$(goma_dir)"',
-      'gyp_defines': 'goma=1 gomadir="$(goma_dir)"',
+      'gyp_defines': 'goma=1 gomadir=$(goma_dir)',
     },
     'rel': {
       'gn_args': 'is_debug=false',
@@ -142,8 +149,8 @@ TEST_CONFIG = """\
 
 
 class UnitTest(unittest.TestCase):
-  def fake_mbw(self, files=None):
-    mbw = FakeMBW()
+  def fake_mbw(self, files=None, win32=False):
+    mbw = FakeMBW(win32=win32)
     mbw.files.setdefault(mbw.default_config, TEST_CONFIG)
     if files:
       for path, contents in files.items():
@@ -248,8 +255,19 @@ class UnitTest(unittest.TestCase):
     })
 
   def test_gn_gen(self):
-    self.check(['gen', '-c', 'gn_debug', '//out/Default'], ret=0)
-    self.check(['gen', '-c', 'gyp_rel_bot', '//out/Release'], ret=0)
+    self.check(['gen', '-c', 'gn_debug', '//out/Default', '-g', '/goma'],
+               ret=0,
+               out=('/fake_src/buildtools/linux64/gn gen //out/Default '
+                    '\'--args=is_debug=true use_goma=true goma_dir="/goma"\' '
+                    '--check\n'))
+
+    mbw = self.fake_mbw(win32=True)
+    self.check(['gen', '-c', 'gn_debug', '-g', 'c:\\goma', '//out/Debug'],
+               mbw=mbw, ret=0,
+               out=('c:\\fake_src\\buildtools\\win\\gn gen //out/Debug '
+                    '"--args=is_debug=true use_goma=true goma_dir=\\"'
+                    'c:\\goma\\"" --check\n'))
+
 
   def test_gn_gen_fails(self):
     mbw = self.fake_mbw()
@@ -285,7 +303,7 @@ class UnitTest(unittest.TestCase):
 
   def test_gn_lookup_goma_dir_expansion(self):
     self.check(['lookup', '-c', 'gn_rel_bot', '-g', '/foo'], ret=0,
-               out=("/fake_src/buildtools/linux64/gn gen '<path>' "
+               out=("/fake_src/buildtools/linux64/gn gen _path_ "
                     "'--args=is_debug=false use_goma=true "
                     "goma_dir=\"/foo\"'\n" ))
 
@@ -303,16 +321,14 @@ class UnitTest(unittest.TestCase):
   def test_gyp_gen(self):
     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"))
+               out=("GYP_DEFINES='goma=1 gomadir=/goma'\n"
+                    "python build/gyp_chromium -G output_dir=out\n"))
 
-    # simulate win32
-    mbw = self.fake_mbw()
-    mbw.sep = '\\'
+    mbw = self.fake_mbw(win32=True)
     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"))
+               out=("set GYP_DEFINES=goma=1 gomadir='c:\\goma'\n"
+                    "python build\\gyp_chromium -G output_dir=out\n"))
 
   def test_gyp_gen_fails(self):
     mbw = self.fake_mbw()
@@ -321,8 +337,8 @@ 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=<path>' "
-                    "-D goma=1 -D gomadir=/foo\n"))
+               out=("GYP_DEFINES='goma=1 gomadir=/foo'\n"
+                    "python build/gyp_chromium -G output_dir=_path_\n"))
 
   def test_help(self):
     orig_stdout = sys.stdout