Add presubmit check to //mojo/{public, edk} for BUILD.gn file flexibility.
This script checks for changes to the build files that would break the ability
to drop the mojo SDK EDK into a client repo at a location other than
//mojo/{public, edk} and have GN work:
- References to "//mojo/public" in the SDK (these should be relative paths).
- References to other absolute paths in the SDK (these shouldn't be there at
all).
- References to "//mojo/public" and "//mojo/edk" in the EDK (these should be
relative paths).
- Source set targets that are not constructed via the appropriate wrapper
(mojo_sdk_source_set or mojo_edk_source_set targets as appropriate).
R=jamesr@chromium.org
Review URL: https://codereview.chromium.org/782743002
diff --git a/mojo/PRESUBMIT.py b/mojo/PRESUBMIT.py
index 557883e..3d85422 100644
--- a/mojo/PRESUBMIT.py
+++ b/mojo/PRESUBMIT.py
@@ -9,8 +9,170 @@
"""
import os.path
+import re
-def CheckChangeOnUpload(input_api, output_api):
+_SDK_WHITELISTED_EXTERNAL_PATHS = [
+ "//testing/gtest",
+ "//third_party/cython",
+ "//third_party/khronos",
+]
+
+_PACKAGE_PATH_PREFIXES = {"SDK": "mojo/public/",
+ "EDK": "mojo/edk/"}
+
+_PACKAGE_SOURCE_SET_TYPES = {"SDK": "mojo_sdk_source_set",
+ "EDK": "mojo_edk_source_set"}
+
+_ILLEGAL_EXTERNAL_PATH_WARNING_MESSAGE = \
+ "Found disallowed external paths within SDK buildfiles."
+
+_ILLEGAL_EDK_ABSOLUTE_PATH_WARNING_MESSAGE = \
+ "Found references to the EDK via absolute paths within EDK buildfiles.",
+
+_ILLEGAL_SDK_ABSOLUTE_PATH_WARNING_MESSAGE_TEMPLATE = \
+ "Found references to the SDK via absolute paths within %s buildfiles."
+
+_ILLEGAL_SDK_ABSOLUTE_PATH_WARNING_MESSAGES = {
+ "SDK": _ILLEGAL_SDK_ABSOLUTE_PATH_WARNING_MESSAGE_TEMPLATE % "SDK",
+ "EDK": _ILLEGAL_SDK_ABSOLUTE_PATH_WARNING_MESSAGE_TEMPLATE % "EDK",
+}
+
+_INCORRECT_SOURCE_SET_TYPE_WARNING_MESSAGE_TEMPLATE = \
+ "All source sets in the %s must be constructed via %s."
+
+_INCORRECT_SOURCE_SET_TYPE_WARNING_MESSAGES = {
+ "SDK": _INCORRECT_SOURCE_SET_TYPE_WARNING_MESSAGE_TEMPLATE
+ % ("SDK", _PACKAGE_SOURCE_SET_TYPES["SDK"]),
+ "EDK": _INCORRECT_SOURCE_SET_TYPE_WARNING_MESSAGE_TEMPLATE
+ % ("EDK", _PACKAGE_SOURCE_SET_TYPES["EDK"]),
+}
+
+def _IsBuildFileWithinPackage(f, package):
+ """Returns whether |f| specifies a GN build file within |package| ("SDK" or
+ "EDK")."""
+ assert package in _PACKAGE_PATH_PREFIXES
+ package_path_prefix = _PACKAGE_PATH_PREFIXES[package]
+
+ if not f.LocalPath().startswith(package_path_prefix):
+ return False
+ if (not f.LocalPath().endswith("/BUILD.gn") and
+ not f.LocalPath().endswith(".gni")):
+ return False
+ return True
+
+def _AffectedBuildFilesWithinPackage(input_api, package):
+ """Returns all the affected build files within |package| ("SDK" or "EDK")."""
+ return [f for f in input_api.AffectedFiles()
+ if _IsBuildFileWithinPackage(f, package)]
+
+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 _AffectedBuildFilesWithinPackage(input_api, package):
+ 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.
+ if referenced_path in _SDK_WHITELISTED_EXTERNAL_PATHS:
+ continue
+
+ illegal_references.append((f.LocalPath(), line_num, referenced_path))
+
+ return illegal_references
+
+def _PathReferenceInBuildFileWarningItem(build_file, line_num, referenced_path):
+ """Returns a string expressing a warning item that |referenced_path| is
+ referenced at |line_num| in |build_file|."""
+ return "%s, line %d (%s)" % (build_file, line_num, referenced_path)
+
+def _IncorrectSourceSetTypeWarningItem(build_file, line_num):
+ """Returns a string expressing that the error occurs at |line_num| in
+ |build_file|."""
+ return "%s, line %d" % (build_file, line_num)
+
+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 = _PathReferenceInBuildFileWarningItem(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.extend([output_api.PresubmitError(
+ _ILLEGAL_SDK_ABSOLUTE_PATH_WARNING_MESSAGES[package],
+ items=sdk_references)])
+
+ if external_deps_references:
+ assert package == "SDK"
+ results.extend([output_api.PresubmitError(
+ _ILLEGAL_EXTERNAL_PATH_WARNING_MESSAGE,
+ items=external_deps_references)])
+
+ if edk_references:
+ assert package == "EDK"
+ results.extend([output_api.PresubmitError(
+ _ILLEGAL_EDK_ABSOLUTE_PATH_WARNING_MESSAGE,
+ 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."""
+ assert package in _PACKAGE_SOURCE_SET_TYPES
+ required_source_set_type = _PACKAGE_SOURCE_SET_TYPES[package]
+
+ problems = []
+ for f in _AffectedBuildFilesWithinPackage(input_api, package):
+ 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(_IncorrectSourceSetTypeWarningItem(f.LocalPath(),
+ line_num))
+
+ if not problems:
+ return []
+ return [output_api.PresubmitError(
+ _INCORRECT_SOURCE_SET_TYPE_WARNING_MESSAGES[package],
+ 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:
@@ -47,7 +209,36 @@
mojo_roll_tools_path,
mopy_path,
]
- results += input_api.canned_checks.RunPylint(
+ results.extend(input_api.canned_checks.RunPylint(
input_api, output_api, extra_paths_list=pylint_extra_paths,
- black_list=temporary_black_list)
+ black_list=temporary_black_list))
+ return results
+
+def _BuildFileChecks(input_api, output_api):
+ """Performs checks on SDK and EDK buildfiles."""
+ 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 _CommonChecks(input_api, output_api):
+ """Checks common to both upload and commit."""
+ results = []
+ results.extend(_BuildFileChecks(input_api, output_api))
+ 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
diff --git a/mojo/PRESUBMIT_test.py b/mojo/PRESUBMIT_test.py
new file mode 100755
index 0000000..5245b68
--- /dev/null
+++ b/mojo/PRESUBMIT_test.py
@@ -0,0 +1,243 @@
+#!/usr/bin/env python
+# Copyright 2014 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import os
+import sys
+import unittest
+
+import PRESUBMIT
+
+sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+from PRESUBMIT_test_mocks import MockFile
+from PRESUBMIT_test_mocks import MockInputApi, MockOutputApi
+
+_SDK_BUILD_FILE = 'mojo/public/some/path/BUILD.gn'
+_EDK_BUILD_FILE = 'mojo/edk/some/path/BUILD.gn'
+_IRRELEVANT_BUILD_FILE = 'mojo/foo/some/path/BUILD.gn'
+
+class AbsoluteReferencesInBuildFilesTest(unittest.TestCase):
+ """Tests the checking for illegal absolute paths within SDK/EDK buildfiles.
+ """
+ def setUp(self):
+ self.sdk_absolute_path = '//mojo/public/some/absolute/path'
+ self.sdk_relative_path = 'mojo/public/some/relative/path'
+ self.edk_absolute_path = '//mojo/edk/some/absolute/path'
+ self.edk_relative_path = 'mojo/edk/some/relative/path'
+ self.whitelisted_external_path = '//testing/gtest'
+ self.non_whitelisted_external_path = '//base'
+
+ def inputApiContainingFileWithPaths(self, filename, paths):
+ """Returns a MockInputApi object with a single file having |filename| as
+ its name and |paths| as its contents, with each path being wrapped in a
+ pair of double-quotes to match the syntax for strings within BUILD.gn
+ files."""
+ contents = [ '"%s"' % path for path in paths ]
+ mock_file = MockFile(filename, contents)
+ mock_input_api = MockInputApi()
+ mock_input_api.files.append(mock_file)
+ return mock_input_api
+
+ def checkWarningWithSingleItem(self,
+ warning,
+ expected_message,
+ build_file,
+ line_num,
+ referenced_path):
+ """Checks that |warning| has a message of |expected_message| and a single
+ item whose contents are the absolute path warning item for
+ (build_file, line_num, referenced_path)."""
+ self.assertEqual(expected_message, warning.message)
+ self.assertEqual(1, len(warning.items))
+ expected_item = PRESUBMIT._PathReferenceInBuildFileWarningItem(
+ build_file, line_num, referenced_path)
+ self.assertEqual(expected_item, warning.items[0])
+
+ def checkSDKAbsolutePathWarningWithSingleItem(self,
+ warning,
+ package,
+ build_file,
+ line_num,
+ referenced_path):
+ """Checks that |warning| has the message for an absolute SDK path within
+ |package| and a single item whose contents are the absolute path warning
+ item for (build_file, line_num, referenced_path)."""
+ expected_message = \
+ PRESUBMIT._ILLEGAL_SDK_ABSOLUTE_PATH_WARNING_MESSAGES[package]
+ self.checkWarningWithSingleItem(warning,
+ expected_message,
+ build_file,
+ line_num,
+ referenced_path)
+
+ def testAbsoluteSDKReferenceInSDKBuildFile(self):
+ """Tests that an absolute SDK path within an SDK buildfile is flagged."""
+ mock_input_api = self.inputApiContainingFileWithPaths(
+ _SDK_BUILD_FILE,
+ [ self.sdk_relative_path, self.sdk_absolute_path ])
+ warnings = PRESUBMIT._BuildFileChecks(mock_input_api, MockOutputApi())
+
+ self.assertEqual(1, len(warnings))
+ self.checkSDKAbsolutePathWarningWithSingleItem(warnings[0],
+ 'SDK',
+ _SDK_BUILD_FILE,
+ 2,
+ self.sdk_absolute_path)
+
+ def testExternalReferenceInSDKBuildFile(self):
+ """Tests that an illegal external path in an SDK buildfile is flagged."""
+ mock_input_api = self.inputApiContainingFileWithPaths(
+ _SDK_BUILD_FILE,
+ [ self.non_whitelisted_external_path, self.whitelisted_external_path ])
+ warnings = PRESUBMIT._BuildFileChecks(mock_input_api, MockOutputApi())
+
+ self.assertEqual(1, len(warnings))
+ expected_message = PRESUBMIT._ILLEGAL_EXTERNAL_PATH_WARNING_MESSAGE
+ self.checkWarningWithSingleItem(warnings[0],
+ expected_message,
+ _SDK_BUILD_FILE,
+ 1,
+ self.non_whitelisted_external_path)
+
+ def testAbsoluteEDKReferenceInSDKBuildFile(self):
+ """Tests that an absolute EDK path in an SDK buildfile is flagged."""
+ mock_input_api = self.inputApiContainingFileWithPaths(
+ _SDK_BUILD_FILE,
+ [ self.edk_absolute_path ])
+ warnings = PRESUBMIT._BuildFileChecks(mock_input_api, MockOutputApi())
+
+ self.assertEqual(1, len(warnings))
+ expected_message = PRESUBMIT._ILLEGAL_EXTERNAL_PATH_WARNING_MESSAGE
+ self.checkWarningWithSingleItem(warnings[0],
+ expected_message,
+ _SDK_BUILD_FILE,
+ 1,
+ self.edk_absolute_path)
+
+ def testAbsoluteSDKReferenceInEDKBuildFile(self):
+ """Tests that an absolute SDK path within an EDK buildfile is flagged."""
+ mock_input_api = self.inputApiContainingFileWithPaths(
+ _EDK_BUILD_FILE,
+ [ self.sdk_relative_path, self.sdk_absolute_path ])
+ warnings = PRESUBMIT._BuildFileChecks(mock_input_api, MockOutputApi())
+ self.assertEqual(1, len(warnings))
+ self.checkSDKAbsolutePathWarningWithSingleItem(warnings[0],
+ 'EDK',
+ _EDK_BUILD_FILE,
+ 2,
+ self.sdk_absolute_path)
+
+ def testAbsoluteEDKReferenceInEDKBuildFile(self):
+ """Tests that an absolute EDK path in an EDK buildfile is flagged."""
+ mock_input_api = self.inputApiContainingFileWithPaths(
+ _EDK_BUILD_FILE,
+ [ self.edk_absolute_path, self.edk_relative_path ])
+ warnings = PRESUBMIT._BuildFileChecks(mock_input_api, MockOutputApi())
+
+ self.assertEqual(1, len(warnings))
+ expected_message = PRESUBMIT._ILLEGAL_EDK_ABSOLUTE_PATH_WARNING_MESSAGE
+ self.checkWarningWithSingleItem(warnings[0],
+ expected_message,
+ _EDK_BUILD_FILE,
+ 1,
+ self.edk_absolute_path)
+
+ def testExternalReferenceInEDKBuildFile(self):
+ """Tests that an external path in an EDK buildfile is not flagged."""
+ mock_input_api = self.inputApiContainingFileWithPaths(
+ _EDK_BUILD_FILE,
+ [ self.non_whitelisted_external_path, self.whitelisted_external_path ])
+ warnings = PRESUBMIT._BuildFileChecks(mock_input_api, MockOutputApi())
+ self.assertEqual(0, len(warnings))
+
+ def testIrrelevantBuildFile(self):
+ """Tests that nothing is flagged in a non SDK/EDK buildfile."""
+ mock_input_api = self.inputApiContainingFileWithPaths(
+ _IRRELEVANT_BUILD_FILE,
+ [ self.sdk_absolute_path,
+ self.sdk_relative_path,
+ self.edk_absolute_path,
+ self.edk_relative_path,
+ self.non_whitelisted_external_path,
+ self.whitelisted_external_path ])
+ warnings = PRESUBMIT._BuildFileChecks(mock_input_api, MockOutputApi())
+ self.assertEqual(0, len(warnings))
+
+class SourceSetTypesInBuildFilesTest(unittest.TestCase):
+ """Tests checking of correct source set types within SDK/EDK buildfiles."""
+
+ def inputApiContainingFileWithSourceSets(self, filename, source_sets):
+ """Returns a MockInputApi object containing a single file having |filename|
+ as its name and |source_sets| as its contents."""
+ mock_file = MockFile(filename, source_sets)
+ mock_input_api = MockInputApi()
+ mock_input_api.files.append(mock_file)
+ return mock_input_api
+
+ def checkWarningWithSingleItem(self,
+ warning,
+ package,
+ build_file,
+ line_num):
+ """Checks that warning has the expected incorrect source set type message
+ for |package| and a single item whose contents are the incorrect source
+ set type item for (build_file, line_num)."""
+ expected_message = \
+ PRESUBMIT._INCORRECT_SOURCE_SET_TYPE_WARNING_MESSAGES[package]
+ self.assertEqual(expected_message, warning.message)
+ self.assertEqual(1, len(warning.items))
+ expected_item = PRESUBMIT._IncorrectSourceSetTypeWarningItem(
+ build_file, line_num)
+ self.assertEqual(expected_item, warning.items[0])
+
+ def testNakedSourceSetInSDKBuildFile(self):
+ """Tests that a source_set within an SDK buildfile is flagged."""
+ mock_input_api = self.inputApiContainingFileWithSourceSets(
+ _SDK_BUILD_FILE,
+ [ 'mojo_sdk_source_set(', 'source_set(' ])
+ warnings = PRESUBMIT._BuildFileChecks(mock_input_api, MockOutputApi())
+
+ self.assertEqual(1, len(warnings))
+ self.checkWarningWithSingleItem(warnings[0], 'SDK', _SDK_BUILD_FILE, 2)
+
+ def testEDKSourceSetInSDKBuildFile(self):
+ """Tests that a mojo_edk_source_set within an SDK buildfile is flagged."""
+ mock_input_api = self.inputApiContainingFileWithSourceSets(
+ _SDK_BUILD_FILE,
+ [ 'mojo_sdk_source_set(', 'mojo_edk_source_set(' ])
+ warnings = PRESUBMIT._BuildFileChecks(mock_input_api, MockOutputApi())
+
+ self.assertEqual(1, len(warnings))
+ self.checkWarningWithSingleItem(warnings[0], 'SDK', _SDK_BUILD_FILE, 2)
+
+ def testNakedSourceSetInEDKBuildFile(self):
+ """Tests that a source_set within an EDK buildfile is flagged."""
+ mock_input_api = self.inputApiContainingFileWithSourceSets(
+ _EDK_BUILD_FILE,
+ [ 'source_set(', 'mojo_edk_source_set(' ])
+ warnings = PRESUBMIT._BuildFileChecks(mock_input_api, MockOutputApi())
+
+ self.assertEqual(1, len(warnings))
+ self.checkWarningWithSingleItem(warnings[0], 'EDK', _EDK_BUILD_FILE, 1)
+
+ def testSDKSourceSetInEDKBuildFile(self):
+ """Tests that a mojo_sdk_source_set within an EDK buildfile is flagged."""
+ mock_input_api = self.inputApiContainingFileWithSourceSets(
+ _EDK_BUILD_FILE,
+ [ 'mojo_sdk_source_set(', 'mojo_edk_source_set(' ])
+ warnings = PRESUBMIT._BuildFileChecks(mock_input_api, MockOutputApi())
+
+ self.assertEqual(1, len(warnings))
+ self.checkWarningWithSingleItem(warnings[0], 'EDK', _EDK_BUILD_FILE, 1)
+
+ def testIrrelevantBuildFile(self):
+ """Tests that a source_set in a non-SDK/EDK buildfile isn't flagged."""
+ mock_input_api = self.inputApiContainingFileWithSourceSets(
+ _IRRELEVANT_BUILD_FILE,
+ [ 'source_set(' ])
+ warnings = PRESUBMIT._BuildFileChecks(mock_input_api, MockOutputApi())
+ self.assertEqual(0, len(warnings))
+
+if __name__ == '__main__':
+ unittest.main()
