-
Notifications
You must be signed in to change notification settings - Fork 662
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
Allow passing a venv to uv pip --python
#3064
Conversation
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.
Thanks! I tweaked the control flow to use a single metadata lookup.
I think this also closes #3062? |
}; | ||
Interpreter::query(executable, cache).map(Some) | ||
} | ||
Err(err) if err.kind() == std::io::ErrorKind::NotFound => { |
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 could consider erroring here if the request contains a separator, though I suppose it's not strictly necessary? The error in that case looks like error: Failed to locate Python interpreter at
.venv/foo/python`` already.
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.
Yeah, I don't think it's worth a special check.
uv pip --python
Will fix the failure, need #3072 first. |
Cow::Owned(path.join("Scripts/python.exe")) | ||
} else { | ||
Cow::Owned(path.join("bin/python")) | ||
} |
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 don't personally care, but this probably won't handle environments like cygwin or mingw64, as I think they use a bin
directory, because they are emulating Unix. They will presumably still have a .exe
suffix, though, as that's a Windows requirement.
I suspect it's better to ignore this problem until someone using one of those environments speaks up, because they will be able to give better information than my guesses...
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.
Yeah makes total sense. I think we already have this problem in a few places so I'm okay with using the same pattern until we fix it holistically...
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.
Looks like you merged while I was commenting!
Oh sorry, I set to auto-merge. |
Not a problem. And thanks for (improving and) merging this - glad my beginner-level Rust was worth it! |
Fixes #3060
Summary
Allows passing a virtual environment (the path to the directory, rather than the path to the Python interpreter within the directory) to the
--python
option of theuv pip
command.Test Plan
Tested manually to confirm that the expected new functionality works. The test suite still passes after this change.
I don't know how to add tests for a new feature like this. I would be happy to do so if someone can give me some pointers on how to do it.