InTheForest/chromium_depot_tools
1
0
Fork 0
mirror of https://chromium.googlesource.com/chromium/tools/depot_tools.git synced 2026-01-11 10:41:31 +00:00
Code Issues Packages Projects Releases Wiki Activity

Optimize presubmit --all with --no_diffs option

Browse Source 
Some of the expensive checks when running presubmit --all, such as
CheckStableMojomChanges (~300 s) and CheckAddedDepsHaveTargetApprovals
(~200 s) only look at diffs and are therefore guaranteed to be NOPs when
running presubmit --all or --files=. Passing along the no_diffs state
lets these expensive checks be skipped, thus allowing for faster
iteration times.

Initial testing suggests that (with some supporting changes in the
Chromium repo) this reduces "presubmit --all" times by about ten
minutes, or a bit more than 10%, and additional improvements may be
possible.

Special handling for the no-diffs case also offers a simple way to avoid
presubmit failures that happen whenever all files are flagged as being
changed.

Finally, and perhaps most importantly for having a presubmit --all bot,
when --no_diffs is passed we can treat errors like "Issue wasn't
uploaded" and "Add a description to the CL" as messages, thus making it
possible to have zero presubmit errors when run on origin/main.

Bug: 1320937, 1322936
Change-Id: I0d09dd4aae8fdaa48c8b2f89337441cf96dcff72
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3628368
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Gavin Mak <gavinmak@google.com>
This commit is contained in:
Bruce Dawson
2022-05-26 20:28:58 +00:00
committed by LUCI CQ
parent 31140af3cf
commit 09c0c073ea
5 changed files with 45 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
git_cl.py
View File

@@ -1362,6 +1362,8 @@ class Changelist(object):
if files:
args.extend(files.split(';'))
args.append('--source_controlled_only')
if files or all_files:
args.append('--no_diffs')
if resultdb and not realm:
# TODO (crbug.com/1113463): store realm somewhere and look it up so

21
presubmit_canned_checks.py
View File

@@ -111,7 +111,7 @@ def CheckChangeHasDescription(input_api, output_api):
"""Checks the CL description is not empty."""
text = input_api.change.DescriptionText()
if text.strip() == '':
if input_api.is_committing:
if input_api.is_committing and not input_api.no_diffs:
return [output_api.PresubmitError('Add a description to the CL.')]
return [output_api.PresubmitNotifyResult('Add a description to the CL.')]
@@ -121,8 +121,11 @@ def CheckChangeHasDescription(input_api, output_api):
def CheckChangeWasUploaded(input_api, output_api):
"""Checks that the issue was uploaded before committing."""
if input_api.is_committing and not input_api.change.issue:
return [output_api.PresubmitError(
'Issue wasn\'t uploaded. Please upload first.')]
message = 'Issue wasn\'t uploaded. Please upload first.'
if input_api.no_diffs:
# Make this just a message with presubmit --all and --files
return [output_api.PresubmitNotifyResult(message)]
return [output_api.PresubmitError(message)]
return []
@@ -326,6 +329,9 @@ def CheckGenderNeutral(input_api, output_api, source_file_filter=None):
"""Checks that there are no gendered pronouns in any of the text files to be
submitted.
"""
if input_api.no_diffs:
return []
gendered_re = input_api.re.compile(
r'(^|\s|\(|\[)([Hh]e|[Hh]is|[Hh]ers?|[Hh]im|[Ss]he|[Gg]uys?)\\b')
@@ -419,6 +425,8 @@ def _FindNewViolationsOfRule(callable_rule,
Returns:
A list of the newly-introduced violations reported by the rule.
"""
if input_api.no_diffs:
return []
return _FindNewViolationsOfRuleForList(
callable_rule, _GenerateAffectedFileExtList(
input_api, source_file_filter), error_formatter)
@@ -475,6 +483,8 @@ def CheckLongLines(input_api, output_api, maxlen, source_file_filter=None):
"""Checks that there aren't any lines longer than maxlen characters in any of
the text files to be submitted.
"""
if input_api.no_diffs:
return []
maxlens = {
'java': 100,
# This is specifically for Android's handwritten makefiles (Android.mk).
@@ -1165,6 +1175,9 @@ def CheckDirMetadataFormat(input_api, output_api, dirmd_bin=None):
def CheckNoNewMetadataInOwners(input_api, output_api):
"""Check that no metadata is added to OWNERS files."""
if input_api.no_diffs:
return []
_METADATA_LINE_RE = input_api.re.compile(
r'^#\s*(TEAM|COMPONENT|OS|WPT-NOTIFY)+\s*:\s*\S+$',
input_api.re.MULTILINE | input_api.re.IGNORECASE)
@@ -1938,6 +1951,8 @@ def CheckInclusiveLanguage(input_api, output_api,
# chromium/src.
if input_api.change.RepositoryRoot() != input_api.PresubmitLocalPath():
return []
if input_api.no_diffs:
return []
warnings = []
errors = []

28
presubmit_support.py
View File

@@ -590,7 +590,8 @@ class InputApi(object):
)
def __init__(self, change, presubmit_path, is_committing,
verbose, gerrit_obj, dry_run=None, thread_pool=None, parallel=False):
verbose, gerrit_obj, dry_run=None, thread_pool=None, parallel=False,
no_diffs=False):
"""Builds an InputApi object.
Args:
@@ -601,6 +602,8 @@ class InputApi(object):
dry_run: if true, some Checks will be skipped.
parallel: if true, all tests reported via input_api.RunTests for all
PRESUBMIT files will be run in parallel.
no_diffs: if true, implies that --files or --all was specified so some
checks can be skipped, and some errors will be messages.
"""
# Version number of the presubmit_support script.
self.version = [int(x) for x in __version__.split('.')]
@@ -608,6 +611,7 @@ class InputApi(object):
self.is_committing = is_committing
self.gerrit = gerrit_obj
self.dry_run = dry_run
self.no_diffs = no_diffs
self.parallel = parallel
self.thread_pool = thread_pool or ThreadPool()
@@ -1493,7 +1497,8 @@ def DoPostUploadExecuter(change, gerrit_obj, verbose, use_python3=False):
class PresubmitExecuter(object):
def __init__(self, change, committing, verbose, gerrit_obj, dry_run=None,
thread_pool=None, parallel=False, use_python3=False):
thread_pool=None, parallel=False, use_python3=False,
no_diffs=False):
"""
Args:
change: The Change object.
@@ -1504,6 +1509,8 @@ class PresubmitExecuter(object):
PRESUBMIT files will be run in parallel.
use_python3: if true, will use python3 instead of python2 by default
if USE_PYTHON3 is not specified.
no_diffs: if true, implies that --files or --all was specified so some
checks can be skipped, and some errors will be messages.
"""
self.change = change
self.committing = committing
@@ -1514,6 +1521,7 @@ class PresubmitExecuter(object):
self.thread_pool = thread_pool
self.parallel = parallel
self.use_python3 = use_python3
self.no_diffs = no_diffs
def ExecPresubmitScript(self, script_text, presubmit_path):
"""Executes a single presubmit script.
@@ -1537,7 +1545,7 @@ class PresubmitExecuter(object):
input_api = InputApi(self.change, presubmit_path, self.committing,
self.verbose, gerrit_obj=self.gerrit,
dry_run=self.dry_run, thread_pool=self.thread_pool,
parallel=self.parallel)
parallel=self.parallel, no_diffs=self.no_diffs)
output_api = OutputApi(self.committing)
context = {}
@@ -1671,7 +1679,8 @@ def DoPresubmitChecks(change,
dry_run=None,
parallel=False,
json_output=None,
use_python3=False):
use_python3=False,
no_diffs=False):
"""Runs all presubmit checks that apply to the files in the change.
This finds all PRESUBMIT.py files in directories enclosing the files in the
@@ -1694,6 +1703,8 @@ def DoPresubmitChecks(change,
PRESUBMIT files will be run in parallel.
use_python3: if true, default to using Python3 for presubmit checks
rather than Python2.
no_diffs: if true, implies that --files or --all was specified so some
checks can be skipped, and some errors will be messages.
Return:
1 if presubmit checks failed or 0 otherwise.
"""
@@ -1718,7 +1729,8 @@ def DoPresubmitChecks(change,
results = []
thread_pool = ThreadPool()
executer = PresubmitExecuter(change, committing, verbose, gerrit_obj,
dry_run, thread_pool, parallel, use_python3)
dry_run, thread_pool, parallel, use_python3,
no_diffs)
skipped_count = 0;
if default_presubmit:
if verbose:
@@ -1989,6 +2001,7 @@ def main(argv=None):
help='Write presubmit errors to json output.')
parser.add_argument('--all_files', action='store_true',
help='Mark all files under source control as modified.')
parser.add_argument('files', nargs='*',
help='List of files to be marked as modified when '
'executing presubmit or post-upload hooks. fnmatch '
@@ -1997,6 +2010,8 @@ def main(argv=None):
help='Constrain \'files\' to those in source control.')
parser.add_argument('--use-python3', action='store_true',
help='Use python3 for presubmit checks by default')
parser.add_argument('--no_diffs', action='store_true',
help='Assume that all "modified" files have no diffs.')
options = parser.parse_args(argv)
log_level = logging.ERROR
@@ -2028,7 +2043,8 @@ def main(argv=None):
options.dry_run,
options.parallel,
options.json_output,
options.use_python3)
options.use_python3,
options.no_diffs)
except PresubmitFailure as e:
import utils
print(e, file=sys.stderr)

2
tests/git_cl_test.py
View File

@@ -3109,6 +3109,7 @@ class ChangelistTest(unittest.TestCase):
'--may_prompt',
'--parallel',
'--all_files',
'--no_diffs',
'--json_output', '/tmp/fake-temp2',
'--description_file', '/tmp/fake-temp1',
])
@@ -3127,6 +3128,7 @@ class ChangelistTest(unittest.TestCase):
'--may_prompt',
'--parallel',
'--all_files',
'--no_diffs',
'--json_output', '/tmp/fake-temp4',
'--description_file', '/tmp/fake-temp3',
])

1
tests/presubmit_unittest.py
View File

@@ -1791,6 +1791,7 @@ class CannedChecksUnittest(PresubmitTestsBase):
input_api.subprocess.CalledProcessError = fake_CalledProcessError
input_api.verbose = False
input_api.is_windows = False
input_api.no_diffs = False
input_api.change = change
input_api.is_committing = committing