Add checks on services' public BUILD.gn files to //mojo/PRESUBMIT.py
This CL extends //mojo/PRESUBMIT.py with the following checks on services'
public BUILD.gn files:
- No references to the Mojo SDK via absolute paths
- No references to other services' buildfiles via absolute paths
- No other absolute-path deps
R=qsr@chromium.org
Review URL: https://codereview.chromium.org/886453002
diff --git a/mojo/PRESUBMIT.py b/mojo/PRESUBMIT.py
index d9b28af..1933e6e 100644
--- a/mojo/PRESUBMIT.py
+++ b/mojo/PRESUBMIT.py
@@ -11,23 +11,32 @@
import os.path
import re
-_SDK_WHITELISTED_EXTERNAL_PATHS = [
- "//testing/gtest",
- "//third_party/cython",
- "//third_party/khronos",
-]
+# NOTE: The EDK allows all external paths, so doesn't need a whitelist.
+_PACKAGE_WHITELISTED_EXTERNAL_PATHS = {
+ "SDK": ["//testing/gtest",
+ "//third_party/cython",
+ "//third_party/khronos"],
+ "services": [ "//testing/gtest" ]
+}
+
_PACKAGE_PATH_PREFIXES = {"SDK": "mojo/public/",
- "EDK": "mojo/edk/"}
+ "EDK": "mojo/edk/",
+ "services": "mojo/services"}
# TODO(etiennej): python_binary_source_set added due to crbug.com/443147
_PACKAGE_SOURCE_SET_TYPES = {"SDK": ["mojo_sdk_source_set",
"python_binary_source_set"],
- "EDK": ["mojo_edk_source_set"]}
+ "EDK": ["mojo_edk_source_set"],
+ "services": ["mojo_sdk_source_set"]}
_ILLEGAL_EXTERNAL_PATH_WARNING_MESSAGE = \
"Found disallowed external paths within SDK buildfiles."
+_ILLEGAL_SERVICES_ABSOLUTE_PATH_WARNING_MESSAGE = \
+ "Found references to services' public buildfiles via absolute paths " \
+ "within services' public buildfiles.",
+
_ILLEGAL_EDK_ABSOLUTE_PATH_WARNING_MESSAGE = \
"Found references to the EDK via absolute paths within EDK buildfiles.",
@@ -37,21 +46,24 @@
_ILLEGAL_SDK_ABSOLUTE_PATH_WARNING_MESSAGES = {
"SDK": _ILLEGAL_SDK_ABSOLUTE_PATH_WARNING_MESSAGE_TEMPLATE % "SDK",
"EDK": _ILLEGAL_SDK_ABSOLUTE_PATH_WARNING_MESSAGE_TEMPLATE % "EDK",
+ "services": _ILLEGAL_SDK_ABSOLUTE_PATH_WARNING_MESSAGE_TEMPLATE
+ % "services' public",
}
_INCORRECT_SOURCE_SET_TYPE_WARNING_MESSAGE_TEMPLATE = \
- "All source sets in the %s must be constructed via %s."
+ "All source sets in %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"]),
+ % ("the SDK", _PACKAGE_SOURCE_SET_TYPES["SDK"]),
"EDK": _INCORRECT_SOURCE_SET_TYPE_WARNING_MESSAGE_TEMPLATE
- % ("EDK", _PACKAGE_SOURCE_SET_TYPES["EDK"]),
+ % ("the EDK", _PACKAGE_SOURCE_SET_TYPES["EDK"]),
+ "services": _INCORRECT_SOURCE_SET_TYPE_WARNING_MESSAGE_TEMPLATE
+ % ("services' client libs", _PACKAGE_SOURCE_SET_TYPES["services"]),
}
def _IsBuildFileWithinPackage(f, package):
- """Returns whether |f| specifies a GN build file within |package| ("SDK" or
- "EDK")."""
+ """Returns whether |f| specifies a GN build file within |package|."""
assert package in _PACKAGE_PATH_PREFIXES
package_path_prefix = _PACKAGE_PATH_PREFIXES[package]
@@ -63,16 +75,16 @@
return True
def _AffectedBuildFilesWithinPackage(input_api, package):
- """Returns all the affected build files within |package| ("SDK" or "EDK")."""
+ """Returns all the affected build files within |package|."""
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.
+ |input_api.AffectedFiles()| that are within |package|.
+ An illegal absolute path within the SDK or a service's 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 = []
@@ -84,13 +96,14 @@
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
+ if not referenced_path.startswith("//mojo"):
+ # In the EDK, all external absolute paths are allowed.
+ if package == "EDK":
+ continue
- # Determine if this is a whitelisted external path.
- if referenced_path in _SDK_WHITELISTED_EXTERNAL_PATHS:
- continue
+ # Determine if this is a whitelisted external path.
+ if referenced_path in _PACKAGE_WHITELISTED_EXTERNAL_PATHS[package]:
+ continue
illegal_references.append((f.LocalPath(), line_num, referenced_path))
@@ -107,12 +120,13 @@
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."""
+ """Makes sure that the BUILD.gn files within |package| do not reference the
+ SDK/EDK via absolute paths, and do not reference disallowed external
+ dependencies."""
sdk_references = []
edk_references = []
external_deps_references = []
+ services_references = []
# Categorize any illegal references.
illegal_references = _FindIllegalAbsolutePathsInBuildFiles(input_api, package)
@@ -124,6 +138,11 @@
sdk_references.append(reference_string)
elif package == "SDK":
external_deps_references.append(reference_string)
+ elif package == "services":
+ if referenced_path.startswith("//mojo/services"):
+ services_references.append(reference_string)
+ else:
+ external_deps_references.append(reference_string)
elif referenced_path.startswith("//mojo/edk"):
edk_references.append(reference_string)
@@ -135,11 +154,17 @@
items=sdk_references)])
if external_deps_references:
- assert package == "SDK"
+ assert package == "SDK" or package == "services"
results.extend([output_api.PresubmitError(
_ILLEGAL_EXTERNAL_PATH_WARNING_MESSAGE,
items=external_deps_references)])
+ if services_references:
+ assert package == "services"
+ results.extend([output_api.PresubmitError(
+ _ILLEGAL_SERVICES_ABSOLUTE_PATH_WARNING_MESSAGE,
+ items=services_references)])
+
if edk_references:
assert package == "EDK"
results.extend([output_api.PresubmitError(
@@ -215,9 +240,9 @@
return results
def _BuildFileChecks(input_api, output_api):
- """Performs checks on SDK and EDK buildfiles."""
+ """Performs checks on SDK, EDK, and services' public buildfiles."""
results = []
- for package in ["SDK", "EDK"]:
+ for package in ["SDK", "EDK", "services"]:
results.extend(_CheckNoIllegalAbsolutePathsInBuildFiles(input_api,
output_api,
package))
diff --git a/mojo/PRESUBMIT_test.py b/mojo/PRESUBMIT_test.py
index 5a95d0e..3e2443c 100755
--- a/mojo/PRESUBMIT_test.py
+++ b/mojo/PRESUBMIT_test.py
@@ -15,11 +15,13 @@
_SDK_BUILD_FILE = 'mojo/public/some/path/BUILD.gn'
_EDK_BUILD_FILE = 'mojo/edk/some/path/BUILD.gn'
+_SERVICE_BUILD_FILE = 'mojo/services/some_service/public/BUILD.gn'
_IRRELEVANT_BUILD_FILE = 'mojo/foo/some/path/BUILD.gn'
_PACKAGE_BUILDFILES = {
'SDK' : _SDK_BUILD_FILE,
'EDK' : _EDK_BUILD_FILE,
+ 'services' : _SERVICE_BUILD_FILE
}
class AbsoluteReferencesInBuildFilesTest(unittest.TestCase):
@@ -30,6 +32,8 @@
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.service_absolute_path = '//mojo/services/some/service'
+ self.service_relative_path = '../../../some/service'
self.whitelisted_external_path = '//testing/gtest'
self.non_whitelisted_external_path = '//base'
@@ -158,6 +162,33 @@
"""Tests that an external path in an EDK buildfile is not flagged."""
self._testExternalReferenceInPackage('EDK')
+ def testAbsoluteSDKReferenceInServiceBuildFile(self):
+ """Tests that an absolute SDK path within a service's public buildfile is
+ flagged."""
+ self._testAbsoluteSDKReferenceInPackage("services")
+
+ def testAbsoluteServiceReferenceInServiceBuildFile(self):
+ """Tests that an absolute path to a service within a service's public
+ buildfile is flagged."""
+ mock_input_api = self.inputApiContainingFileWithPaths(
+ _SERVICE_BUILD_FILE,
+ [ self.service_relative_path, self.service_absolute_path ])
+ warnings = PRESUBMIT._BuildFileChecks(mock_input_api, MockOutputApi())
+
+ self.assertEqual(1, len(warnings))
+ expected_message = \
+ PRESUBMIT._ILLEGAL_SERVICES_ABSOLUTE_PATH_WARNING_MESSAGE
+ self.checkWarningWithSingleItem(warnings[0],
+ expected_message,
+ _SERVICE_BUILD_FILE,
+ 2,
+ self.service_absolute_path)
+
+ def testExternalReferenceInServiceBuildFile(self):
+ """Tests that an illegal external path in a service's buildfile is flagged
+ ."""
+ self._testExternalReferenceInPackage("services")
+
def testIrrelevantBuildFile(self):
"""Tests that nothing is flagged in a non SDK/EDK buildfile."""
mock_input_api = self.inputApiContainingFileWithPaths(
@@ -257,6 +288,16 @@
"""Tests that a mojo_sdk_source_set within an EDK buildfile is flagged."""
self._testWrongTypeOfWrapperSourceSetInPackage('EDK')
+ def testNakedSourceSetInServiceBuildFile(self):
+ """Tests that a source_set within a service's public buildfile is flagged.
+ """
+ self._testNakedSourceSetInPackage("services")
+
+ def testEDKSourceSetInServiceBuildFile(self):
+ """Tests that a mojo_edk_source_set within a service's public buildfile is
+ flagged."""
+ self._testWrongTypeOfWrapperSourceSetInPackage("services")
+
def testIrrelevantBuildFile(self):
"""Tests that a source_set in a non-SDK/EDK buildfile isn't flagged."""
mock_input_api = self.inputApiContainingFileWithSourceSets(
