Repository navigation
Conversation
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
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.
| /// 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.
## Summary Follow-up to #18176. This fixes the remaining absolute/relative path issues when `uv add` edits `pyproject.toml`. `uv add` now preserves the user's path form: - Relative paths remain relative. - Absolute paths and literal `file://` URLs remain absolute. - URLs containing expanded variables remain relative, preserving the portability behavior from #18680 for both named and unnamed requirements. This applies to local directories and local archives. * Closes #17307 * Closes #17316 ## Test Plan - Integration coverage for relative paths, absolute paths, and literal `file://` URLs. - Integration coverage for directory and archive sources. - Regression coverage for expanded-variable URLs. - `cargo nextest run -E 'test(add_relative_and_absolute_paths) or test(add_relative_and_absolute_archives) or test(lock_pep508_urls_with_vars)'` - `cargo nextest run -p uv-pep508 -p uv-pypi-types` - `cargo clippy --workspace --all-targets --all-features --locked -- -D warnings` - `cargo xwin clippy --workspace --all-targets --all-features --locked -- -D warnings`
## Summary Follow-up to #18176. This fixes the remaining absolute/relative path issues when `uv add` edits `pyproject.toml`. `uv add` now preserves the user's path form: - Relative paths remain relative. - Absolute paths and literal `file://` URLs remain absolute. - URLs containing expanded variables remain relative, preserving the portability behavior from #18680 for both named and unnamed requirements. This applies to local directories and local archives. * Closes #17307 * Closes #17316 ## Test Plan - Integration coverage for relative paths, absolute paths, and literal `file://` URLs. - Integration coverage for directory and archive sources. - Regression coverage for expanded-variable URLs. - `cargo nextest run -E 'test(add_relative_and_absolute_paths) or test(add_relative_and_absolute_archives) or test(lock_pep508_urls_with_vars)'` - `cargo nextest run -p uv-pep508 -p uv-pypi-types` - `cargo clippy --workspace --all-targets --all-features --locked -- -D warnings` - `cargo xwin clippy --workspace --all-targets --all-features --locked -- -D warnings`
## Summary Follow-up to #18176. This fixes the remaining absolute/relative path issues when `uv add` edits `pyproject.toml`. `uv add` now preserves the user's path form: - Relative paths remain relative. - Absolute paths and literal `file://` URLs remain absolute. - URLs containing expanded variables remain relative, preserving the portability behavior from #18680 for both named and unnamed requirements. This applies to local directories and local archives. * Closes #17307 * Closes #17316 ## Test Plan - Integration coverage for relative paths, absolute paths, and literal `file://` URLs. - Integration coverage for directory and archive sources. - Regression coverage for expanded-variable URLs. - `cargo nextest run -E 'test(add_relative_and_absolute_paths) or test(add_relative_and_absolute_archives) or test(lock_pep508_urls_with_vars)'` - `cargo nextest run -p uv-pep508 -p uv-pypi-types` - `cargo clippy --workspace --all-targets --all-features --locked -- -D warnings` - `cargo xwin clippy --workspace --all-targets --all-features --locked -- -D warnings`
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