fix: look for uv next to python if it's not on PATH #795
Conversation
cjolowicz
force-pushed
the
fix-uv-lookup
branch
5 times, most recently
from
78d20af
to
74dd8ac
Compare
tests/test_virtualenv.py
Outdated
| @pytest.mark.parametrize( | ||
| ["which_result", "find_uv_bin_result", "expected"], | ||
| [ | ||
| ("/usr/bin/uv", UV_IN_PIPX_VENV, "/usr/bin/uv"), |
There was a problem hiding this comment.
Is this the desired behavior?
You might argue that Nox should prefer the uv in its own environment.
Or you might argue that we should prefer PATH lookup because it's the standard mechanism, and looking in the environment is just a fallback for an edge case.
We're taking the second stance here, but I don't have a strong opinion fwiw.
There was a problem hiding this comment.
I'd say principal of least surprise suggests the second is best IMO
There was a problem hiding this comment.
In scikit-build-core, I try to import CMake/ninja first before falling back on path search. For a Python library, using something coinstalled I think makes more sense. Otherwise you can’t pip install uv to get the latest uv if you have a system uv.
There was a problem hiding this comment.
Provided that the venv is activated, uv from the venv's bin would take precedence, so pip install uv would work. pipx is a bit of an edge case, and an argument could probably be made that it should expose its venv/bin folder to its exported scripts.
There was a problem hiding this comment.
Not sure why it should be assumed that it's active? Usually environments are designed to be used without being active (though there are some exceptions).
Right now, it might not seem to make much difference, but as uv gets older and pre-installed in more places, it's likely to be much more important to be able to pip install a modern uv. That's really important for cmake/ninja (more for cmake since it changes more).
There was a problem hiding this comment.
The other option for the action would be to pre-install uv (making it globally available), which isn’t terrible.
There was a problem hiding this comment.
We can place the minimum version we require in the extra. Then it will get upgraded if it’s too old for us.
I'm not worried about our requirements, but about those of our users when calling session.install. Also, Nox releases less frequently than uv, by far.
You can also use pipx reinstall nox[uv].
That's not a good experience for users. People will use pipx install nox[uv] and pipx upgrade-all, and it shouldn't leave them with an outdated uv.
Pipx run (which is almost exclusively what I use) reinstalls after a week.
I don't think the majority of users uses pipx run nox. (And for those injecting packages like nox-poetry that's not an option.)
The extra is useful to make this work in the action.
If there's another way to make this work in the action that doesn't cause problems for our users and support overhead for us, I'd say that's worth investigating.
There was a problem hiding this comment.
It looks like this might be very easily fixable from pipx's side (see linked issue).
There was a problem hiding this comment.
Ok I'm fine to go ahead with this here.
This comment was marked as outdated.
This comment was marked as outdated.
Fixes #791