diff --git a/PRESUBMIT.py b/PRESUBMIT.py index c8ac1656a5..ff46605230 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 @@ SOURCES_RE = re.compile(r'sources \+?= \[(?P.*?)\]', FILE_PATH_RE = re.compile(r'"(?P(\w|\/)+)(?P\.\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 CheckNoMixingSources(input_api, gn_files, output_api): 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 @@ def CheckUnwantedDependencies(input_api, output_api): # 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 @@ def CheckOrphanHeaders(input_api, output_api): 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 aabcd813e4..4b39bc5c00 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\s*)\w+\("(?P\w+)"\) {' r'(?P.*?)' @@ -28,27 +25,16 @@ TARGET_RE = re.compile(r'\d+\$(?P\s*)\w+\("(?P\w+)"\) {' SOURCES_RE = re.compile(r'sources \+?= \[(?P.*?)\]', 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 @@ def _ReadFileAndPrependLines(file_path): 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 @@ def _CheckBuildFile(build_file_path, packages, logger): 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 @@ def CheckPackageBoundaries(root_dir, logger, build_files=None): 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 @@ def main(): 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 1acdb10891..7a5874cf26 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 @@ def ReadPylFile(file_path): 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 b067c3c5b0..410e08a543 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 5447ab041d..38b8322c25 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 92b889cd17..b9935b6d68 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 94ec0bee12..9ddb5418c8 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")]