| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Finally! :) |
Sorry, something went wrong.
| - 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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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 ...
Sorry, something went wrong.
|
@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.
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 |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
I agree 100%, that what I was thinking about all the way :) |
Sorry, something went wrong.
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 Doc/tools/extensions/patchlevel.py and put it somewhere common for use by both? |
Sorry, something went wrong.
|
@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:
|
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
~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. |
Sorry, something went wrong.
|
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! |
Sorry, something went wrong.
I developed it on Windows--though I normalised all paths to forward-slash, for simplicity. A |
Sorry, something went wrong.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Continuing your idea in #109408 (comment)
Refs #109452