Simplify PRESUBMIT.py and check_package_boundaries.py
Bug: webrtc:6954
Change-Id: Ia93eb8cc8a13bdcba5217fd8d52b72defa108b2f
Reviewed-on: https://webrtc-review.googlesource.com/6021
Commit-Queue: Oleh Prypin <oprypin@webrtc.org>
Reviewed-by: Henrik Kjellander <kjellander@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20158}diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index c8ac165..ff46605 100755
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -9,9 +9,9 @@
import json
import os
import re
-import subprocess
import sys
from collections import defaultdict
+from contextlib import contextmanager
# Files and directories that are *skipped* by cpplint in the presubmit script.
@@ -107,16 +107,15 @@
FILE_PATH_RE = re.compile(r'"(?P<file_path>(\w|\/)+)(?P<extension>\.\w+)"')
-def _RunCommand(command, cwd):
- """Runs a command and returns the output from that command."""
- p = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE,
- cwd=cwd)
- stdout = p.stdout.read()
- stderr = p.stderr.read()
- p.wait()
- p.stdout.close()
- p.stderr.close()
- return p.returncode, stdout, stderr
+@contextmanager
+def _AddToPath(*paths):
+ original_sys_path = sys.path
+ sys.path.extend(paths)
+ try:
+ yield
+ finally:
+ # Restore sys.path to what it was before.
+ sys.path = original_sys_path
def VerifyNativeApiHeadersListIsValid(input_api, output_api):
@@ -379,15 +378,15 @@
def CheckNoPackageBoundaryViolations(input_api, gn_files, output_api):
cwd = input_api.PresubmitLocalPath()
- script_path = os.path.join('tools_webrtc', 'presubmit_checks_lib',
- 'check_package_boundaries.py')
- command = [sys.executable, script_path, cwd]
- command += [os.path.join(cwd, gn_file.LocalPath()) for gn_file in gn_files]
- returncode, _, stderr = _RunCommand(command, cwd)
- if returncode:
+ with _AddToPath(input_api.os_path.join(
+ cwd, 'tools_webrtc', 'presubmit_checks_lib')):
+ from check_package_boundaries import CheckPackageBoundaries
+ build_files = [os.path.join(cwd, gn_file.LocalPath()) for gn_file in gn_files]
+ errors = CheckPackageBoundaries(cwd, build_files)[:5]
+ if errors:
return [output_api.PresubmitError(
- 'There are package boundary violations in the following GN files:\n\n'
- '%s' % stderr)]
+ 'There are package boundary violations in the following GN files:',
+ long_text='\n\n'.join(str(err) for err in errors))]
return []
def CheckGnChanges(input_api, output_api):
@@ -417,21 +416,16 @@
# We need to wait until we have an input_api object and use this
# roundabout construct to import checkdeps because this file is
# eval-ed and thus doesn't have __file__.
- original_sys_path = sys.path
- try:
- checkdeps_path = input_api.os_path.join(input_api.PresubmitLocalPath(),
- 'buildtools', 'checkdeps')
- if not os.path.exists(checkdeps_path):
- return [output_api.PresubmitError(
- 'Cannot find checkdeps at %s\nHave you run "gclient sync" to '
- 'download Chromium and setup the symlinks?' % checkdeps_path)]
- sys.path.append(checkdeps_path)
+ checkdeps_path = input_api.os_path.join(input_api.PresubmitLocalPath(),
+ 'buildtools', 'checkdeps')
+ if not os.path.exists(checkdeps_path):
+ return [output_api.PresubmitError(
+ 'Cannot find checkdeps at %s\nHave you run "gclient sync" to '
+ 'download all the DEPS entries?' % checkdeps_path)]
+ with _AddToPath(checkdeps_path):
import checkdeps
from cpp_checker import CppChecker
from rules import Rule
- finally:
- # Restore sys.path to what it was before.
- sys.path = original_sys_path
added_includes = []
for f in input_api.AffectedFiles():
@@ -708,15 +702,10 @@
error_msg = """Header file {} is not listed in any GN target.
Please create a target or add it to an existing one in {}"""
results = []
- original_sys_path = sys.path
- try:
- sys.path = sys.path + [input_api.os_path.join(
- input_api.PresubmitLocalPath(), 'tools_webrtc', 'presubmit_checks_lib')]
+ with _AddToPath(input_api.os_path.join(
+ input_api.PresubmitLocalPath(), 'tools_webrtc', 'presubmit_checks_lib')):
from check_orphan_headers import GetBuildGnPathFromFilePath
from check_orphan_headers import IsHeaderInBuildGn
- finally:
- # Restore sys.path to what it was before.
- sys.path = original_sys_path
for f in input_api.AffectedSourceFiles(input_api.FilterSourceFile):
if f.LocalPath().endswith('.h') and f.Action() == 'A':
diff --git a/tools_webrtc/presubmit_checks_lib/check_package_boundaries.py b/tools_webrtc/presubmit_checks_lib/check_package_boundaries.py
index aabcd81..4b39bc5 100644
--- a/tools_webrtc/presubmit_checks_lib/check_package_boundaries.py
+++ b/tools_webrtc/presubmit_checks_lib/check_package_boundaries.py
@@ -9,15 +9,12 @@
# be found in the AUTHORS file in the root of the source tree.
import argparse
-import logging
+import collections
import os
import re
import sys
-DISPLAY_LEVEL = 1
-IGNORE_LEVEL = 0
-
# TARGET_RE matches a GN target, and extracts the target name and the contents.
TARGET_RE = re.compile(r'\d+\$(?P<indent>\s*)\w+\("(?P<target_name>\w+)"\) {'
r'(?P<target_contents>.*?)'
@@ -28,27 +25,16 @@
SOURCES_RE = re.compile(r'sources \+?= \[(?P<sources>.*?)\]',
re.MULTILINE | re.DOTALL)
-LOG_FORMAT = '%(message)s'
-ERROR_MESSAGE = ("{}:{} in target '{}':\n"
- " Source file '{}'\n"
- " crosses boundary of package '{}'.\n")
+ERROR_MESSAGE = ("{build_file_path}:{line_number} in target '{target_name}':\n"
+ " Source file '{source_file}'\n"
+ " crosses boundary of package '{subpackage}'.")
-class Logger(object):
- def __init__(self, messages_left=None):
- self.log_level = DISPLAY_LEVEL
- self.messages_left = messages_left
-
- def Log(self, build_file_path, line_number, target_name, source_file,
- subpackage):
- if self.messages_left is not None:
- if not self.messages_left:
- self.log_level = IGNORE_LEVEL
- else:
- self.messages_left -= 1
- message = ERROR_MESSAGE.format(build_file_path, line_number, target_name,
- source_file, subpackage)
- logging.log(self.log_level, message)
+class PackageBoundaryViolation(
+ collections.namedtuple('PackageBoundaryViolation',
+ 'build_file_path line_number target_name source_file subpackage')):
+ def __str__(self):
+ return ERROR_MESSAGE.format(**self._asdict())
def _BuildSubpackagesPattern(packages, query):
@@ -70,12 +56,11 @@
for line_number, line in enumerate(f, 1))
-def _CheckBuildFile(build_file_path, packages, logger):
- """Iterates oven all the targets of the given BUILD.gn file, and verifies that
+def _CheckBuildFile(build_file_path, packages):
+ """Iterates over all the targets of the given BUILD.gn file, and verifies that
the source files referenced by it don't belong to any of it's subpackages.
- Returns True if a package boundary violation was found.
+ Returns an iterator over PackageBoundaryViolations for this package.
"""
- found_violations = False
package = os.path.dirname(build_file_path)
subpackages_re = _BuildSubpackagesPattern(packages, package)
@@ -90,14 +75,11 @@
source_file = subpackages_match.group('source_file')
line_number = subpackages_match.group('line_number')
if subpackage:
- found_violations = True
- logger.Log(build_file_path, line_number, target_name, source_file,
- subpackage)
-
- return found_violations
+ yield PackageBoundaryViolation(build_file_path, line_number,
+ target_name, source_file, subpackage)
-def CheckPackageBoundaries(root_dir, logger, build_files=None):
+def CheckPackageBoundaries(root_dir, build_files=None):
packages = [root for root, _, files in os.walk(root_dir)
if 'BUILD.gn' in files]
@@ -107,11 +89,13 @@
else:
build_files = [os.path.join(package, 'BUILD.gn') for package in packages]
- return any([_CheckBuildFile(build_file_path, packages, logger)
- for build_file_path in build_files])
+ messages = []
+ for build_file_path in build_files:
+ messages.extend(_CheckBuildFile(build_file_path, packages))
+ return messages
-def main():
+def main(argv):
parser = argparse.ArgumentParser(
description='Script that checks package boundary violations in GN '
'build files.')
@@ -127,14 +111,18 @@
help='If set, the maximum number of violations to be '
'displayed.')
- args = parser.parse_args()
+ args = parser.parse_args(argv)
- logging.basicConfig(format=LOG_FORMAT)
- logging.getLogger().setLevel(DISPLAY_LEVEL)
- logger = Logger(args.max_messages)
+ messages = CheckPackageBoundaries(args.root_dir, args.build_files)
+ messages = messages[:args.max_messages]
- return CheckPackageBoundaries(args.root_dir, logger, args.build_files)
+ for i, message in enumerate(messages):
+ if i > 0:
+ print
+ print message
+
+ return bool(messages)
if __name__ == '__main__':
- sys.exit(main())
+ sys.exit(main(sys.argv[1:]))
diff --git a/tools_webrtc/presubmit_checks_lib/check_package_boundaries_test.py b/tools_webrtc/presubmit_checks_lib/check_package_boundaries_test.py
index 1acdb10..7a5874c 100755
--- a/tools_webrtc/presubmit_checks_lib/check_package_boundaries_test.py
+++ b/tools_webrtc/presubmit_checks_lib/check_package_boundaries_test.py
@@ -25,54 +25,46 @@
return ast.literal_eval(f.read())
-class Logger(object):
- def __init__(self, test_dir):
- self.messages = []
- self.test_dir = test_dir
-
- def Log(self, build_file_path, line_number, target_name, source_file,
- subpackage):
- build_file_path = os.path.relpath(build_file_path, self.test_dir)
- build_file_path = build_file_path.replace(os.path.sep, '/')
- self.messages.append([build_file_path, line_number, target_name,
- source_file, subpackage])
-
-
class UnitTest(unittest.TestCase):
- def RunTest(self, test_dir, check_all_build_files=False):
- logger = Logger(test_dir)
+ def _RunTest(self, test_dir, check_all_build_files=False):
build_files = [os.path.join(test_dir, 'BUILD.gn')]
if check_all_build_files:
build_files = None
- CheckPackageBoundaries(test_dir, logger, build_files)
+
+ messages = []
+ for violation in CheckPackageBoundaries(test_dir, build_files):
+ build_file_path = os.path.relpath(violation.build_file_path, test_dir)
+ build_file_path = build_file_path.replace(os.path.sep, '/')
+ messages.append(violation._replace(build_file_path=build_file_path))
+
expected_messages = ReadPylFile(os.path.join(test_dir, 'expected.pyl'))
- self.assertListEqual(sorted(expected_messages), sorted(logger.messages))
+ self.assertListEqual(sorted(expected_messages), sorted(messages))
def testNoErrors(self):
- self.RunTest(os.path.join(TESTDATA_DIR, 'no_errors'))
+ self._RunTest(os.path.join(TESTDATA_DIR, 'no_errors'))
def testMultipleErrorsSingleTarget(self):
- self.RunTest(os.path.join(TESTDATA_DIR, 'multiple_errors_single_target'))
+ self._RunTest(os.path.join(TESTDATA_DIR, 'multiple_errors_single_target'))
def testMultipleErrorsMultipleTargets(self):
- self.RunTest(os.path.join(TESTDATA_DIR, 'multiple_errors_multiple_targets'))
+ self._RunTest(os.path.join(TESTDATA_DIR,
+ 'multiple_errors_multiple_targets'))
def testCommonPrefix(self):
- self.RunTest(os.path.join(TESTDATA_DIR, 'common_prefix'))
+ self._RunTest(os.path.join(TESTDATA_DIR, 'common_prefix'))
def testAllBuildFiles(self):
- self.RunTest(os.path.join(TESTDATA_DIR, 'all_build_files'), True)
+ self._RunTest(os.path.join(TESTDATA_DIR, 'all_build_files'), True)
def testSanitizeFilename(self):
# The `dangerous_filename` test case contains a directory with '++' in its
# name. If it's not properly escaped, a regex error would be raised.
- self.RunTest(os.path.join(TESTDATA_DIR, 'dangerous_filename'), True)
+ self._RunTest(os.path.join(TESTDATA_DIR, 'dangerous_filename'), True)
def testRelativeFilename(self):
test_dir = os.path.join(TESTDATA_DIR, 'all_build_files')
- logger = Logger(test_dir)
with self.assertRaises(AssertionError):
- CheckPackageBoundaries(test_dir, logger, ["BUILD.gn"])
+ CheckPackageBoundaries(test_dir, ["BUILD.gn"])
if __name__ == '__main__':
diff --git a/tools_webrtc/presubmit_checks_lib/testdata/all_build_files/expected.pyl b/tools_webrtc/presubmit_checks_lib/testdata/all_build_files/expected.pyl
index b067c3c..410e08a 100644
--- a/tools_webrtc/presubmit_checks_lib/testdata/all_build_files/expected.pyl
+++ b/tools_webrtc/presubmit_checks_lib/testdata/all_build_files/expected.pyl
@@ -1,20 +1,20 @@
-[['subpackage2/BUILD.gn',
+[('subpackage2/BUILD.gn',
'12',
'error_2',
'subsubpackage2/dummy_subsubpackage2.cc',
- 'subsubpackage2'],
- ['subpackage2/BUILD.gn',
+ 'subsubpackage2'),
+ ('subpackage2/BUILD.gn',
'13',
'error_2',
'subsubpackage2/dummy_subsubpackage2.h',
- 'subsubpackage2'],
- ['subpackage1/BUILD.gn',
+ 'subsubpackage2'),
+ ('subpackage1/BUILD.gn',
'12',
'error_1',
'subsubpackage1/dummy_subsubpackage1.cc',
- 'subsubpackage1'],
- ['subpackage1/BUILD.gn',
+ 'subsubpackage1'),
+ ('subpackage1/BUILD.gn',
'13',
'error_1',
'subsubpackage1/dummy_subsubpackage1.h',
- 'subsubpackage1']]
+ 'subsubpackage1')]
diff --git a/tools_webrtc/presubmit_checks_lib/testdata/dangerous_filename/expected.pyl b/tools_webrtc/presubmit_checks_lib/testdata/dangerous_filename/expected.pyl
index 5447ab0..38b8322 100644
--- a/tools_webrtc/presubmit_checks_lib/testdata/dangerous_filename/expected.pyl
+++ b/tools_webrtc/presubmit_checks_lib/testdata/dangerous_filename/expected.pyl
@@ -1,5 +1,5 @@
-[["BUILD.gn",
+[("BUILD.gn",
"13",
"dummy_target",
"libc++/dummy_subpackage_file.h",
- "libc++"]]
+ "libc++")]
diff --git a/tools_webrtc/presubmit_checks_lib/testdata/multiple_errors_multiple_targets/expected.pyl b/tools_webrtc/presubmit_checks_lib/testdata/multiple_errors_multiple_targets/expected.pyl
index 92b889c..b9935b6 100644
--- a/tools_webrtc/presubmit_checks_lib/testdata/multiple_errors_multiple_targets/expected.pyl
+++ b/tools_webrtc/presubmit_checks_lib/testdata/multiple_errors_multiple_targets/expected.pyl
@@ -1,20 +1,20 @@
-[['BUILD.gn',
+[('BUILD.gn',
'18',
'error_1',
'subpackage1/dummy_subpackage1.cc',
- 'subpackage1'],
- ['BUILD.gn',
+ 'subpackage1'),
+ ('BUILD.gn',
'19',
'error_1',
'subpackage1/dummy_subpackage1.h',
- 'subpackage1'],
- ['BUILD.gn',
+ 'subpackage1'),
+ ('BUILD.gn',
'25',
'error_2',
'subpackage1/dummy_subpackage2.cc',
- 'subpackage1'],
- ['BUILD.gn',
+ 'subpackage1'),
+ ('BUILD.gn',
'26',
'error_2',
'subpackage1/dummy_subpackage2.h',
- 'subpackage1']]
+ 'subpackage1')]
diff --git a/tools_webrtc/presubmit_checks_lib/testdata/multiple_errors_single_target/expected.pyl b/tools_webrtc/presubmit_checks_lib/testdata/multiple_errors_single_target/expected.pyl
index 94ec0be..9ddb541 100644
--- a/tools_webrtc/presubmit_checks_lib/testdata/multiple_errors_single_target/expected.pyl
+++ b/tools_webrtc/presubmit_checks_lib/testdata/multiple_errors_single_target/expected.pyl
@@ -1,10 +1,10 @@
-[["BUILD.gn",
+[("BUILD.gn",
"11",
"dummy_target",
"subpackage/dummy_subpackage_file.cc",
- "subpackage"],
- ["BUILD.gn",
+ "subpackage"),
+ ("BUILD.gn",
"12",
"dummy_target",
"subpackage/dummy_subpackage_file.h",
- "subpackage"]]
+ "subpackage")]