gh-109408: Run patchcheck in GitHub Actions
#109459
Conversation
|
Finally! :) |
| - name: Run patchcheck | ||
| if: github.event_name == 'pull_request' | ||
| run: | | ||
| git fetch origin |
There was a problem hiding this comment.
Do we need to fetch origin? It takes 1m 49s for this step.
We don't do it on Azure Pipelines and patchcheck takes 2s.
There was a problem hiding this comment.
Let's try :)
I think that we might need it because of the heavy git machinery inside patchcheck.
I think we might need it during backports for older branches.
There was a problem hiding this comment.
Nope, it does not work:
Run # git fetch origin
Checked 107 modules (31 built-in, 75 shared, 1 n/a on linux-x86_64, 0 disabled, 0 missing, 0 failed on import)
LD_LIBRARY_PATH=/home/runner/work/cpython/cpython ./python ./Tools/patchcheck/patchcheck.py --ci true
Getting base branch for PR ... origin/main
fatal: ambiguous argument 'origin/main': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
error running git diff --name-status origin/main
make: *** [Makefile:2914: patchcheck] Error 1
Getting the list of files that have been added/changed ...
|
@sobolevn -- if you want to keep Two changes are needed to
It may be worth considering using the A |
|
@AA-Turner I am not quite comfortable refactoring code that I don't quite understand :) Later we can change the tooling to be more convenient / faster, but for now I would like to stick with my solution. |
|
In the end I'd like to remove or replace as much of patchcheck with equivalents that we do understand, like pre-commit or Ruff linting. But this is a good first step. |
|
I agree 100%, that what I was thinking about all the way :) |
This approach takes 22s, including fetching source and installing a prebuilt Python. This PR piggy backs on 'Check if generated files are up to date' to take advantage of a local Python build, but is +1m 48s, mostly fetching source.
Could be nice to refactor |
|
@hugovk I propose we merge this as-is for now, because Later we can:
|
|
A slightly different approach would be AA-Turner@5ad4faf, which avoids all of the Git chicanery and just runs patchcheck on all files in the source tree. This seems to take ~20s (ex1, ex2). A |
~30s for the whole thing, but that's not too bad and I think I prefer this approach -- we also run on all files for pre-commit on the CI. |
|
Yeap, all files is also fine by me (since they are all correct anyway). I will rework @AA-Turner's script to make it possible to run on Windows as well, because it might be useful for local testing. Thanks! |
I developed it on Windows--though I normalised all paths to forward-slash, for simplicity. A |
Continuing your idea in #109408 (comment)
Refs #109452