From b7dc45f8e8119d367b7513045db7cce68861709f Mon Sep 17 00:00:00 2001 From: Mirko Bonadei Date: Tue, 21 Jan 2020 08:42:41 +0100 Subject: [PATCH] Update check_package_boundaries. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before reformatting GN files (see [1] for why this is needed), the presubmit check to ensure targets are not violating package boundaries needs to be fixed because its regular expressions don't always work with the new format. This CL removes the parsing of line numbers to relax the regular expressions without losing any functionality. Error before this CL: *************** /webrtc/src/BUILD.gn:674 in target 'android_junit_tests': Source file 'examples/androidjunit/src/org/appspot/apprtc/BluetoothManagerTest.java' crosses boundary of package 'examples'. /webrtc/src/BUILD.gn:675 in target 'android_junit_tests': Source file 'examples/androidjunit/src/org/appspot/apprtc/DirectRTCClientTest.java' crosses boundary of package 'examples'. /webrtc/src/BUILD.gn:676 in target 'android_junit_tests': Source file 'examples/androidjunit/src/org/appspot/apprtc/TCPChannelClientTest.java' crosses boundary of package 'examples'. /webrtc/src/BUILD.gn:677 in target 'android_junit_tests': Source file 'sdk/android/tests/src/org/webrtc/AndroidVideoDecoderTest.java' crosses boundary of package 'sdk'. /webrtc/src/BUILD.gn:678 in target 'android_junit_tests': Source file 'sdk/android/tests/src/org/webrtc/CameraEnumerationTest.java' crosses boundary of package 'sdk'. *************** Error after this CL: *************** /webrtc/src/BUILD.gn in target 'android_junit_tests': Source file 'examples/androidjunit/src/org/appspot/apprtc/BluetoothManagerTest.java' crosses boundary of package 'examples'. /webrtc/src/BUILD.gn in target 'android_junit_tests': Source file 'examples/androidjunit/src/org/appspot/apprtc/DirectRTCClientTest.java' crosses boundary of package 'examples'. /webrtc/src/BUILD.gn in target 'android_junit_tests': Source file 'examples/androidjunit/src/org/appspot/apprtc/TCPChannelClientTest.java' crosses boundary of package 'examples'. /webrtc/src/BUILD.gn in target 'android_junit_tests': Source file 'sdk/android/tests/src/org/webrtc/AndroidVideoDecoderTest.java' crosses boundary of package 'sdk'. /webrtc/src/BUILD.gn in target 'android_junit_tests': Source file 'sdk/android/tests/src/org/webrtc/CameraEnumerationTest.java' crosses boundary of package 'sdk'. *************** [1] - https://gn-review.googlesource.com/c/gn/+/6860 Bug: webrtc:11302 Change-Id: Ia39387d089a0c56a2c3ad9a7264c20eb5a38ac93 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/166535 Reviewed-by: Patrik Höglund Commit-Queue: Mirko Bonadei Cr-Commit-Position: refs/heads/master@{#30331} --- .../check_package_boundaries.py | 18 ++++++++---------- .../testdata/all_build_files/expected.pyl | 4 ---- .../testdata/dangerous_filename/expected.pyl | 1 - .../expected.pyl | 4 ---- .../multiple_errors_single_target/expected.pyl | 2 -- 5 files changed, 8 insertions(+), 21 deletions(-) diff --git a/tools_webrtc/presubmit_checks_lib/check_package_boundaries.py b/tools_webrtc/presubmit_checks_lib/check_package_boundaries.py index 4b39bc5c00..1b3c1f8e43 100644 --- a/tools_webrtc/presubmit_checks_lib/check_package_boundaries.py +++ b/tools_webrtc/presubmit_checks_lib/check_package_boundaries.py @@ -16,23 +16,23 @@ import sys # 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+)"\) {' +TARGET_RE = re.compile(r'(?P\s*)\w+\("(?P\w+)"\) {' r'(?P.*?)' - r'\d+\$(?P=indent)}', + r'(?P=indent)}', re.MULTILINE | re.DOTALL) # SOURCES_RE matches a block of sources inside a GN target. SOURCES_RE = re.compile(r'sources \+?= \[(?P.*?)\]', re.MULTILINE | re.DOTALL) -ERROR_MESSAGE = ("{build_file_path}:{line_number} in target '{target_name}':\n" +ERROR_MESSAGE = ("{build_file_path} in target '{target_name}':\n" " Source file '{source_file}'\n" " crosses boundary of package '{subpackage}'.") class PackageBoundaryViolation( collections.namedtuple('PackageBoundaryViolation', - 'build_file_path line_number target_name source_file subpackage')): + 'build_file_path target_name source_file subpackage')): def __str__(self): return ERROR_MESSAGE.format(**self._asdict()) @@ -42,7 +42,7 @@ def _BuildSubpackagesPattern(packages, query): of the given query.""" query += os.path.sep length = len(query) - pattern = r'(?P\d+)\$\s*"(?P(?P' + pattern = r'\s*"(?P(?P' pattern += '|'.join(re.escape(package[length:].replace(os.path.sep, '/')) for package in packages if package.startswith(query)) pattern += r')/[\w\./]*)"' @@ -50,10 +50,9 @@ def _BuildSubpackagesPattern(packages, query): def _ReadFileAndPrependLines(file_path): - """Reads the contents of a file and prepends the line number to every line.""" + """Reads the contents of a file.""" with open(file_path) as f: - return "".join("{}${}".format(line_number, line) - for line_number, line in enumerate(f, 1)) + return "".join(f.readlines()) def _CheckBuildFile(build_file_path, packages): @@ -73,9 +72,8 @@ def _CheckBuildFile(build_file_path, packages): for subpackages_match in subpackages_re.finditer(sources): subpackage = subpackages_match.group('subpackage') source_file = subpackages_match.group('source_file') - line_number = subpackages_match.group('line_number') if subpackage: - yield PackageBoundaryViolation(build_file_path, line_number, + yield PackageBoundaryViolation(build_file_path, target_name, source_file, subpackage) 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 410e08a543..07f98e9a6e 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,16 @@ [('subpackage2/BUILD.gn', - '12', 'error_2', 'subsubpackage2/dummy_subsubpackage2.cc', 'subsubpackage2'), ('subpackage2/BUILD.gn', - '13', 'error_2', 'subsubpackage2/dummy_subsubpackage2.h', 'subsubpackage2'), ('subpackage1/BUILD.gn', - '12', 'error_1', 'subsubpackage1/dummy_subsubpackage1.cc', 'subsubpackage1'), ('subpackage1/BUILD.gn', - '13', 'error_1', 'subsubpackage1/dummy_subsubpackage1.h', '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 38b8322c25..34f23f8a01 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,4 @@ [("BUILD.gn", - "13", "dummy_target", "libc++/dummy_subpackage_file.h", "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 b9935b6d68..9b9ad01c6c 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,16 @@ [('BUILD.gn', - '18', 'error_1', 'subpackage1/dummy_subpackage1.cc', 'subpackage1'), ('BUILD.gn', - '19', 'error_1', 'subpackage1/dummy_subpackage1.h', 'subpackage1'), ('BUILD.gn', - '25', 'error_2', 'subpackage1/dummy_subpackage2.cc', 'subpackage1'), ('BUILD.gn', - '26', 'error_2', 'subpackage1/dummy_subpackage2.h', '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 9ddb5418c8..012d3bd1c2 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,8 @@ [("BUILD.gn", - "11", "dummy_target", "subpackage/dummy_subpackage_file.cc", "subpackage"), ("BUILD.gn", - "12", "dummy_target", "subpackage/dummy_subpackage_file.h", "subpackage")]