-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Always reinstall local source trees passed to uv pip install
#12176
Conversation
07ad594
to
2e977a2
Compare
c8935ae
to
db7d652
Compare
crates/uv-cache/src/lib.rs
Outdated
|| path.is_some_and(|path| { | ||
paths | ||
.iter() | ||
.any(|target| same_file::is_same_file(path, target).unwrap_or(true)) |
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.
On failure we assume true
and always invalidate? That seems surprising.
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 feel strongly, but am curious for your thinking here)
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.
Removed it. I didn't realize it errors when one file doesn't exist; I just didn't think about it deeply, thanks!
Self::Packages(packages) => packages.contains(package_name), | ||
Self::Packages(.., paths) => paths | ||
.iter() | ||
.any(|target| same_file::is_same_file(path, target).unwrap_or(true)), |
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.
See previous comment
db7d652
to
dc2002f
Compare
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 pull request says "source trees" but this implementation is just local source trees, right? Can you change that?
Do we want to change this for remote source trees, i.e., Git repositories too? Should we add test coverage for that now?
Yeah, just local source trees. |
uv pip install
uv pip install
dc2002f
to
c001216
Compare
Summary
This ended up being more involved than expected. The gist is that we setup all the packages we want to reinstall upfront (they're passed in on the command-line); but at that point, we don't have names for all the packages that the user has specified. (Consider, e.g.,
uv pip install .
-- we don't have a name for.
, so we can't add it to the list ofReinstall
packages.)Now,
Reinstall
also accepts paths, so we can augmentReinstall
based on the user-provided paths.Closes #12038.