Reland "Adding PRESUBMIT check to avoid mixing C, C++ and Objc-C/Obj-C++.""
This reverts commit 034a6b8a4cdf151ae7377c87c5b2b6156d658173.
Reason for revert: Trying to fix the issue of rtc_base:rtc_base which has 2 kind of source files but in exclusive if branches.
Original change's description:
> Revert "Adding PRESUBMIT check to avoid mixing C, C++ and Objc-C/Obj-C++."
>
> This reverts commit 0c15c5332fea2bbf5fe29dd806f9f4e606eeb9b8.
>
> Reason for revert: This causes problems in this moment. I have to fix a target in rtc_base before landing this presubmit check.
>
> Original change's description:
> > 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 <mbonadei@webrtc.org>
> > Reviewed-by: Henrik Kjellander <kjellander@webrtc.org>
> > Cr-Commit-Position: refs/heads/master@{#19897}
>
> TBR=kjellander@webrtc.org,mbonadei@webrtc.org
>
> Change-Id: I73ff609b0140719473afd36ead1632e5cc3b41f6
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: webrtc:7743
> Reviewed-on: https://webrtc-review.googlesource.com/2180
> Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
> Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#19898}
TBR=kjellander@webrtc.org,mbonadei@webrtc.org
Change-Id: I18dbb5a6a01ac2a184446542c29b25a3e33508ea
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:7743
Reviewed-on: https://webrtc-review.googlesource.com/2181
Reviewed-by: Henrik Kjellander <kjellander@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#19900}
This commit is contained in:
Mirko Bonadei
committed by
Commit Bot
Commit Bot
parent
080832eb37
commit
4dc4e259ce
101
PRESUBMIT.py
101
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<indent>\s*)\w+\("(?P<target_name>\w+)"\) {'
|
||||
r'(?P<target_contents>.*?)'
|
||||
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<sources>.*?)\]',
|
||||
re.MULTILINE | re.DOTALL)
|
||||
|
||||
# FILE_PATH_RE matchies a file path.
|
||||
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."""
|
||||
@ -297,33 +313,70 @@ 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):
|
||||
# list_of_sources is a list of tuples of the form
|
||||
# (c_files, cc_files, objc_files) that keeps track of all the sources
|
||||
# defined in a target. A GN target can have more that on definition of
|
||||
# sources (since it supports if/else statements).
|
||||
# E.g.:
|
||||
# rtc_static_library("foo") {
|
||||
# if (is_win) {
|
||||
# sources = [ "foo.cc" ]
|
||||
# } else {
|
||||
# sources = [ "foo.mm" ]
|
||||
# }
|
||||
# }
|
||||
# This is allowed and the presubmit check should support this case.
|
||||
list_of_sources = []
|
||||
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):
|
||||
if '+=' not in sources_match.group(0):
|
||||
if c_files or cc_files or objc_files:
|
||||
list_of_sources.append((c_files, cc_files, objc_files))
|
||||
c_files = []
|
||||
cc_files = []
|
||||
objc_files = []
|
||||
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)
|
||||
list_of_sources.append((c_files, cc_files, objc_files))
|
||||
for c_files_list, cc_files_list, objc_files_list in list_of_sources:
|
||||
if _MoreThanOneSourceUsed(c_files_list, cc_files_list, objc_files_list):
|
||||
all_sources = sorted(c_files_list + cc_files_list + objc_files_list)
|
||||
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 +403,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):
|
||||
|
||||
@ -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,157 @@ 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 testNoErrorIfSourcesAreInExclusiveIfBranches(self):
|
||||
self._GenerateBuildFile(textwrap.dedent("""
|
||||
rtc_source_set("bar_foo") {
|
||||
if (is_win) {
|
||||
sources = [
|
||||
"bar.cc",
|
||||
],
|
||||
}
|
||||
if (is_ios) {
|
||||
sources = [
|
||||
"bar.mm",
|
||||
],
|
||||
}
|
||||
}
|
||||
rtc_source_set("foo_bar") {
|
||||
if (is_win) {
|
||||
sources = [
|
||||
"foo.cc",
|
||||
],
|
||||
}
|
||||
if (is_ios) {
|
||||
sources = [
|
||||
"foo.mm",
|
||||
],
|
||||
}
|
||||
}
|
||||
"""))
|
||||
self.input_api.files = [MockFile(self.file_path)]
|
||||
errors = PRESUBMIT.CheckNoMixingSources(self.input_api,
|
||||
[MockFile(self.file_path)],
|
||||
self.output_api)
|
||||
self.assertEqual(0, len(errors))
|
||||
|
||||
def testErrorIfSourcesAreNotInExclusiveIfBranches(self):
|
||||
self._GenerateBuildFile(textwrap.dedent("""
|
||||
rtc_source_set("bar_foo") {
|
||||
if (is_win) {
|
||||
sources = [
|
||||
"bar.cc",
|
||||
],
|
||||
}
|
||||
if (foo_bar) {
|
||||
sources += [
|
||||
"bar.mm",
|
||||
],
|
||||
}
|
||||
}
|
||||
rtc_source_set("foo_bar") {
|
||||
if (is_win) {
|
||||
sources = [
|
||||
"foo.cc",
|
||||
],
|
||||
}
|
||||
if (foo_bar) {
|
||||
sources += [
|
||||
"foo.mm",
|
||||
],
|
||||
}
|
||||
if (is_ios) {
|
||||
sources = [
|
||||
"bar.m",
|
||||
"bar.c",
|
||||
],
|
||||
}
|
||||
}
|
||||
"""))
|
||||
self.input_api.files = [MockFile(self.file_path)]
|
||||
errors = PRESUBMIT.CheckNoMixingSources(self.input_api,
|
||||
[MockFile(self.file_path)],
|
||||
self.output_api)
|
||||
self.assertEqual(1, len(errors))
|
||||
self.assertTrue('bar.cc' in str(errors[0]))
|
||||
self.assertTrue('bar.mm' in str(errors[0]))
|
||||
self.assertTrue('foo.cc' in str(errors[0]))
|
||||
self.assertTrue('foo.mm' in str(errors[0]))
|
||||
self.assertTrue('bar.m' in str(errors[0]))
|
||||
self.assertTrue('bar.c' in str(errors[0]))
|
||||
|
||||
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_static_library("bar_foo") {
|
||||
sources = [
|
||||
"%s",
|
||||
"%s",
|
||||
"%s",
|
||||
],
|
||||
}
|
||||
rtc_source_set("foo_bar") {
|
||||
sources = [
|
||||
"%s",
|
||||
"%s",
|
||||
"%s",
|
||||
],
|
||||
}
|
||||
""" % (tuple(sources) * 2)))
|
||||
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()
|
||||
|
||||
@ -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
|
||||
|
||||
Reference in New Issue
Block a user