-
-
Notifications
You must be signed in to change notification settings - Fork 151
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
fix: look for uv next to python if it's not on PATH #795
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say principal of least surprise suggests the second is best IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related: pypa/pipx#925
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this might be very easily fixable from pipx's side (see linked issue).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I'm fine to go ahead with this here.
Fixes #791