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: ensure 'uv' always works in a uv venv #818

Merged
merged 6 commits into from
Apr 15, 2024

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Apr 12, 2024

It's necessary sometimes to use the backend tool (pip, uv, conda, mamba, micromamba) inside a virtual environment. We already have a bit of special casing for it, making sure it's not considered "external". This PR just makes sure that .run("uv", ...) is always supported if you are using the uv backend. All the other backends are already gaurenteed to be on the path if they are active, so no change is needed (yet, anyway - this mechanism could also enable using pip from outside instead of always installing it, might be worth investigating in the future).

Followup to and generalizes #795.

@henryiii henryiii force-pushed the henryiii/fix/useuv branch from f288bc5 to ccb0ec4 Compare April 12, 2024 17:28
@henryiii henryiii marked this pull request as draft April 12, 2024 17:29
@henryiii henryiii force-pushed the henryiii/fix/useuv branch 3 times, most recently from 529d632 to 90c62b0 Compare April 12, 2024 20:10
@henryiii henryiii marked this pull request as ready for review April 12, 2024 22:10
@henryiii henryiii force-pushed the henryiii/fix/useuv branch from 36daaed to 7415271 Compare April 12, 2024 22:13
@henryiii
Copy link
Collaborator Author

henryiii commented Apr 14, 2024

Added handling for one more corner case: if a user installs uv into a uv environment, they'd probably expect the new uv to be used after they installed it, just like pip is for pip environments. This worked except for the "installed alongside nox" case. Now it matches the "on path" case. Tested locally with:

@nox.session(venv_backend="uv")
def uv(session):
    session.install("uv")
    session.install("pip")
$ pipx run --spec .[uv] nox -s uv
⚠️  nox is already on your PATH and installed at /usr/local/bin/nox. Downloading and running anyway.
nox > Running session uv
nox > Creating virtual environment (uv) using python in .nox/uv
nox > /Users/henryschreiner/.local/pipx/.cache/ee854022650cc85/bin/uv pip install uv
nox > uv pip install pip

@cjolowicz
Copy link
Collaborator

Can you add a source code comment to explain this edge case?

It would also be good to have a comment for the Path.samefile call in find_uv, and to add both branches to the test matrix in test_find_uv.

Signed-off-by: Henry Schreiner <[email protected]>
@henryiii henryiii force-pushed the henryiii/fix/useuv branch from c18f2ca to 8765007 Compare April 15, 2024 02:05
@henryiii
Copy link
Collaborator Author

Added comments and added to tests.

@henryiii henryiii merged commit 8f33d1c into wntrblm:main Apr 15, 2024
22 checks passed
@henryiii henryiii deleted the henryiii/fix/useuv branch April 15, 2024 05:26
@henryiii henryiii mentioned this pull request Apr 15, 2024
@edgarrmondragon
Copy link
Contributor

@henryiii wdyt of also not warning when uvx is used in uv venv?

@henryiii
Copy link
Collaborator Author

Seems good. Though you can always use "uv", "tool", "run" instead. I can see if I can make a PR. Could you check my other two open PRs? I'd like a set of eyes on them before merging, and I'd like to move toward a release soonish. :)

@edgarrmondragon
Copy link
Contributor

Seems good. Though you can always use "uv", "tool", "run" instead. I can see if I can make a PR.

Ah uv tool run works in the meantime, though I hate typing 8 more characters! 😅

Could you check my other two open PRs? I'd like a set of eyes on them before merging, and I'd like to move toward a release soonish. :)

Done they LGTM, though mind you I don't have write access here...

@henryiii henryiii mentioned this pull request Jan 31, 2025
@henryiii
Copy link
Collaborator Author

Yes, the approval shows up grey, that's fine, it's not a hard requirement and I do have write access. :)

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.

3 participants