mb: Disallow gn `data` entries that refer to directories in the build directory

`data` entries that are directories are fine for directories in the source
tree, but in the build tree they cause incorrect incremental builds:
Consider a build step that copies a bunch of files into the build out dir.
At a later revision, it copies fewer files. Incremental bots will still have
the now-removed file sitting around, and due to the listed directory,
the stale file will be zipped up and sent to swarming.

For now, enable this check only on non-cros non-msan linux, on fuchsia,
and on iOS.

Add temporary whitelist entries for existing violations. (See patch set
17 for how some of them can be removed, but I'd like to land those in
separate CLs.)

Bug: 912946
Change-Id: Ia01d8e43f0256e3fcae39cb3e1529084d587bb7f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1764392
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Takuto Ikuta <tikuta@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690235}

tools/mb/mb.py

@@ -846,9 +846,9 @@ class MetaBuildWrapper(object):
       return ret
 
     if getattr(self.args, 'swarming_targets_file', None):
-      self.GenerateIsolates(vals, isolate_targets, isolate_map, build_dir)
+      ret = self.GenerateIsolates(vals, isolate_targets, isolate_map, build_dir)
 
-    return 0
+    return ret
 
   def RunGNGenAllIsolates(self, vals):
     """
@@ -964,8 +964,11 @@ class MetaBuildWrapper(object):
       runtime_deps = self.ReadFile(path_to_use).splitlines()
 
       canonical_target = target.replace(':','_').replace('/','_')
-      self.WriteIsolateFiles(build_dir, command, canonical_target, runtime_deps,
-                             extra_files)
+      ret = self.WriteIsolateFiles(build_dir, command, canonical_target,
+                                   runtime_deps, vals, extra_files)
+      if ret:
+        return ret
+    return 0
 
   def PossibleRuntimeDepsPaths(self, vals, ninja_targets, isolate_map):
     """Returns a map of targets to possible .runtime_deps paths.
@@ -1052,8 +1055,10 @@ class MetaBuildWrapper(object):
 
     runtime_deps = out.splitlines()
 
-    self.WriteIsolateFiles(build_dir, command, target, runtime_deps,
-                           extra_files)
+    ret = self.WriteIsolateFiles(build_dir, command, target, runtime_deps, vals,
+                                 extra_files)
+    if ret:
+      return ret
 
     ret, _, _ = self.Run([
         self.executable,
@@ -1067,14 +1072,62 @@ class MetaBuildWrapper(object):
 
     return ret
 
-  def WriteIsolateFiles(self, build_dir, command, target, runtime_deps,
+  def WriteIsolateFiles(self, build_dir, command, target, runtime_deps, vals,
                         extra_files):
     isolate_path = self.ToAbsPath(build_dir, target + '.isolate')
+    files = sorted(set(runtime_deps + extra_files))
+
+    # Complain if any file is a directory that's inside the build directory,
+    # since that makes incremental builds incorrect. See
+    # https://crbug.com/912946
+    is_android = 'target_os="android"' in vals['gn_args']
+    is_cros = ('target_os="chromeos"' in vals['gn_args'] or
+               vals.get('cros_passthrough', False))
+    is_mac = self.platform == 'darwin'
+    is_win = self.platform == 'win32' or 'target_os="win"' in vals['gn_args']
+    is_msan = 'is_msan=true' in vals['gn_args']
+
+    err = ''
+    for f in files:
+      # Skip a few configs that need extra cleanup for now.
+      # TODO(https://crbug.com/912946): Fix everything on all platforms and
+      # enable check everywhere.
+      if is_android or is_cros or is_mac or is_win or is_msan:
+        break
+
+      # Skip a few existing violations that need to be cleaned up. Each of
+      # these will lead to incorrect incremental builds if their directory
+      # contents change. Do not add to this list.
+      # TODO(https://crbug.com/912946): Remove this if statement.
+      if (f == 'angledata/gl_cts/' or  # http://anglebug.com/3827
+          f == 'gen/chrome/browser/resources/media_router/extension/' or
+          f == 'gen/components/subresource_filter/tools/' or
+          f == 'locales/' or
+          f.startswith('nacl_test_data/') or
+          f.startswith('ppapi_nacl_tests_libs/') or
+          f == 'test_url_loader_data/'):
+        continue
+
+      path = os.path.normpath(self.PathJoin(build_dir, f))
+      # This runs before the build, so we can't use isdir(path). But
+      # isolate.py luckily requires data directories to end with '/', so we
+      # can check for that. (Check f instead of path because normpath() removes
+      # trailing slashes.)
+      if path.startswith(build_dir) and f.endswith('/'):
+        # See https://crbug.com/912946
+        err += '\n' + path
+
+    if err:
+      self.Print('error: gn `data` items may not list generated directories; '
+                 'list files in directory instead for:' + err,
+                 file=sys.stderr)
+      return 1
+
     self.WriteFile(isolate_path,
       pprint.pformat({
         'variables': {
           'command': command,
-          'files': sorted(set(runtime_deps + extra_files)),
+          'files': files,
         }
       }) + '\n')
 

tools/mb/mb_unittest.py

@@ -5,6 +5,8 @@
 
 """Tests for mb.py."""
 
+from __future__ import print_function
+
 import json
 import os
 import re
@@ -613,6 +615,49 @@ class UnitTest(unittest.TestCase):
     self.check(['isolate', '//out/Default', 'base_unittests'],
                files=files, ret=0)
 
+  def test_isolate_dir(self):
+    files = {
+      '/fake_src/out/Default/toolchain.ninja': "",
+      '/fake_src/testing/buildbot/gn_isolate_map.pyl': (
+          "{'base_unittests': {"
+          "  'label': '//base:base_unittests',"
+          "  'type': 'raw',"
+          "  'args': [],"
+          "}}\n"
+      ),
+    }
+    mbw = self.fake_mbw(files=files)
+    mbw.cmds.append((0, '', ''))  # Result of `gn gen`
+    mbw.cmds.append((0, '', ''))  # Result of `autoninja`
+
+    # Result of `gn desc runtime_deps`
+    mbw.cmds.append((0, 'base_unitests\n../../test_data/\n', ''))
+    self.check(['isolate', '-c', 'debug_goma', '//out/Default',
+                'base_unittests'], mbw=mbw, ret=0, err='')
+
+  def test_isolate_generated_dir(self):
+    files = {
+      '/fake_src/out/Default/toolchain.ninja': "",
+      '/fake_src/testing/buildbot/gn_isolate_map.pyl': (
+          "{'base_unittests': {"
+          "  'label': '//base:base_unittests',"
+          "  'type': 'raw',"
+          "  'args': [],"
+          "}}\n"
+      ),
+    }
+    mbw = self.fake_mbw(files=files)
+    mbw.cmds.append((0, '', ''))  # Result of `gn gen`
+    mbw.cmds.append((0, '', ''))  # Result of `autoninja`
+
+    # Result of `gn desc runtime_deps`
+    mbw.cmds.append((0, 'base_unitests\ntest_data/\n', ''))
+    expected_err = ('error: gn `data` items may not list generated directories;'
+                    ' list files in directory instead for:\n'
+                    '//out/Default/test_data\n')
+    self.check(['isolate', '-c', 'debug_goma', '//out/Default',
+                'base_unittests'], mbw=mbw, ret=1, err=expected_err)
+
   def test_run(self):
     files = {
       '/fake_src/testing/buildbot/gn_isolate_map.pyl': (