Skip to content
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

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

cjolowicz
Copy link
Collaborator

Fixes #791

nox/virtualenv.py Outdated Show resolved Hide resolved
nox/virtualenv.py Outdated Show resolved Hide resolved
nox/virtualenv.py Outdated Show resolved Hide resolved
@cjolowicz cjolowicz force-pushed the fix-uv-lookup branch 5 times, most recently from 78d20af to 74dd8ac Compare March 6, 2024 08:08
@cjolowicz cjolowicz marked this pull request as ready for review March 6, 2024 08:09
@pytest.mark.parametrize(
["which_result", "find_uv_bin_result", "expected"],
[
("/usr/bin/uv", UV_IN_PIPX_VENV, "/usr/bin/uv"),
Copy link
Collaborator Author

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.

Copy link
Collaborator

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

Copy link
Collaborator

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.

Copy link
Contributor

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.

Copy link
Collaborator

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).

Copy link
Collaborator

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.

Copy link
Collaborator Author

@cjolowicz cjolowicz Mar 8, 2024

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related: pypa/pipx#925

Copy link
Contributor

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).

Copy link
Collaborator Author

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.

@cjolowicz cjolowicz marked this pull request as draft March 6, 2024 19:57
@cjolowicz

This comment was marked as outdated.

@cjolowicz cjolowicz marked this pull request as ready for review March 7, 2024 07:53
nox/virtualenv.py Outdated Show resolved Hide resolved
@cjolowicz cjolowicz merged commit 08813c3 into wntrblm:main Mar 8, 2024
22 checks passed
@cjolowicz cjolowicz deleted the fix-uv-lookup branch March 8, 2024 21:59
This was referenced Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

uv executable not found when installed with pipx install nox[uv]
4 participants