-
Notifications
You must be signed in to change notification settings - Fork 294
Fix the PATH computation in GHA on windows, and require uv on PATH
#2696
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
base: main
Are you sure you want to change the base?
Conversation
Don't get clever about it at all, instead make uv on PATH a hard requirement.
|
I'm back now, I'll try to take a look soon, I would like for all of the packages I work on that can use uv (build, nox, cibuildwheel, check-sdist) to do exactly the same behavior (I think this is also true for tox and others). If the problem is that we are not setting up our fake virtual environment correctly, then we should fix that. I can get a build release out soon to see if that helps, as well. |
|
Thanks. Yeah, just a thought- it's kinda hard for the Python packages don't inherit between venvs. But PATH entries do. So that's some of the logic behind this approach. |
|
By the way, I have a theory why #2630 broke some builders e.g. meson-python in #2663. In the version on Lines 89 to 91 in 24bade1
And the PATH from that bash step is written, and used in the powershell step. So we don't actually get the powershell-native PATH, we get whatever bash is giving (perhaps that includes MinGW tools). |
|
This PR avoids that by outputting a 'prepend-path' variable from the bash step, and updating PATH in the pwsh step. |
|
I think I'd prefer the patch-up fixes #2673 and most of #2690; those restore the previous behavior and fix an oversight. Then longer term we can decide if we go this way to improve using "uv" from cibuildwheel's commands (those fixes are related to cibuildwheel itself working correctly). If we do go this way, I want us to remove the |
|
Going with #2673 would stop I've done a little testing of my theory that the way the PATH is extended in a bash step is the reason behind the #2663 meson link.exe issue. Test results are here - https://github.com/joerick/ping/actions/runs/20798304116/job/59737382262 - I must say that it's a little confusing, but certainly there are a few extra PATH entries and different link.exe in the bash shell. Another option (that would be less disruptive) would be to keep the prepend approach as seen in this PR, but still resolve uv using find_uv. Having said that, my preference is still to ensure that
|
uv on PATH
Don't get clever about it at all, instead make uv on PATH a hard requirement for the caller of the cibuildwheel CLI. See #2691 (comment) for context.
In the github action, this approach does add the cibuildwheel install venv bin to PATH. This has the potential downside of exposing some other transitive deps of cibuildwheel, but I think that's a theoretical downside, I'm not aware of a way that could be an issue in practice. I think it's an okay tradeoff for more simplicity.
close #2690
close #2673
close #2691