gh-109408: Run `patchcheck` in GitHub Actions by sobolevn · Pull Request #109459 · python/cpython
Finally! :)
Sorry for the noise, it took me quite some time to reproduce this in CI.
| - name: Run patchcheck | ||
| if: github.event_name == 'pull_request' | ||
| run: | | ||
| git fetch origin |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 patchcheck in its own job for speed, AA-Turner@8af0f2c is a sketch of an approach.
Two changes are needed to patchcheck, both due to prior assumptions that patchcheck runs on a local build of CPython.
- On CI, use the current working directory as
SRCDIRrather than usingsysconfig. - We use
patchlevel.hto get the Python version, notsys.version_info(the docs use this approach too)
It may be worth considering using the CI environment variable rather than a ci=true CLI flag, but that could be delayed to a later date.
A
@AA-Turner I am not quite comfortable refactoring code that I don't quite understand :)
And since this is liteally the first time I see patchcheck, I would prefer to keep this PR as simple as possible.
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 :)
@sobolevn -- if you want to keep
patchcheckin its own job for speed, AA-Turner@8af0f2c is a sketch of an approach.
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.
- We use
patchlevel.hto get the Python version, notsys.version_info(the docs use this approach too)
Could be nice to refactor Doc/tools/extensions/patchlevel.py and put it somewhere common for use by both?
@hugovk I propose we merge this as-is for now, because +1m43s is not even close to how long our regular tests take (around 15m 4s). Moreover, we will save +13m from Azure Pipeline, when duplication is deleted:
Later we can:
- Replace most checks with
pre-commit/ruff - Refactor the rest checks to use modern alternatives / API
- Do not build CPython from source here for no reason
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
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 will rework @AA-Turner's script to make it possible to run on Windows as well
I developed it on Windows--though I normalised all paths to forward-slash, for simplicity.
A