Add presubmit check to //mojo/{public, edk} for BUILD.gn file flexibility.

mojo/PRESUBMIT.py

Index: mojo/PRESUBMIT.py
diff --git a/mojo/PRESUBMIT.py b/mojo/PRESUBMIT.py
index 557883e17bfd0f6f8ef16c54b80b2d7131c27854..6aea922da56013d63c4ec8fa8f9e5f0a507386ed 100644
--- a/mojo/PRESUBMIT.py
+++ b/mojo/PRESUBMIT.py
@@ -9,8 +9,141 @@ for more details about the presubmit API built into depot_tools.
 """
 
 import os.path
+import re
 
-def CheckChangeOnUpload(input_api, output_api):
+SDK_WHITELISTED_EXTERNAL_PATHS = [

[viettrungluu, 2014/12/10 00:02:02]

Maybe this should be _SDK_... (leading underscore).

[blundell, 2014/12/10 16:13:05]

Done.

+  "//testing/gtest",
+  "//third_party/cython",
+  "//third_party/khronos",
+]
+
+def _IsBuildFileWithinPackage(f, package):
+  """Returns whether |f| specifies a GN build file within |package| ("SDK" or
+     "EDK")."""

[viettrungluu, 2014/12/10 00:02:02]

nit: I don't believe docstrings should be indented to align, according to any
style guide....

[blundell, 2014/12/10 16:13:06]

Done.

+  if package == "SDK":

[viettrungluu, 2014/12/10 00:02:02]

Suggestion:

Define a global:

_PACKAGE_PATH_PREFIXES = {"SDK": "mojo/public",
                          "EDK": "mojo/edk"}

Then this is just:

  assert package in _PACKAGE_PATH_PREFIXES
  package_path_prefix = _PACKAGE_PATH_PREFIXES["package"]

(the assertion is optional, since it'll blow up in a pretty obvious way on
failure)

[blundell, 2014/12/10 16:13:05]

Done.

+    package_path_prefix = "mojo/public"
+  elif package == "EDK":
+    package_path_prefix = "mojo/edk"
+  else:
+    assert 0, "unknown package: %s" % package
+
+  if not f.LocalPath().startswith(package_path_prefix):

[viettrungluu, 2014/12/10 00:02:02]

Arguably, you should include a trailing '/' in the prefixes above (otherwise
mojo/edk_not_really would match).

[blundell, 2014/12/10 16:13:05]

Done.

+    return False
+  if (not f.LocalPath().endswith("BUILD.gn") and

[viettrungluu, 2014/12/10 00:02:02]

This isn't quite right. <shrug>

[blundell, 2014/12/10 16:13:06]

I assume you mean because it will return True for e.g. "notaBUILD.gn"? Changed
to "/BUILD.gn", since all the BUILD.gn files we're concerned with are under some
prefix.

+      not f.LocalPath().endswith(".gni")):
+    return False
+  return True
+
+def _FindIllegalAbsolutePathsInBuildFiles(input_api, package):
+  """Finds illegal absolute paths within the build files in
+     |input_api.AffectedFiles()| that are within |package| ("SDK" or "EDK").
+     An illegal absolute path within the SDK is one that is to the SDK itself
+     or a non-whitelisted external path. An illegal absolute path within the
+     EDK is one that is to the SDK or the EDK.
+     Returns any such references in a list of (file_path, line_number,
+     referenced_path) tuples."""
+  illegal_references = []
+  for f in input_api.AffectedFiles():
+    if not _IsBuildFileWithinPackage(f, package):

[viettrungluu, 2014/12/10 00:02:02]

Maybe the "correct" thing to do is to define a helper:

def _AffectedBuildFilesWithinPackage(input_api, package):
  return [f for f in input_api.AffectedFiles() if _IsBuildFileWithinPackage(f,
package)]

[blundell, 2014/12/10 16:13:06]

Done.

+      continue
+
+    for line_num, line in f.ChangedContents():
+      # Determine if this is a reference to an absolute path.
+      m = re.search(r'"(//[^"]*)"', line)
+      if not m:
+        continue
+      referenced_path = m.group(1)
+
+      # In the EDK, all external absolute paths are allowed.
+      if package == "EDK" and not referenced_path.startswith("//mojo"):
+        continue
+
+      # Determine if this is a whitelisted external path.
+      for whitelisted_path in SDK_WHITELISTED_EXTERNAL_PATHS:
+        if referenced_path.startswith(whitelisted_path):
+          continue
+
+      illegal_references.append((f.LocalPath(), line_num, referenced_path))
+
+  return illegal_references
+
+def _CheckNoIllegalAbsolutePathsInBuildFiles(input_api, output_api, package):
+  """Makes sure that the BUILD.gn files within |package| ("SDK" or "EDK") do not
+     reference the SDK/EDK via absolute paths, and do not reference disallowed
+     external dependencies."""
+  sdk_references = []
+  edk_references = []
+  external_deps_references = []
+
+  # Categorize any illegal references.
+  illegal_references = _FindIllegalAbsolutePathsInBuildFiles(input_api, package)
+  for build_file, line_num, referenced_path in illegal_references:
+    reference_string = "%s, line %d (%s)" % (build_file,
+                                             line_num,
+                                             referenced_path)
+    if referenced_path.startswith("//mojo/public"):
+      sdk_references.append(reference_string)
+    elif package == "SDK":
+      external_deps_references.append(reference_string)
+    else:
+      assert referenced_path.startswith("//mojo/edk")
+      edk_references.append(reference_string)
+
+  # Package up categorized illegal references into results.
+  results = []
+  if sdk_references:
+    results += [output_api.PresubmitError(

[viettrungluu, 2014/12/10 00:02:02]

results.append() instead?

[blundell, 2014/12/10 16:13:05]

results.extend() seems to be the canonical form here from looking at other
PRESUBMIT.py files. Changed throughout this file.

+        ("Found references to the SDK via absolute paths within %s buildfiles."
+         % package),
+        items=sdk_references)]
+
+  if external_deps_references:
+    assert package == "SDK"
+    results += [output_api.PresubmitError(

[viettrungluu, 2014/12/10 00:02:02]

" (etc.)

[blundell, 2014/12/10 16:13:05]

Done.

+        "Found disallowed external paths within SDK buildfiles.",
+        items=external_deps_references)]
+
+  if edk_references:
+    assert package == "EDK"
+    results += [output_api.PresubmitError(
+        "Found references to the EDK via absolute paths within EDK buildfiles.",
+        items=edk_references)]
+
+  return results
+
+def _CheckSourceSetsAreOfCorrectType(input_api, output_api, package):
+  """Makes sure that the BUILD.gn files always use the correct wrapper type for
+     |package|, which can be one of ["SDK", "EDK"], to construct source_set
+     targets."""
+  if package == "SDK":

[viettrungluu, 2014/12/10 00:02:02]

Similar suggestion as above.

[blundell, 2014/12/10 16:13:05]

Done.

+    required_source_set_type = "mojo_sdk_source_set"
+  elif package == "EDK":
+    required_source_set_type = "mojo_edk_source_set"
+  else:
+    assert 0, "unknown package: %s" % package
+
+  problems = []
+  for f in input_api.AffectedFiles():
+    if not _IsBuildFileWithinPackage(f, package):
+      continue
+
+    for line_num, line in f.ChangedContents():
+      m = re.search(r"[a-z_]*source_set\(", line)
+      if not m:
+        continue
+      source_set_type = m.group(0)[:-1]
+      if source_set_type == required_source_set_type:
+        continue
+      problems.append("%s, line %d" % (f.LocalPath(), line_num))
+
+  if not problems:
+    return []
+  return [output_api.PresubmitError(
+      "All source sets in the %s must be constructed via %s." % (
+           package, required_source_set_type),
+      items=problems)]
+
+def _CheckChangePylintsClean(input_api, output_api):
   # Additional python module paths (we're in src/mojo/); not everyone needs
   # them, but it's easiest to add them to everyone's path.
   # For ply and jinja2:
@@ -51,3 +184,26 @@ def CheckChangeOnUpload(input_api, output_api):
       input_api, output_api, extra_paths_list=pylint_extra_paths,
       black_list=temporary_black_list)
   return results
+
+def _CommonChecks(input_api, output_api):
+  """Checks common to both upload and commit."""
+  results = []
+  for package in ["SDK", "EDK"]:
+    results.extend(_CheckNoIllegalAbsolutePathsInBuildFiles(input_api,
+                                                            output_api,
+                                                            package))
+    results.extend(_CheckSourceSetsAreOfCorrectType(input_api,
+                                                    output_api,
+                                                    package))
+  return results
+
+def CheckChangeOnUpload(input_api, output_api):
+  results = []
+  results.extend(_CommonChecks(input_api, output_api))
+  results.extend(_CheckChangePylintsClean(input_api, output_api))
+  return results
+
+def CheckChangeOnCommit(input_api, output_api):
+  results = []
+  results.extend(_CommonChecks(input_api, output_api))
+  return results