Preserve absolute paths in uv add for local dependencies#17316
Preserve absolute paths in uv add for local dependencies#17316nooscraft wants to merge 4 commits intoastral-sh:mainfrom
uv add for local dependencies#17316Conversation
When adding a local package with an absolute path like `uv add /path/to/package`, keep it absolute instead of converting to relative. Relative paths stay relative. Fixes astral-sh#17307
Keep the path format as provided by the user instead of always converting to relative paths. Workspace members remain relative for portability.
EliteTK
left a comment
EliteTK
left a comment
There was a problem hiding this comment.
I've had a look around / and manually tested some corner cases and it seems like this approach should work for the forward path. But...
There's places where we make up a PathBuiltDist on the spot e.g.:
uv/crates/uv-resolver/src/lock/mod.rs
Lines 2805 to 2809 in cd55d1c
And it seems like this change might end up with us getting absolute paths in places where previously we would want relative ones.
I haven't found a case like that yet, but it may be worth changing the tests where the snapshots have changed to using relative paths instead, just to see if it shakes out a case like this.
Overall, I'm not experienced enough in this area of the code to be sure if this is the best approach for this. I'll let others comment though.
crates/uv-resolver/src/lock/mod.rs
Outdated
| /// Check if the user originally provided an absolute path by inspecting the VerbatimUrl. | ||
| fn was_absolute_path(url: &VerbatimUrl) -> bool { | ||
| // Check the original user-provided string if available | ||
| if let Some(given) = url.given() { | ||
| // Check if the given string starts with "/" (Unix) or contains ":" (Windows drive) | ||
| let path_str = given.strip_prefix("file://").unwrap_or(given); | ||
| Path::new(path_str).is_absolute() | ||
| } else { | ||
| // Fallback: assume file:// URLs with absolute paths were originally absolute | ||
| url.scheme() == "file" | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Given that we now have 4 places where we are doing this, I think it's may be a candidate for just having a function. Something which also incorporates this logic:
if was_absolute_path(&url) {
std::path::absolute(&install_path)
} else {
relative_to(&install_path, root)
.or_else(|_| std::path::absolute(&install_path))
}So the call sites can just become absolute_or_relative_to(instal_path, root, url).map_err(...)?.
Not sure about the name or location, will get back to you on that.
There was a problem hiding this comment.
Thanks for the review comments and I have addressed the suggestion.
Updated the path resolution methods to maintain the original absolute or relative nature of paths provided by the user. Introduced a new function, `absolute_or_relative_to`, to streamline the handling of paths in various contexts, ensuring that workspace members remain relative while external dependencies respect the user's original input.
2 participants
When adding a local package with an absolute path like
uv add /path/to/package, keep it absolute instead of converting to relative. Relative paths stay relative.Fixes #17307