fix(uv): canonicalize workspace root to resolve symlinks during discovery#5
fix(uv): canonicalize workspace root to resolve symlinks during discovery#5beast-ofcourse wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesWorkspace Path Canonicalization
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
9f835c4 to
03f0954
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/uv-resolver/src/error.rs (1)
483-484: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale doc comment after narrowing to marker-only proxies.
The outer
collapse_proxiesdoc comment still says it collapses incompatibilities that "wrap anPubGrubPackageInner::Extrapackage," but the implementation now specifically targetsMarker-only proxies and intentionally excludesExtra/Group(per the new inline comment at Line 487-488). Update the doc comment to reflect the narrowed scope so it doesn't mislead future readers.📝 Suggested doc fix
/// Given a [`DerivationTree`], collapse any [`External::FromDependencyOf`] incompatibilities - /// wrap an [`PubGrubPackageInner::Extra`] package. + /// that wrap a marker-only [`PubGrubPackageInner::Marker`] proxy package. Proxies that carry + /// user-meaningful information (`Extra`, `Group`) are intentionally preserved. pub(crate) fn collapse_proxies(derivation_tree: ErrorTree) -> ErrorTree {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/uv-resolver/src/error.rs` around lines 483 - 484, The doc comment for collapse_proxies is stale and still describes collapsing incompatibilities that wrap a PubGrubPackageInner::Extra package. Update the comment to match the current implementation, which only collapses marker-only proxies and explicitly excludes Extra and Group, so the public documentation aligns with the behavior of collapse_proxies and its related marker handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/uv-workspace/src/workspace.rs`:
- Around line 319-324: The cache path comparison is mixing a canonicalized
workspace root with a merely normalized external cache root, which can break
ancestor checks on Windows. Update the path handling in
workspace_root/external_cache_root comparison logic so both values are
transformed the same way before calling starts_with, using the existing
normalization/canonicalization flow around workspace_root and
external_cache_root to keep the comparison consistent across platforms.
---
Nitpick comments:
In `@crates/uv-resolver/src/error.rs`:
- Around line 483-484: The doc comment for collapse_proxies is stale and still
describes collapsing incompatibilities that wrap a PubGrubPackageInner::Extra
package. Update the comment to match the current implementation, which only
collapses marker-only proxies and explicitly excludes Extra and Group, so the
public documentation aligns with the behavior of collapse_proxies and its
related marker handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 76e44ceb-d13e-497f-b21b-59d2e898dcfb
📒 Files selected for processing (2)
crates/uv-resolver/src/error.rscrates/uv-workspace/src/workspace.rs
| // Canonicalize to resolve symlinks, ensuring the workspace root path is | ||
| // consistent regardless of how it's accessed (e.g. via `/symlink/to/project` | ||
| // vs `/real/path/to/project`). | ||
| let path = fs_err::canonicalize(&path) | ||
| .map_err(WorkspaceErrorKind::Normalize)?; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does dunce::canonicalize avoid the \\?\ UNC prefix issue with std::fs::canonicalize on Windows for path comparisons like starts_with?
💡 Result:
Yes, dunce::canonicalize is designed specifically to avoid the \?\ UNC (extended-length) path prefix issue on Windows that affects std::fs::canonicalize [1][2]. By default, std::fs::canonicalize on Windows returns paths with the \?\ prefix (e.g., \?\C:\Windows), which can cause compatibility issues with many Windows applications and problems when performing path comparisons like starts_with [3][4]. The dunce::canonicalize function acts as a drop-in replacement that works as follows: - It calls std::fs::canonicalize internally [5]. - On Windows, it analyzes the resulting path and, if it is safe to do so, strips the \?\ prefix to return a legacy-compatible path (e.g., C:\Windows) [1][5]. - If the path cannot be safely converted—such as when it contains reserved filenames (e.g., COM, LPT) or other characteristics that require the extended-length syntax—it leaves the UNC prefix intact to ensure the path remains valid and robust [1][2]. Because dunce::canonicalize returns the most compatible (usually non-UNC) format whenever possible, it is much safer for path comparisons like starts_with than std::fs::canonicalize, as it prevents the unexpected presence of the \?\ prefix from breaking string-based logic [1][2]. On non-Windows platforms, dunce::canonicalize simply calls the standard library function, making it safe to use cross-platform [2].
Citations:
- 1: https://docs.rs/dunce/latest/dunce/index.html
- 2: https://docs.rs/crate/dunce/latest
- 3: std::fs::canonicalize returns UNC paths on Windows, and a lot of software doesn't support UNC paths rust-lang/rust#42869
- 4: https://doc.rust-lang.org/stable/std/fs/fn.canonicalize.html
- 5: https://docs.rs/dunce/latest/src/dunce/lib.rs.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file around the cited lines and the cache-root comparison.
sed -n '300,360p' crates/uv-workspace/src/workspace.rs
printf '\n---\n'
sed -n '1110,1175p' crates/uv-workspace/src/workspace.rs
printf '\n---\n'
rg -n "starts_with\\(|simplified\\(|canonicalize\\(|normalize_path\\(" crates/uv-workspace/src/workspace.rsRepository: beast-ofcourse/uv
Length of output: 6763
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the implementation of normalize_path and any Windows-specific path handling.
rg -n "fn normalize_path|normalize_path\\(" crates -g '!target'
printf '\n---\n'
fd -a normalize_path cratesRepository: beast-ofcourse/uv
Length of output: 5228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect normalize_path and its tests for Windows verbatim-path handling.
sed -n '230,310p' crates/uv-fs/src/path.rs
printf '\n---\n'
sed -n '680,860p' crates/uv-fs/src/path.rsRepository: beast-ofcourse/uv
Length of output: 9753
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the verbatim-path tests and simplified-path helpers.
sed -n '860,940p' crates/uv-fs/src/path.rs
printf '\n---\n'
rg -n "fn simplified|simplified_display|portable_display|from_normalized_path|\\?\\\\" crates/uv-fs/src/path.rs crates -g '!target'Repository: beast-ofcourse/uv
Length of output: 40887
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Simplified trait implementation to see how verbatim paths are handled.
sed -n '1,90p' crates/uv-fs/src/path.rsRepository: beast-ofcourse/uv
Length of output: 3431
Avoid comparing canonicalized and non-canonicalized cache paths on Windows.
workspace_root is canonicalized here, but external_cache_root is only normalized. On Windows, that can leave one side in \\?\ form and make starts_with miss an actual ancestor/descendant relationship, so cache-member filtering can behave incorrectly. Normalize both sides the same way before comparing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/uv-workspace/src/workspace.rs` around lines 319 - 324, The cache path
comparison is mixing a canonicalized workspace root with a merely normalized
external cache root, which can break ancestor checks on Windows. Update the path
handling in workspace_root/external_cache_root comparison logic so both values
are transformed the same way before calling starts_with, using the existing
normalization/canonicalization flow around workspace_root and
external_cache_root to keep the comparison consistent across platforms.
03f0954 to
be7339d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/uv/tests/workspace/workspace.rs (1)
2523-2549: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't exercise the
--lockedpath that actually reproduces the reported bug.The PR description states the original failure surfaced specifically with
uv lock --lockeddue to path mismatches between canonicalized and non-canonicalized code paths. This test only runs a plainuv lockfrom the symlink, which is less likely to catch that exact regression (a freshuv lockis generally more tolerant of path differences than--locked, which strictly compares outputs). Consider chaining a--lockedinvocation (or re-runninguv lock --lockedafter the initial lock) from the symlinked path to more faithfully guard against the reported issue (#19222).Suggested strengthening of the regression test
uv_snapshot!(context.filters(), context.lock().current_dir(&link), @" success: true exit_code: 0 ----- stdout ----- ----- stderr ----- Using CPython 3.12.[X] interpreter at: [PYTHON-3.12] Resolved 1 package in [TIME] "); + // Re-run with `--locked` from the symlinked path, matching the failure mode + // originally reported in astral-sh/uv#19222. + uv_snapshot!(context.filters(), context.lock().arg("--locked").current_dir(&link), @" + success: true + exit_code: 0 + ----- stdout ----- + ----- stderr ----- + Resolved 1 package in [TIME] + "); + Ok(())As per PR objectives: "later operations, such as
uv lock --locked, to fail because some code paths canonicalized paths while others did not, leading to mismatches."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/uv/tests/workspace/workspace.rs` around lines 2523 - 2549, The regression test in workspace_discovery_with_symlinked_root only exercises a plain uv lock, so it may miss the path-mismatch bug described for uv lock --locked. Update the test to first establish the lock and then run uv lock --locked from the symlinked current_dir on the same Workspace::discover code path, so the canonicalization mismatch is actually validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/uv/tests/workspace/workspace.rs`:
- Around line 2523-2549: The regression test in
workspace_discovery_with_symlinked_root only exercises a plain uv lock, so it
may miss the path-mismatch bug described for uv lock --locked. Update the test
to first establish the lock and then run uv lock --locked from the symlinked
current_dir on the same Workspace::discover code path, so the canonicalization
mismatch is actually validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ebdd4c0a-f581-49c4-886f-924a4a4e6996
📒 Files selected for processing (1)
crates/uv/tests/workspace/workspace.rs
1 participant
When the current working directory is accessed through a symlink (e.g., /symlink/to/project\ pointing to /real/path/to/project), workspace discovery would store the non-canonical path as the workspace root. This caused subsequent operations like \uv lock --locked\ to fail, because some downstream code canonicalized the path while others didn't, leading to path mismatches.
By canonicalizing the workspace root path early in \Workspace::discover, we ensure all paths derived from the workspace root are consistent regardless of how the project is accessed.
Fixes astral-sh#19222
Summary by CodeRabbit
uv lockfrom a symlinked workspace root, verifying discovery succeeds and resolves the canonical (real) root.