-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CI: Call local python pre-commit
scripts via shell
#92556
Conversation
Are shell scripts a reliable entry point on Windows? I wonder if the entry point should actually be a Python script with |
It's reliable insofar as pre-commit also utilizes shell scripts; if a Windows instance couldn't support this script, it wouldn't support pre-commit outright. If a Windows user is using Git, they've almost certainly got a means of running shell scripts. In any case, Windows users are the roadblock regardless; they're the only ones that don't have Still, doesn't hurt to double-check. @chrisl8 @nlupugla were both Windows users that tested the other PRs, their input would be appreciated. |
I believe that the reason why pre-commits and other shell scripts work for Windows users is that when you install git on Windows it also installs "git Bash" and associates .sh files with it. This image is copied from Google, so could be out of date, but here is a reference: So basically no, Windows cannot run .sh scripts, but any Windows host with git installed should be able to unless the user didn't follow the default git installation path. Here is a good reference on "Git Bash" that others who land here may find useful: I'll test this PR and report back, and I agree that others should test too. The Windows landscape is not as consistent as Linux/MacOS when it comes to git and shell availability. |
TestingTL;DR: Mixed results. Seems better with this PR, but not perfect. I'm not sure if this is the same issue or a new one though, as the "Python was not found" messages go away, and some things work that did not before. Current Master latest
This PR
Note that the odd formatting IS what I saw on screen too. |
Shoot; back to the drawing board |
Well not really, you totally fixed the I can dig into what is causing he EDIT: It occurs to me that you didn't understand my testing comment. Did you scroll down and see the results with this PR? The Master run was expected to fail. |
Ah! I did see the second results, but I misread the error as applying to all the cases instead of just one. Okay yeah, its not starting completely from scratch this time. Looking at how the odd formatting seems to move strictly downward, I'll bet this is somehow tied to a lack of carriage return. But with the wrapper method working, it's possible that this issue could be dodged entirely if the wrapper was applied to something like an scons call instead. Either way, this gives me something to look into! |
Did some timing tests, and this turns out to make the hooks significantly slower. Combined with the above formatting oddities, I'm marking this as a draft for now while I look into alternatives. |
@Repiteo You might look into akien-mga's idea too:
Feel free to ping me with any further testing requests as well. |
Unfortunately it seems that stock MacOS only comes with |
Superseded by #92697 |
Alternative to #90634 and #90662
This is a slightly hacky but fully-functional workaround for allowing local
pre-commit
python scripts to be fully multi-platform. Because we can only provide a static entry point, this PR converts said entry point to a new shell script wrapper:misc/scripts/python.sh
. This script's job is to evaluate several Python alias calls to see if they exist on the user's path; if a match is found, that alias is used to call the provided arguments directly.Also assigns proper arguments for
doc-status
, as I learned in local testing that it required a similar argument setup asmake-rst
. Both of these now call said arguments viaargs
instead of extending offentry
.