admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Run pylint on presubmit only for modified python files.

Browse Source 
Currently, PRESUBMIT.py always runs pylint on all .py files when
at least one python file changes.
This helps to maintain consistency across the codebase, but
due to changes in the pylintrc rules, it has been failing for months.
Migrating all python files to the new rules can take a lot of time,
so as a workaround, for now, just run pylint on modified files.

Also, fixed or suppressed all complaints of too long lines in the
PRESUBMIT.py file to get this CL to pass the presubmit.

Bug: webrtc:12114
Change-Id: I4f6c0c269b3fe07878e168e7c90c196cb34f1d16
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/220980
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34408}
This commit is contained in:
Byoungchan Lee 2021-07-01 22:21:44 +09:00 committed by WebRTC LUCI CQ
parent 510c94cbfb
commit 94f2ef2e84
1 changed files with 65 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

113
PRESUBMIT.py
View File

@ -146,9 +146,9 @@ def VerifyNativeApiHeadersListIsValid(input_api, output_api):
if non_existing_paths: if non_existing_paths:
return [ return [
output_api.PresubmitError( output_api.PresubmitError(
'Directories to native API headers have changed which has made the ' 'Directories to native API headers have changed which has made '
'list in PRESUBMIT.py outdated.\nPlease update it to the current ' 'the list in PRESUBMIT.py outdated.\nPlease update it to the '
'location of our native APIs.', non_existing_paths) 'current location of our native APIs.', non_existing_paths)
] ]
return [] return []
@ -212,10 +212,10 @@ def CheckNoIOStreamInHeaders(input_api, output_api, source_file_filter):
if len(files): if len(files):
return [ return [
output_api.PresubmitError( output_api.PresubmitError(
'Do not #include <iostream> in header files, since it inserts static ' 'Do not #include <iostream> in header files, since it inserts '
+ 'static initialization into every file including the header. '
'initialization into every file including the header. Instead, ' 'Instead, #include <ostream>. See http://crbug.com/94794',
+ '#include <ostream>. See http://crbug.com/94794', files) files)
] ]
return [] return []
@ -237,15 +237,15 @@ def CheckNoPragmaOnce(input_api, output_api, source_file_filter):
return [ return [
output_api.PresubmitError( output_api.PresubmitError(
'Do not use #pragma once in header files.\n' 'Do not use #pragma once in header files.\n'
'See http://www.chromium.org/developers/coding-style#TOC-File-headers', 'See http://www.chromium.org/developers/coding-style'
'#TOC-File-headers',
files) files)
] ]
return [] return []
def CheckNoFRIEND_TEST(# pylint: disable=invalid-name
def CheckNoFRIEND_TEST(
input_api, input_api,
output_api, # pylint: disable=invalid-name output_api,
source_file_filter): source_file_filter):
"""Make sure that gtest's FRIEND_TEST() macro is not used, the """Make sure that gtest's FRIEND_TEST() macro is not used, the
FRIEND_TEST_ALL_PREFIXES() macro from testsupport/gtest_prod_util.h should be FRIEND_TEST_ALL_PREFIXES() macro from testsupport/gtest_prod_util.h should be
@ -263,9 +263,9 @@ def CheckNoFRIEND_TEST(
return [] return []
return [ return [
output_api.PresubmitPromptWarning( output_api.PresubmitPromptWarning(
'WebRTC\'s code should not use ' 'WebRTC\'s code should not use gtest\'s FRIEND_TEST() macro. '
'gtest\'s FRIEND_TEST() macro. Include testsupport/gtest_prod_util.h and ' 'Include testsupport/gtest_prod_util.h and use '
'use FRIEND_TEST_ALL_PREFIXES() instead.\n' + '\n'.join(problems)) 'FRIEND_TEST_ALL_PREFIXES() instead.\n' + '\n'.join(problems))
] ]
@ -346,9 +346,9 @@ def CheckNoSourcesAbove(input_api, gn_files, output_api):
if violating_gn_files: if violating_gn_files:
return [ return [
output_api.PresubmitError( output_api.PresubmitError(
'Referencing source files above the directory of the GN file is not ' 'Referencing source files above the directory of the GN file '
'allowed. Please introduce new GN targets in the proper location ' 'is not allowed. Please introduce new GN targets in the proper '
'instead.\n' 'location instead.\n'
'Invalid source entries:\n' 'Invalid source entries:\n'
'%s\n' '%s\n'
'Violating GN files:' % '\n'.join(violating_source_entries), 'Violating GN files:' % '\n'.join(violating_source_entries),
@ -407,9 +407,9 @@ def CheckNoMixingSources(input_api, gn_files, output_api):
gn_file_content = input_api.ReadFile(gn_file) gn_file_content = input_api.ReadFile(gn_file)
for target_match in TARGET_RE.finditer(gn_file_content): for target_match in TARGET_RE.finditer(gn_file_content):
# list_of_sources is a list of tuples of the form # list_of_sources is a list of tuples of the form
# (c_files, cc_files, objc_files) that keeps track of all the sources # (c_files, cc_files, objc_files) that keeps track of all the
# defined in a target. A GN target can have more that on definition of # sources defined in a target. A GN target can have more that
# sources (since it supports if/else statements). # on definition of sources (since it supports if/else statements).
# E.g.: # E.g.:
# rtc_static_library("foo") { # rtc_static_library("foo") {
# if (is_win) { # if (is_win) {
@ -454,7 +454,8 @@ def CheckNoMixingSources(input_api, gn_files, output_api):
return [ return [
output_api.PresubmitError( output_api.PresubmitError(
'GN targets cannot mix .c, .cc and .m (or .mm) source files.\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' 'Please create a separate target for each collection of '
'sources.\n'
'Mixed sources: \n' 'Mixed sources: \n'
'%s\n' '%s\n'
'Violating GN files:\n%s\n' % 'Violating GN files:\n%s\n' %
@ -476,8 +477,8 @@ def CheckNoPackageBoundaryViolations(input_api, gn_files, output_api):
if errors: if errors:
return [ return [
output_api.PresubmitError( output_api.PresubmitError(
'There are package boundary violations in the following GN files:', 'There are package boundary violations in the following GN '
long_text='\n\n'.join(str(err) for err in errors)) 'files:', long_text='\n\n'.join(str(err) for err in errors))
] ]
return [] return []
@ -491,7 +492,7 @@ def CheckNoWarningSuppressionFlagsAreAdded(gn_files,
input_api, input_api,
output_api, output_api,
error_formatter=_ReportFileAndLine): error_formatter=_ReportFileAndLine):
"""Make sure that warning suppression flags are not added wihtout a reason.""" """Ensure warning suppression flags are not added wihtout a reason."""
msg = ('Usage of //build/config/clang:extra_warnings is discouraged ' msg = ('Usage of //build/config/clang:extra_warnings is discouraged '
'in WebRTC.\n' 'in WebRTC.\n'
'If you are not adding this code (e.g. you are just moving ' 'If you are not adding this code (e.g. you are just moving '
@ -674,7 +675,8 @@ def CheckGnGen(input_api, output_api):
if errors: if errors:
return [ return [
output_api.PresubmitPromptWarning( output_api.PresubmitPromptWarning(
'Some #includes do not match the build dependency graph. Please run:\n' 'Some #includes do not match the build dependency graph. '
'Please run:\n'
' gn gen --check <out_dir>', ' gn gen --check <out_dir>',
long_text='\n\n'.join(errors)) long_text='\n\n'.join(errors))
] ]
@ -729,18 +731,20 @@ def CheckUnwantedDependencies(input_api, output_api, source_file_filter):
if error_descriptions: if error_descriptions:
results.append( results.append(
output_api.PresubmitError( output_api.PresubmitError(
'You added one or more #includes that violate checkdeps rules.\n' 'You added one or more #includes that violate checkdeps rules.'
'Check that the DEPS files in these locations contain valid rules.\n' '\nCheck that the DEPS files in these locations contain valid '
'See https://cs.chromium.org/chromium/src/buildtools/checkdeps/ for ' 'rules.\nSee '
'more details about checkdeps.', error_descriptions)) 'https://cs.chromium.org/chromium/src/buildtools/checkdeps/ '
'for more details about checkdeps.', error_descriptions))
if warning_descriptions: if warning_descriptions:
results.append( results.append(
output_api.PresubmitPromptOrNotify( output_api.PresubmitPromptOrNotify(
'You added one or more #includes of files that are temporarily\n' 'You added one or more #includes of files that are temporarily'
'allowed but being removed. Can you avoid introducing the\n' '\nallowed but being removed. Can you avoid introducing the\n'
'#include? See relevant DEPS file(s) for details and contacts.\n' '#include? See relevant DEPS file(s) for details and contacts.'
'See https://cs.chromium.org/chromium/src/buildtools/checkdeps/ for ' '\nSee '
'more details about checkdeps.', warning_descriptions)) 'https://cs.chromium.org/chromium/src/buildtools/checkdeps/ '
'for more details about checkdeps.', warning_descriptions))
return results return results
@ -787,9 +791,10 @@ def CheckChangeHasBugField(input_api, output_api):
else: else:
return [ return [
output_api.PresubmitError( output_api.PresubmitError(
'The "Bug: [bug number]" footer is mandatory. Please create a bug and ' 'The "Bug: [bug number]" footer is mandatory. Please create a '
'reference it using either of:\n' 'bug and reference it using either of:\n'
' * https://bugs.webrtc.org - reference it using Bug: webrtc:XXXX\n' ' * https://bugs.webrtc.org - reference it using Bug: '
'webrtc:XXXX\n'
' * https://crbug.com - reference it using Bug: chromium:XXXXXX' ' * https://crbug.com - reference it using Bug: chromium:XXXXXX'
) )
] ]
@ -911,10 +916,19 @@ def CommonChecks(input_api, output_api):
results.extend( results.extend(
input_api.canned_checks.CheckLicense(input_api, output_api, input_api.canned_checks.CheckLicense(input_api, output_api,
_LicenseHeader(input_api))) _LicenseHeader(input_api)))
# TODO(bugs.webrtc.org/12114): Delete this filter and run pylint on
# all python files. This is a temporary solution.
python_file_filter = lambda f: (f.LocalPath().endswith('.py') and
source_file_filter(f))
python_changed_files = [f.LocalPath() for f in input_api.AffectedFiles(
file_filter=python_file_filter)]
results.extend( results.extend(
input_api.canned_checks.RunPylint( input_api.canned_checks.RunPylint(
input_api, input_api,
output_api, output_api,
files_to_check=python_changed_files,
files_to_skip=( files_to_skip=(
r'^base[\\\/].*\.py$', r'^base[\\\/].*\.py$',
r'^build[\\\/].*\.py$', r'^build[\\\/].*\.py$',
@ -932,12 +946,13 @@ def CommonChecks(input_api, output_api):
pylintrc='pylintrc')) pylintrc='pylintrc'))
# TODO(nisse): talk/ is no more, so make below checks simpler? # TODO(nisse): talk/ is no more, so make below checks simpler?
# WebRTC can't use the presubmit_canned_checks.PanProjectChecks function since # WebRTC can't use the presubmit_canned_checks.PanProjectChecks function
# we need to have different license checks in talk/ and webrtc/ directories. # since we need to have different license checks
# in talk/ and webrtc/directories.
# Instead, hand-picked checks are included below. # Instead, hand-picked checks are included below.
# .m and .mm files are ObjC files. For simplicity we will consider .h files in # .m and .mm files are ObjC files. For simplicity we will consider
# ObjC subdirectories ObjC headers. # .h files in ObjC subdirectories ObjC headers.
objc_filter_list = (r'.+\.m$', r'.+\.mm$', r'.+objc\/.+\.h$') objc_filter_list = (r'.+\.m$', r'.+\.mm$', r'.+objc\/.+\.h$')
# Skip long-lines check for DEPS and GN files. # Skip long-lines check for DEPS and GN files.
build_file_filter_list = (r'.+\.gn$', r'.+\.gni$', 'DEPS') build_file_filter_list = (r'.+\.gn$', r'.+\.gni$', 'DEPS')
@ -1163,9 +1178,9 @@ def CheckAbslMemoryInclude(input_api, output_api, source_file_filter):
if len(files): if len(files):
return [ return [
output_api.PresubmitError( output_api.PresubmitError(
'Please include "absl/memory/memory.h" header for absl::WrapUnique.\n' 'Please include "absl/memory/memory.h" header for '
'This header may or may not be included transitively depending on the ' 'absl::WrapUnique.\nThis header may or may not be included '
'C++ standard version.', files) 'transitively depending on the C++ standard version.', files)
] ]
return [] return []
@ -1343,13 +1358,15 @@ def CheckAddedDepsHaveTargetApprovals(input_api, output_api):
if input_api.tbr: if input_api.tbr:
return [ return [
output_api.PresubmitNotifyResult( output_api.PresubmitNotifyResult(
'--tbr was specified, skipping OWNERS check for DEPS additions' '--tbr was specified, skipping OWNERS check for DEPS '
'additions'
) )
] ]
if input_api.dry_run: if input_api.dry_run:
return [ return [
output_api.PresubmitNotifyResult( output_api.PresubmitNotifyResult(
'This is a dry run, skipping OWNERS check for DEPS additions' 'This is a dry run, skipping OWNERS check for DEPS '
'additions'
) )
] ]
if not input_api.change.issue: if not input_api.change.issue:
@ -1393,8 +1410,8 @@ def CheckAddedDepsHaveTargetApprovals(input_api, output_api):
if unapproved_dependencies: if unapproved_dependencies:
output_list = [ output_list = [
output( output(
'You need LGTM from owners of depends-on paths in DEPS that were ' 'You need LGTM from owners of depends-on paths in DEPS that '
'modified in this CL:\n %s' % ' were modified in this CL:\n %s' %
'\n '.join(sorted(unapproved_dependencies))) '\n '.join(sorted(unapproved_dependencies)))
] ]
suggested_owners = input_api.owners_client.SuggestOwners( suggested_owners = input_api.owners_client.SuggestOwners(