From 0c15c5332fea2bbf5fe29dd806f9f4e606eeb9b8 Mon Sep 17 00:00:00 2001 From: Mirko Bonadei Date: Tue, 19 Sep 2017 10:41:02 +0200 Subject: [PATCH] Adding PRESUBMIT check to avoid mixing C, C++ and Objc-C/Obj-C++. The error message will be something like: GN targets cannot mix .c (or .cc) and .m (or .mm) source files. Please create a separate target for each collection of sources. Mixed sources: { BUILD_GN_PATH: [ [ TARGET_NAME, [ SOURCES ] ], ... ], ... } Bug: webrtc:7743 Change-Id: I45dd2c621b830e5aeb081fa8d17c9497a49c2554 Reviewed-on: https://webrtc-review.googlesource.com/1980 Commit-Queue: Mirko Bonadei Reviewed-by: Henrik Kjellander Cr-Commit-Position: refs/heads/master@{#19897} --- PRESUBMIT.py | 79 ++++++++++++++++++++++++++++------------- presubmit_test.py | 75 +++++++++++++++++++++++++++++++++++--- presubmit_test_mocks.py | 12 +++++++ 3 files changed, 138 insertions(+), 28 deletions(-) diff --git a/PRESUBMIT.py b/PRESUBMIT.py index 37047d47ce..aebadb5ae2 100755 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py @@ -11,6 +11,7 @@ import os import re import subprocess import sys +from collections import defaultdict # Files and directories that are *skipped* by cpplint in the presubmit script. @@ -67,6 +68,7 @@ NATIVE_API_DIRS = ( 'modules/audio_device/include', 'pc', ) + # These directories should not be used but are maintained only to avoid breaking # some legacy downstream code. LEGACY_API_DIRS = ( @@ -90,8 +92,22 @@ LEGACY_API_DIRS = ( 'system_wrappers/include', 'voice_engine/include', ) + API_DIRS = NATIVE_API_DIRS[:] + LEGACY_API_DIRS[:] +# TARGET_RE matches a GN target, and extracts the target name and the contents. +TARGET_RE = re.compile(r'(?P\s*)\w+\("(?P\w+)"\) {' + r'(?P.*?)' + 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) + +# FILE_PATH_RE matchies a file path. +FILE_PATH_RE = re.compile(r'"(?P(\w|\/)+)(?P\.\w+)"') + def _RunCommand(command, cwd): """Runs a command and returns the output from that command.""" @@ -297,33 +313,48 @@ def CheckNoSourcesAbove(input_api, gn_files, output_api): items=violating_gn_files)] return [] -def CheckNoMixingCAndCCSources(input_api, gn_files, output_api): - # Disallow mixing .c and .cc source files in the same target. - source_pattern = input_api.re.compile(r' +sources \+?= \[(.*?)\]', - re.MULTILINE | re.DOTALL) - file_pattern = input_api.re.compile(r'"(.*)"') - violating_gn_files = dict() +def CheckNoMixingSources(input_api, gn_files, output_api): + """Disallow mixing C, C++ and Obj-C/Obj-C++ in the same target. + + See bugs.webrtc.org/7743 for more context. + """ + def _MoreThanOneSourceUsed(*sources_lists): + sources_used = 0 + for source_list in sources_lists: + if len(source_list): + sources_used += 1 + return sources_used > 1 + + errors = defaultdict(lambda: []) for gn_file in gn_files: - contents = input_api.ReadFile(gn_file) - for source_block_match in source_pattern.finditer(contents): + gn_file_content = input_api.ReadFile(gn_file) + for target_match in TARGET_RE.finditer(gn_file_content): c_files = [] cc_files = [] - for file_list_match in file_pattern.finditer(source_block_match.group(1)): - source_file = file_list_match.group(1) - if source_file.endswith('.c'): - c_files.append(source_file) - if source_file.endswith('.cc'): - cc_files.append(source_file) - if c_files and cc_files: - violating_gn_files[gn_file.LocalPath()] = sorted(c_files + cc_files) - if violating_gn_files: + objc_files = [] + target_name = target_match.group('target_name') + target_contents = target_match.group('target_contents') + for sources_match in SOURCES_RE.finditer(target_contents): + for file_match in FILE_PATH_RE.finditer(sources_match.group(1)): + file_path = file_match.group('file_path') + extension = file_match.group('extension') + if extension == '.c': + c_files.append(file_path + extension) + if extension == '.cc': + cc_files.append(file_path + extension) + if extension in ['.m', '.mm']: + objc_files.append(file_path + extension) + if _MoreThanOneSourceUsed(c_files, cc_files, objc_files): + all_sources = sorted(c_files + cc_files + objc_files) + errors[gn_file.LocalPath()].append((target_name, all_sources)) + if errors: return [output_api.PresubmitError( - 'GN targets cannot mix .cc and .c source files. Please create a ' - 'separate target for each collection of sources.\n' + 'GN targets cannot mix .c, .cc and .m (or .mm) source files.\n' + 'Please create a separate target for each collection of sources.\n' 'Mixed sources: \n' '%s\n' - 'Violating GN files:' % json.dumps(violating_gn_files, indent=2), - items=violating_gn_files.keys())] + 'Violating GN files:\n%s\n' % (json.dumps(errors, indent=2), + '\n'.join(errors.keys())))] return [] def CheckNoPackageBoundaryViolations(input_api, gn_files, output_api): @@ -350,9 +381,9 @@ def CheckGnChanges(input_api, output_api): result = [] if gn_files: result.extend(CheckNoSourcesAbove(input_api, gn_files, output_api)) - result.extend(CheckNoMixingCAndCCSources(input_api, gn_files, output_api)) - result.extend(CheckNoPackageBoundaryViolations( - input_api, gn_files, output_api)) + result.extend(CheckNoMixingSources(input_api, gn_files, output_api)) + result.extend(CheckNoPackageBoundaryViolations(input_api, gn_files, + output_api)) return result def CheckUnwantedDependencies(input_api, output_api): diff --git a/presubmit_test.py b/presubmit_test.py index f17a20f5b2..5e342baf5b 100755 --- a/presubmit_test.py +++ b/presubmit_test.py @@ -60,7 +60,7 @@ class CheckNewlineAtTheEndOfProtoFilesTest(unittest.TestCase): shutil.rmtree(self.tmp_dir, ignore_errors=True) def testErrorIfProtoFileDoesNotEndWithNewline(self): - self.__GenerateProtoWithoutNewlineAtTheEnd() + self._GenerateProtoWithoutNewlineAtTheEnd() self.input_api.files = [MockFile(self.proto_file_path)] errors = PRESUBMIT.CheckNewlineAtTheEndOfProtoFiles(self.input_api, self.output_api) @@ -70,13 +70,13 @@ class CheckNewlineAtTheEndOfProtoFilesTest(unittest.TestCase): str(errors[0])) def testNoErrorIfProtoFileEndsWithNewline(self): - self.__GenerateProtoWithNewlineAtTheEnd() + self._GenerateProtoWithNewlineAtTheEnd() self.input_api.files = [MockFile(self.proto_file_path)] errors = PRESUBMIT.CheckNewlineAtTheEndOfProtoFiles(self.input_api, self.output_api) self.assertEqual(0, len(errors)) - def __GenerateProtoWithNewlineAtTheEnd(self): + def _GenerateProtoWithNewlineAtTheEnd(self): with open(self.proto_file_path, 'w') as f: f.write(textwrap.dedent(""" syntax = "proto2"; @@ -84,7 +84,7 @@ class CheckNewlineAtTheEndOfProtoFilesTest(unittest.TestCase): package webrtc.audioproc; """)) - def __GenerateProtoWithoutNewlineAtTheEnd(self): + def _GenerateProtoWithoutNewlineAtTheEnd(self): with open(self.proto_file_path, 'w') as f: f.write(textwrap.dedent(""" syntax = "proto2"; @@ -92,5 +92,72 @@ class CheckNewlineAtTheEndOfProtoFilesTest(unittest.TestCase): package webrtc.audioproc;""")) +class CheckNoMixingSourcesTest(unittest.TestCase): + + def setUp(self): + self.tmp_dir = tempfile.mkdtemp() + self.file_path = os.path.join(self.tmp_dir, 'BUILD.gn') + self.input_api = MockInputApi() + self.output_api = MockOutputApi() + + def tearDown(self): + shutil.rmtree(self.tmp_dir, ignore_errors=True) + + def testErrorIfCAndCppAreMixed(self): + self._AssertNumberOfErrorsWithSources(1, ['foo.c', 'bar.cc', 'bar.h']) + + def testErrorIfCAndObjCAreMixed(self): + self._AssertNumberOfErrorsWithSources(1, ['foo.c', 'bar.m', 'bar.h']) + + def testErrorIfCAndObjCppAreMixed(self): + self._AssertNumberOfErrorsWithSources(1, ['foo.c', 'bar.mm', 'bar.h']) + + def testErrorIfCppAndObjCAreMixed(self): + self._AssertNumberOfErrorsWithSources(1, ['foo.cc', 'bar.m', 'bar.h']) + + def testErrorIfCppAndObjCppAreMixed(self): + self._AssertNumberOfErrorsWithSources(1, ['foo.cc', 'bar.mm', 'bar.h']) + + def testNoErrorIfOnlyC(self): + self._AssertNumberOfErrorsWithSources(0, ['foo.c', 'bar.c', 'bar.h']) + + def testNoErrorIfOnlyCpp(self): + self._AssertNumberOfErrorsWithSources(0, ['foo.cc', 'bar.cc', 'bar.h']) + + def testNoErrorIfOnlyObjC(self): + self._AssertNumberOfErrorsWithSources(0, ['foo.m', 'bar.m', 'bar.h']) + + def testNoErrorIfOnlyObjCpp(self): + self._AssertNumberOfErrorsWithSources(0, ['foo.mm', 'bar.mm', 'bar.h']) + + def testNoErrorIfObjCAndObjCppAreMixed(self): + self._AssertNumberOfErrorsWithSources(0, ['foo.m', 'bar.mm', 'bar.h']) + + def _AssertNumberOfErrorsWithSources(self, number_of_errors, sources): + assert 3 == len(sources), 'This function accepts a list of 3 source files' + self._GenerateBuildFile(textwrap.dedent(""" + rtc_source_set("foo_bar") { + sources = [ + "%s", + "%s", + "%s", + ], + } + """ % tuple(sources))) + self.input_api.files = [MockFile(self.file_path)] + errors = PRESUBMIT.CheckNoMixingSources(self.input_api, + [MockFile(self.file_path)], + self.output_api) + self.assertEqual(number_of_errors, len(errors)) + if number_of_errors == 1: + for source in sources: + if not source.endswith('.h'): + self.assertTrue(source in str(errors[0])) + + def _GenerateBuildFile(self, content): + with open(self.file_path, 'w') as f: + f.write(content) + + if __name__ == '__main__': unittest.main() diff --git a/presubmit_test_mocks.py b/presubmit_test_mocks.py index f7ead25fab..cbfc4f50d7 100644 --- a/presubmit_test_mocks.py +++ b/presubmit_test_mocks.py @@ -25,6 +25,15 @@ class MockInputApi(object): # pylint: disable=unused-argument return self.files + def ReadFile(self, affected_file, mode='rU'): + filename = affected_file.AbsoluteLocalPath() + for f in self.files: + if f.LocalPath() == filename: + with open(filename, mode) as f: + return f.read() + # Otherwise, file is not in our mock API. + raise IOError, "No such file or directory: '%s'" % filename + class MockOutputApi(object): """Mock class for the OutputApi class. @@ -71,3 +80,6 @@ class MockFile(object): def LocalPath(self): return self._local_path + + def AbsoluteLocalPath(self): + return self._local_path