Testing and bugfixing for the new MB gyp/gn wrapper.

This fixes a number of small issues I found while actually testing
things to get it ready to run on the bots. In particular, 'mb analyze'
didn't work right for GN. Tests were added.

TBR=brettw@chromium.org
BUG=466436

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

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

tools/mb/docs/user_guide.md

@@ -28,7 +28,7 @@ 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.
 
-The path must be a GN-style path relative to the checkout root (as above).
+The path must be a GN-style "source-absolute" path (as above).
 
 If gen ends up using GYP, the path must have a valid GYP configuration as the
 last component of the path (i.e., specify `//out/Release_x64`, not `//out`).
@@ -46,17 +46,19 @@ 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.
 
 
-The first positional argument is the path to the build directory.
+The first positional argument must be a GN-style "source-absolute" path
+to the build directory.
 
-The second positional argument is a path to a JSON file containing a single
-object with two fields:
+The second positional argument is a (normal) path to a JSON file containing
+a single object with two fields:
 
   * `files`: an array of the modified filenames to check (as
     paths relative to the checkout root).
   * `targets`: an array of the unqualified target names to check.
 
-The third positional argument is a path where the mb will write the result,
-also as a JSON object. This object may contain the following fields:
+The third positional argument is a (normal) path to where mb will write
+the result, also as a JSON object. This object may contain the following
+fields:
 
   * `error`: this should only be present if something failed.
   * `targets`: the subset of the input `targets` that depend on the

tools/mb/mb.py

@@ -23,9 +23,9 @@ import subprocess
 
 
 def main(args):
-  mb = MetaBuildWrapper()
-  mb.ParseArgs(args)
-  return mb.args.func()
+  mbw = MetaBuildWrapper()
+  mbw.ParseArgs(args)
+  return mbw.args.func()
 
 
 class MetaBuildWrapper(object):
@@ -284,8 +284,8 @@ class MetaBuildWrapper(object):
     for m in mixins:
       if m not in self.mixins:
         raise MBErr('Unknown mixin "%s"' % m)
-      if m in visited:
-        raise MBErr('Cycle in mixins for "%s": %s' % (m, visited))
+
+      # TODO: check for cycles in mixins.
 
       visited.append(m)
 
@@ -316,7 +316,7 @@ class MetaBuildWrapper(object):
   def GNCmd(self, path, gn_args):
     # TODO(dpranke): Find gn explicitly in the path ...
     cmd = ['gn', 'gen', path]
-    gn_args = gn_args.replace("$(goma_dir)", gn_args)
+    gn_args = gn_args.replace("$(goma_dir)", self.args.goma_dir)
     if gn_args:
       cmd.append('--args=%s' % gn_args)
     return cmd
@@ -343,13 +343,15 @@ class MetaBuildWrapper(object):
     ret, _, _ = self.Run(cmd)
     return ret
 
-  def ParseGYPOutputPath(self, path):
+  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:]
 
   def ParseGYPConfigPath(self, path):
-    assert(path.startswith('//'))
-    output_dir, _, config = path[2:].rpartition('/')
+    rpath = self.ToSrcRelPath(path)
+    output_dir, _, config = rpath.rpartition('/')
     self.CheckGYPConfigIsSupported(config, path)
     return output_dir, config
 
@@ -389,21 +391,18 @@ class MetaBuildWrapper(object):
       self.WriteJSONOutput({'status': 'Found dependency (all)'})
       return 0
 
-    cmd = (['gn', 'refs', self.args.path[0]] + inp['files'] +
+    cmd = (['gn', 'refs', self.args.path[0] ] +
+           ['//' + f for f in inp['files']] +
            ['--type=executable', '--all', '--as=output'])
     needed_targets = []
     ret, out, _ = self.Run(cmd)
-
     if ret:
       self.WriteFailureAndRaise('gn refs returned %d: %s' % (ret, out))
 
-    rpath = os.path.relpath(self.args.path[0], self.chromium_src_dir) + os.sep
+    rpath = self.ToSrcRelPath(self.args.path[0]) + os.sep
     needed_targets = [t.replace(rpath, '') for t in out.splitlines()]
     needed_targets = [nt for nt in needed_targets if nt in inp['targets']]
 
-    for nt in needed_targets:
-      self.Print(nt)
-
     if needed_targets:
       # TODO: it could be that a target X might depend on a target Y
       # and both would be listed in the input, but we would only need
@@ -444,7 +443,7 @@ class MetaBuildWrapper(object):
     output_path = self.args.output_path[0]
     if output_path:
       try:
-        self.WriteFile(output_path, json.dumps(obj))
+        self.WriteFile(output_path, json.dumps(obj, indent=2) + '\n')
       except Exception as e:
         raise MBErr('Error %s writing to the output path "%s"' %
                    (e, output_path))
@@ -467,9 +466,9 @@ class MetaBuildWrapper(object):
     ret, out, err = self.Call(cmd)
     if self.args.verbose:
       if out:
-        self.Print(out)
+        self.Print(out, end='')
       if err:
-        self.Print(err, file=sys.stderr)
+        self.Print(err, end='', file=sys.stderr)
     return ret, out, err
 
   def Call(self, cmd):

tools/mb/mb_config.pyl

@@ -103,7 +103,7 @@
     },
 
     'release_trybot': {
-      'mixins': ['release_bot', 'dcheck_always_on', 'goma']
+      'mixins': ['release_bot', 'dcheck_always_on']
     },
     
     'shared': {

tools/mb/mb_unittest.py

@@ -4,19 +4,20 @@
 
 """Tests for mb.py."""
 
+import json
 import sys
 import unittest
 
 import mb
 
 
-class FakeMB(mb.MetaBuildWrapper):
+class FakeMBW(mb.MetaBuildWrapper):
   def __init__(self):
-    super(FakeMB, self).__init__()
+    super(FakeMBW, self).__init__()
     self.files = {}
     self.calls = []
-    self.out = []
-    self.err = []
+    self.out = ''
+    self.err = ''
     self.chromium_src_dir = '/fake_src'
     self.default_config = '/fake_src/tools/mb/mb_config.pyl'
 
@@ -41,9 +42,9 @@ class FakeMB(mb.MetaBuildWrapper):
     end = kwargs.get('end', '\n')
     f = kwargs.get('file', sys.stdout)
     if f == sys.stderr:
-      self.err.append(sep.join(args) + end)
+      self.err += sep.join(args) + end
     else:
-      self.out.append(sep.join(args) + end)
+      self.out += sep.join(args) + end
 
 class IntegrationTest(unittest.TestCase):
   def test_validate(self):
@@ -58,12 +59,14 @@ TEST_CONFIG = """\
   'configs': {
     'gyp_rel_bot': ['gyp', 'rel', 'goma'],
     'gn_debug': ['gn', 'debug'],
+    'gn_rel_bot': ['gn', 'rel', 'goma'],
     'private': ['gyp', 'fake_feature1'],
     'unsupported': ['gn', 'fake_feature2'],
   },
   'masters': {
     'fake_master': {
       'fake_builder': 'gyp_rel_bot',
+      'fake_gn_builder': 'gn_rel_bot',
     },
   },
   'mixins': {
@@ -96,28 +99,45 @@ TEST_CONFIG = """\
 
 
 class UnitTest(unittest.TestCase):
-  def check(self, args, files=None, cmds=None, out=None, err=None, ret=None):
-    m = FakeMB()
+  def fake_mbw(self, files):
+    mbw = FakeMBW()
     if files:
       for path, contents in files.items():
-        m.files[path] = contents
-    m.files.setdefault(mb.default_config, TEST_CONFIG)
-    m.ParseArgs(args)
-    actual_ret = m.args.func()
+        mbw.files[path] = contents
+    mbw.files.setdefault(mbw.default_config, TEST_CONFIG)
+    return mbw
+
+  def check(self, args, mbw=None, files=None, out=None, err=None, ret=None):
+    if not mbw:
+      mbw = self.fake_mbw(files)
+    mbw.ParseArgs(args)
+    actual_ret = mbw.args.func()
     if ret is not None:
       self.assertEqual(actual_ret, ret)
     if out is not None:
-      self.assertEqual(m.out, out)
+      self.assertEqual(mbw.out, out)
     if err is not None:
-      self.assertEqual(m.err, err)
-    if cmds is not None:
-      self.assertEqual(m.cmds, cmds)
+      self.assertEqual(mbw.err, err)
+    return mbw
+
+  def test_gn_analyze(self):
+    files = {'/tmp/in.json': """{\
+               "files": ["foo/foo_unittest.cc"],
+               "targets": ["foo_unittests", "bar_unittests"]
+             }"""}
+    mbw = self.fake_mbw(files)
+    mbw.Call = lambda cmd: (0, 'out/Default/foo_unittests\n', '')
 
-  def test_analyze(self):
-    files = {'/tmp/in.json': '{"files": [], "targets": []}'}
     self.check(['analyze', '-c', 'gn_debug', '//out/Default',
-                '/tmp/in.json', '/tmp/out.json'],
-               files=files, ret=0)
+                '/tmp/in.json', '/tmp/out.json'], mbw=mbw, ret=0)
+    out = json.loads(mbw.files['/tmp/out.json'])
+    self.assertEqual(out, {
+      'status': 'Found dependency',
+      'targets': ['foo_unittests'],
+      'build_targets': ['foo_unittests']
+    })
+
+  def test_gyp_analyze(self):
     self.check(['analyze', '-c', 'gyp_rel_bot', '//out/Release',
                 '/tmp/in.json', '/tmp/out.json'],
                ret=0)
@@ -126,6 +146,14 @@ class UnitTest(unittest.TestCase):
     self.check(['gen', '-c', 'gn_debug', '//out/Default'], ret=0)
     self.check(['gen', '-c', 'gyp_rel_bot', '//out/Release'], ret=0)
 
+  def test_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"))
+    self.check(['lookup', '-c', 'gn_rel_bot', '-g', '/foo'], ret=0,
+               out=("gn gen '<path>' '--args=is_debug=false use_goma=true "
+                    "goma_dir=\"/foo\"'\n" ))
+
   def test_help(self):
     self.assertRaises(SystemExit, self.check, ['-h'])
     self.assertRaises(SystemExit, self.check, ['help'])