Simplify api/DEPS presubmit check.
After the problem fixed by [1], it seems wiser to only check that `include_rules` contains all the top level directories and delegate the action of adding a rule for .cc file to the first user of an header from a new root level directory. This should make the presubmit check more actionable (in [1] for example the solution was to *not* add "+ios" but to consider ios/ as a non WebRTC directory). [1] - https://webrtc-review.googlesource.com/c/107707 Bug: webrtc:9887, webrtc:9924 Change-Id: Ic85e2153a2b83a4874c8faec3c5d1a8c61fe6faf Reviewed-on: https://webrtc-review.googlesource.com/c/107731 Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org> Reviewed-by: Karl Wiberg <kwiberg@webrtc.org> Cr-Commit-Position: refs/heads/master@{#25386}
This commit is contained in:

committed by
Commit Bot

parent
7d76a31f3d
commit
904903705f
74
PRESUBMIT.py
74
PRESUBMIT.py
@ -867,58 +867,56 @@ def CommonChecks(input_api, output_api):
|
|||||||
|
|
||||||
|
|
||||||
def CheckApiDepsFileIsUpToDate(input_api, output_api):
|
def CheckApiDepsFileIsUpToDate(input_api, output_api):
|
||||||
|
"""Check that 'include_rules' in api/DEPS is up to date.
|
||||||
|
|
||||||
|
The file api/DEPS must be kept up to date in order to avoid to avoid to
|
||||||
|
include internal header from WebRTC's api/ headers.
|
||||||
|
|
||||||
|
This check is focused on ensuring that 'include_rules' contains a deny
|
||||||
|
rule for each root level directory. More focused allow rules can be
|
||||||
|
added to 'specific_include_rules'.
|
||||||
|
"""
|
||||||
results = []
|
results = []
|
||||||
api_deps = os.path.join(input_api.PresubmitLocalPath(), 'api', 'DEPS')
|
api_deps = os.path.join(input_api.PresubmitLocalPath(), 'api', 'DEPS')
|
||||||
with open(api_deps) as f:
|
with open(api_deps) as f:
|
||||||
deps_content = _ParseDeps(f.read())
|
deps_content = _ParseDeps(f.read())
|
||||||
|
|
||||||
include_rules = deps_content.get('include_rules', [])
|
include_rules = deps_content.get('include_rules', [])
|
||||||
specific_include_rules = deps_content.get('specific_include_rules', [])
|
|
||||||
cc_include_rules = specific_include_rules.get(r'.*\.cc', [])
|
|
||||||
|
|
||||||
top_level_files = [f for f in os.listdir(input_api.PresubmitLocalPath())
|
# Only check top level directories affected by the current CL.
|
||||||
if f != 'api' and not f.startswith('.')]
|
dirs_to_check = set()
|
||||||
top_level_dirs = []
|
for f in input_api.AffectedFiles():
|
||||||
for f in top_level_files:
|
path_tokens = [t for t in f.LocalPath().split(os.sep) if t]
|
||||||
if os.path.isdir(os.path.join(input_api.PresubmitLocalPath(), f)):
|
if len(path_tokens) > 1:
|
||||||
top_level_dirs.append(f)
|
if (path_tokens[0] != 'api' and
|
||||||
|
os.path.isdir(os.path.join(input_api.PresubmitLocalPath(),
|
||||||
|
path_tokens[0]))):
|
||||||
|
dirs_to_check.add(path_tokens[0])
|
||||||
|
|
||||||
missing_include_rules = []
|
missing_include_rules = set()
|
||||||
for p in top_level_dirs:
|
for p in dirs_to_check:
|
||||||
rule = '-%s' % p
|
rule = '-%s' % p
|
||||||
if rule not in include_rules:
|
if rule not in include_rules:
|
||||||
missing_include_rules.append(rule)
|
missing_include_rules.add(rule)
|
||||||
|
|
||||||
if missing_include_rules:
|
if missing_include_rules:
|
||||||
results.append(output_api.PresubmitError(
|
error_msg = [
|
||||||
'Please add the following lines to `include_rules` in file\n'
|
'include_rules = [\n',
|
||||||
'%s:\n%s' % (api_deps, str(missing_include_rules))))
|
' ...\n',
|
||||||
|
]
|
||||||
|
|
||||||
missing_cc_include_rules = []
|
for r in sorted(missing_include_rules):
|
||||||
non_webrtc_dirs = [
|
error_msg.append(' "%s",\n' % str(r))
|
||||||
'base',
|
|
||||||
'build',
|
error_msg.append(' ...\n')
|
||||||
'build_overrides',
|
error_msg.append(']\n')
|
||||||
'buildtools',
|
|
||||||
'data',
|
|
||||||
'infra',
|
|
||||||
'ios',
|
|
||||||
'out',
|
|
||||||
'resources',
|
|
||||||
'testing',
|
|
||||||
'style-guide',
|
|
||||||
]
|
|
||||||
webrtc_top_level_dirs = [d for d in top_level_dirs
|
|
||||||
if d not in non_webrtc_dirs]
|
|
||||||
|
|
||||||
for p in webrtc_top_level_dirs:
|
|
||||||
rule = '+%s' % p
|
|
||||||
if rule not in cc_include_rules:
|
|
||||||
missing_cc_include_rules.append(rule)
|
|
||||||
if missing_cc_include_rules:
|
|
||||||
results.append(output_api.PresubmitError(
|
results.append(output_api.PresubmitError(
|
||||||
r'Please add the following lines to the `.*\.cc` rule under '
|
'New root level directory detected! WebRTC api/ headers should '
|
||||||
'`specific_include_rules` in file\n'
|
'not #include headers from \n'
|
||||||
'%s:\n%s' % (api_deps, str(missing_cc_include_rules))))
|
'the new directory, so please update "include_rules" in file\n'
|
||||||
|
'"%s". Example:\n%s\n' % (api_deps, ''.join(error_msg))))
|
||||||
|
|
||||||
return results
|
return results
|
||||||
|
|
||||||
|
|
||||||
|
10
api/DEPS
10
api/DEPS
@ -1,3 +1,5 @@
|
|||||||
|
# This is supposed to be a complete list of top-level directories,
|
||||||
|
# excepting only api/ itself.
|
||||||
include_rules = [
|
include_rules = [
|
||||||
"-audio",
|
"-audio",
|
||||||
"-base",
|
"-base",
|
||||||
@ -283,10 +285,10 @@ specific_include_rules = {
|
|||||||
"+rtc_base/scoped_ref_ptr.h",
|
"+rtc_base/scoped_ref_ptr.h",
|
||||||
],
|
],
|
||||||
|
|
||||||
# We allow .cc files in webrtc/api/ to #include a bunch of stuff
|
# .cc files in api/ should not be restricted in what they can #include,
|
||||||
# that's off-limits for the .h files. That's because .h files leak
|
# so we re-add all the top-level directories here. (That's because .h
|
||||||
# their #includes to whoever's #including them, but .cc files do not
|
# files leak their #includes to whoever's #including them, but .cc files
|
||||||
# since no one #includes them.
|
# do not since no one #includes them.)
|
||||||
".*\.cc": [
|
".*\.cc": [
|
||||||
"+audio",
|
"+audio",
|
||||||
"+call",
|
"+call",
|
||||||
|
Reference in New Issue
Block a user