presubmit --all tells the presubmit system that all files are 'modified'
but ChangedContents still goes off to see what changes are present. If
most files were unchanged this was all handled perfectly, but if _all_
files were unchanged then _GitDiffCache would interpret the empty
dictionary of changes as a reason to ask git for diffs, millions of
times. This made some checks take more than 100x as long. The overall
effect on presubmit --all time is not known because I was never willing
to wait the multiple days for them to terminate.
That is, this would take many days to run:
git checkout -b empty -t origin/main
git cl presubmit --all
whereas a single-character change to any file would let this run in
about two hours.
After three weeks of working on presubmits I only hit this twice which
is why it took me so long to realize what the problem was.
Bug: 1309977
Change-Id: Ib280ea386107843b9174d835b0895316a5ed240c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3589900
Reviewed-by: Aravind Vasudevan <aravindvasudev@google.com>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
It is easy to get type confusion and end up passing a list as the
message parameter to _PresubmitResult. This error will not be detected
until the end of the run - perhaps hours later - when all evidence of
where the list came from is lost.
This change ensures that the message parameter is a string. If it is not
then the exception that is thrown should allow quick identification of
the problematic code.
This also fixes a presubmit unit test that passed None as the message.
We could support that but I don't think that we should.
Bug: 1309977
Change-Id: Ifb1d5100d47922b0ebd8bb834caa6fbba690b43c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3566436
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Chromium's presubmits contain a lot of latent errors - presubmit errors
that will trigger the next time a file is modified, but have been
waiting for years. Flushing those out with "git cl presubmit --all"
works poorly because it exercises all presubmits simultaneously, which
is too slow and triggers too many failures. Adding a --files option lets
small areas of the tree or specific file types to be exercised in a
controlled manner.
Bug: 1311697
Change-Id: I36ec6a759a80000d6ed4a8cc218ece327d45f8d6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3559174
Auto-Submit: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Gavin Mak <gavinmak@google.com>
When running git cl presubmit --all which is handy for finding
latent presubmit bugs, the 'No diff found for %s' gets printed many
thousands of times, making actual issues difficult to find. This change
suppresses that message, to make actual issues easier to find.
The message for slow presubmits prints the name of the slow presubmit,
but there are 243 different CheckChangeOnCommit functions, so the name
is actually not sufficient. Therefore this change plumbs through the
path to the script containing the presubmit and prints it.
Similarly, the cpplint message "Done processing %s" doesn't add enough
value.
These changes will make it easier to find the signal in the presubmit
noise.
Bug: 1309977
Change-Id: Iba40b5748266e3296eeb530bb00182db4814aa5d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3556594
Reviewed-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Gitwatch supports closing bugs with a FIX(ES|ED|ING)? tag in the
commit message, but we only check BUG to ensure a bug is
specified, add the fix fields to BugsFromDescription so that presubmits
will still pass if only eg: FIXES=b:1234 is given.
Gitwatch reference:
http://shortn/_LGPgBjgi3A
FIXED=b:203812728
TEST=presubmit_unittest.py doesn't fail more than it already does
Change-Id: I6afc38c786e281dcefb4d359bb9212ebe5980ed6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3237598
Auto-Submit: Sean McAllister <smcallis@google.com>
Reviewed-by: Josip Sokcevic <sokcevic@google.com>
Commit-Queue: Josip Sokcevic <sokcevic@google.com>
When presubmit checks fail, we want to print the error message from the
underlying check/tool. We use _PresubmitResult objects to collect these
diagnostics and output them to the console and json files.
This change ensures that the string values passed to the
_PresubmitResult constructor will be converted, as needed, to 'str'
instances. This way, we will avoid encoding errors and make sure to
print the actual messages instead of repr(message).
SHERIFFS: this patch breaks backwards compatibility of presubmit error
handling but we don't expect issues in downstream projects. Please
revert if downstream presubmit checks start failing spuriously.
Bug: 1225052
Change-Id: If7f5c99cb5d730c1bfbd6d108b912f77b5f2455c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3037262
Commit-Queue: Tom McKee <tommckee@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@google.com>
This CL adds a new field to the codereview.settings file used by
git_cl for project-wide defaults. If `USE_PYTHON3` is set to True,
then we will run the PRESUBMIT checks under Python3 by default
instead of Python2, unless the PRESUBMIT.py file contains
`USE_PYTHON3 = False` on a line by itself
(as opposed to now, where we'll use Python2 by default unless the
file contains `USE_PYTHON3 = True`).
This will allow us have Python3 be the default for new files
and to eventually eliminate any uses of `USE_PYTHON3` from the
individual presubmit files. Of course, you will have to go in and
explicitly add `USE_PYTHON3 = False` to any Py2-only files prior
to enabling this.
Bug: 1207012
Change-Id: Id8ec45b43940e5bcffeee196398c711c541e733e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2917747
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Josip Sokcevic <sokcevic@google.com>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Commit-Queue: Dirk Pranke <dpranke@google.com>
If a presubmit happened to generate a crash, the logic we have
for reporting the presubmit check times would call `six.reraise()`
incorrectly, leading to a second crash.
This CL fixes the invocation of the second crash, and also adds
an error message to help users tripping over one possible source
of the first crash (reading a binary file as text).
Bug: 1210746
Change-Id: Ic46f38901b6acf2055b3feb7272dc751dc69037c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2921322
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Commit-Queue: Dirk Pranke <dpranke@google.com>
Presubmits when uploading occasionally take a very long time - over a
minute - and after the fact there is no easy way to know why. This makes
fixes to slow presubmits take longer.
This change builds on crrev.com/c/2532895 to print a message when any
individual presubmit function takes longer than ten seconds. During
normal usage this is a NOP but it will presumably find the long poles
when presubmits are running particularly slowly.
This is similar to gclient runhooks where any hooks that take longer
than ten seconds have their run time printed.
Change-Id: If57ed35d7a7d221f6380e9b97cf72af56f75e441
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2911594
Reviewed-by: Dirk Pranke <dpranke@google.com>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
CheckForWindowsLineEndings is a Chromium presubmit to make sure that
\r\n line endings (Windows line endings) don't get committed. Among
other things these line endings mess up the license checks, leading to
cryptic errors.
The problem is that ChangedContents() was stripping line endings, making
any checking for them futile. This change adds an option to preserve
line endings, to be used by Chromium's CheckForWindowsLineEndings.
It's not clear how long this has been broken.
See also crrev.com/c/2911914 which must land after this.
Bug: 801033
Change-Id: I7cdb9b3581788624f9eca081da43bb070ee412a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2910488
Reviewed-by: Dirk Pranke <dpranke@google.com>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
This CL adds support for running PRESUBMIT.py under either Python2
or Python3 as specified in each PRESUBMIT.py file.
To run the checks under Python3, the PRESUBMIT.py file must contain
a line exactly matching "^USE_PYTHON3 = True$". If the file
does not contain this string, the checks will run under Python2.
Different PRESUBMIT.py files in a single CL may thus contain
a mix of 2- and 3-compatible checks, but each individual file will
only be run in one or the other (it doesn't likely make sense to run
them in both by default).
Bug: 1157663
Change-Id: Ic74977941a6519388089328b6e1dfba2e885924b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2832654
Commit-Queue: Dirk Pranke <dpranke@google.com>
Reviewed-by: Josip Sokcevic <sokcevic@google.com>
