mirror of
https://github.com/mollyim/webrtc.git
synced 2025-05-12 21:30:45 +01:00
Fix bugs in check_package_boundaries.py presubmit
It was not producing any results on presubmit for 2 reasons: - The list of modified GN files contained relative paths but the list of all GN files contains absolute paths. - The root directory was not passed to the script and the path of the first file substituted it. Fix potential problem with using unescaped names in a regex. Blacklist testdata directory with intentionally bad BUILD.gn files. NOTRY=True Bug: webrtc:6954 Change-Id: Ib0b845b9440b594960bc8a656e8a84d2ccb4a684 Reviewed-on: https://webrtc-review.googlesource.com/5981 Reviewed-by: Edward Lemur <ehmaldonado@webrtc.org> Reviewed-by: Henrik Kjellander <kjellander@webrtc.org> Commit-Queue: Oleh Prypin <oprypin@webrtc.org> Cr-Commit-Position: refs/heads/master@{#20149}
This commit is contained in:
parent
88b23f6662
commit
afe016501e
6 changed files with 55 additions and 7 deletions
|
@ -381,8 +381,8 @@ 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]
|
||||
command += [gn_file.LocalPath() for gn_file in gn_files]
|
||||
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:
|
||||
return [output_api.PresubmitError(
|
||||
|
@ -392,7 +392,8 @@ def CheckNoPackageBoundaryViolations(input_api, gn_files, output_api):
|
|||
|
||||
def CheckGnChanges(input_api, output_api):
|
||||
source_file_filter = lambda x: input_api.FilterSourceFile(
|
||||
x, white_list=(r'.+\.(gn|gni)$',))
|
||||
x, white_list=(r'.+\.(gn|gni)$',),
|
||||
black_list=r'.*/presubmit_checks_lib/testdata/.*')
|
||||
|
||||
gn_files = []
|
||||
for f in input_api.AffectedSourceFiles(source_file_filter):
|
||||
|
|
|
@ -57,7 +57,7 @@ def _BuildSubpackagesPattern(packages, query):
|
|||
query += os.path.sep
|
||||
length = len(query)
|
||||
pattern = r'(?P<line_number>\d+)\$\s*"(?P<source_file>(?P<subpackage>'
|
||||
pattern += '|'.join(package[length:].replace(os.path.sep, '/')
|
||||
pattern += '|'.join(re.escape(package[length:].replace(os.path.sep, '/'))
|
||||
for package in packages if package.startswith(query))
|
||||
pattern += r')/[\w\./]*)"'
|
||||
return re.compile(pattern)
|
||||
|
@ -100,10 +100,13 @@ def _CheckBuildFile(build_file_path, packages, logger):
|
|||
def CheckPackageBoundaries(root_dir, logger, build_files=None):
|
||||
packages = [root for root, _, files in os.walk(root_dir)
|
||||
if 'BUILD.gn' in files]
|
||||
default_build_files = [os.path.join(package, 'BUILD.gn')
|
||||
for package in packages]
|
||||
|
||||
build_files = build_files or default_build_files
|
||||
if build_files is not None:
|
||||
for build_file_path in build_files:
|
||||
assert build_file_path.startswith(root_dir)
|
||||
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])
|
||||
|
||||
|
|
|
@ -63,6 +63,17 @@ class UnitTest(unittest.TestCase):
|
|||
def testAllBuildFiles(self):
|
||||
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)
|
||||
|
||||
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"])
|
||||
|
||||
|
||||
if __name__ == '__main__':
|
||||
unittest.main()
|
||||
|
|
15
tools_webrtc/presubmit_checks_lib/testdata/dangerous_filename/BUILD.gn
vendored
Normal file
15
tools_webrtc/presubmit_checks_lib/testdata/dangerous_filename/BUILD.gn
vendored
Normal file
|
@ -0,0 +1,15 @@
|
|||
# Copyright (c) 2017 The WebRTC project authors. All Rights Reserved.
|
||||
#
|
||||
# Use of this source code is governed by a BSD-style license
|
||||
# that can be found in the LICENSE file in the root of the source
|
||||
# tree. An additional intellectual property rights grant can be found
|
||||
# in the file PATENTS. All contributing project authors may
|
||||
# be found in the AUTHORS file in the root of the source tree.
|
||||
|
||||
# "libc++" is considered a "dangerous filename" because it's an invalid regex.
|
||||
|
||||
target("dummy_target") {
|
||||
sources = [
|
||||
"libc++/dummy_subpackage_file.h",
|
||||
]
|
||||
}
|
5
tools_webrtc/presubmit_checks_lib/testdata/dangerous_filename/expected.pyl
vendored
Normal file
5
tools_webrtc/presubmit_checks_lib/testdata/dangerous_filename/expected.pyl
vendored
Normal file
|
@ -0,0 +1,5 @@
|
|||
[["BUILD.gn",
|
||||
"13",
|
||||
"dummy_target",
|
||||
"libc++/dummy_subpackage_file.h",
|
||||
"libc++"]]
|
13
tools_webrtc/presubmit_checks_lib/testdata/dangerous_filename/libc++/BUILD.gn
vendored
Normal file
13
tools_webrtc/presubmit_checks_lib/testdata/dangerous_filename/libc++/BUILD.gn
vendored
Normal file
|
@ -0,0 +1,13 @@
|
|||
# Copyright (c) 2017 The WebRTC project authors. All Rights Reserved.
|
||||
#
|
||||
# Use of this source code is governed by a BSD-style license
|
||||
# that can be found in the LICENSE file in the root of the source
|
||||
# tree. An additional intellectual property rights grant can be found
|
||||
# in the file PATENTS. All contributing project authors may
|
||||
# be found in the AUTHORS file in the root of the source tree.
|
||||
|
||||
group("dummy_subpackage") {
|
||||
sources = [
|
||||
"dummy_subpackage.h",
|
||||
]
|
||||
}
|
Loading…
Reference in a new issue