mirror of
https://github.com/pnpm/pnpm.git
synced 2026-07-19 12:12:34 -04:00
fix(pacquet/fs): fall back to absolute target across Windows drives (#11721)
* fix(pacquet/fs): fall back to absolute target across Windows drives
`pathdiff::diff_paths` does not return `None` when the two paths sit on
different Windows drive roots; it produces a path that climbs out with
`..` and then re-anchors with the target's drive letter (for example,
`..\..\C:\Users\...`). Windows rejects such a symlink target with
`ERROR_INVALID_PARAMETER` (os error 87), which is the failure mode CI
on Windows hits when the workspace lives on `D:` and the global store
installed by `setup-pnpm` lives on `C:`.
Node.js's `path.win32.relative` returns the absolute `to` argument
when the two paths share no common root, which is the behavior
upstream `symlink-dir` relies on. Mirror that by detecting unequal
`Component::Prefix` values up front and returning the absolute target
unchanged. Drive-letter comparison is case-insensitive to match
NTFS and Node.js semantics.
* fix(pacquet/fs): normalize verbatim Windows paths before relative diff
`Prefix::Disk('C')` and `Prefix::VerbatimDisk('C')` name the same
physical drive, but compare unequal as `Component::Prefix` values, so
the cross-root guard added in the previous commit would unnecessarily
fall back to an absolute target whenever one input was a verbatim
`\\?\C:\...` path. Worse, `pathdiff::diff_paths` itself would emit a
re-anchored garbage path on the variant-mismatched case if the guard
ever let it through.
Run both inputs through `dunce::simplified` on Windows, which strips
the `\\?\` prefix when the non-verbatim form is still valid, so
mixed verbatim and non-verbatim views of the same drive collapse
onto a common `Prefix::Disk` before the comparison and the diff.
Tighten `case_normalize` to map `VerbatimDisk` onto `Disk` for the
residual long-path case that `dunce` declines to simplify.
* style(pacquet/fs): split relative_target_inner per platform
`clippy::needless_return` rejects the explicit `return` in the
Windows `{ ... }` block because the block itself is the function's
tail expression on Windows. The block-with-cfg-attribute layout
also obscured that the function body shrinks to a single tail
expression on non-Windows. Extracting two `#[cfg]`-gated
`relative_target_inner` helpers makes both targets read as a
straight tail expression and drops the clippy violation.
* fix(pacquet/fs): make same_path_root variant-strict
Collapsing `Disk` and `VerbatimDisk` (and the UNC analogues) onto a
common variant in `same_path_root` is unsafe when one input is a
verbatim path that `dunce::simplified` declined to strip (e.g., a
path long enough that the non-verbatim form would exceed the legacy
limit). `same_path_root` would declare the two paths same-root, but
`pathdiff::diff_paths` would still walk `Component::Prefix` for
equality and emit a re-anchored garbage path on the variant
mismatch — the same `..\\..\\C:\\Users\\...` shape that triggers
`ERROR_INVALID_PARAMETER` (os error 87) on Windows.
Restore the variant-strict comparison so mismatched variants fall
back to an absolute target, which is the correct outcome on the
edge case. The common short-path mixed-form case (`\\?\C:\foo`
paired with `C:\bar`) still produces a relative target because
`dunce::simplified` normalizes the variants upstream of this check.
* docs(pacquet): tests are documentation; do not narrate them in code
Add an explicit rule to AGENTS.md: when a behavioral scenario or
edge case is already captured by a test (name, setup, assertions),
do not re-narrate it in a doc comment on the implementation. The
doc comment should state the contract; the test demonstrates the
behavior.
Apply the rule to the recently added Windows symlink fix. The doc
comments on `relative_target_for` and `same_path_root` had grown
into mini essays restating the `ERROR_INVALID_PARAMETER` failure
mode, the CI scenario, and the `dunce::simplified` rationale,
all of which is covered by the three Windows tests. Trim both to
the contract plus one short note on the only non-obvious invariant
(why `same_path_root` is variant-strict). The tests in turn shed
their multi-paragraph preambles in favor of self-explanatory names
and direct assertions, keeping only a one-line regression pointer
on the CI-failure test.
* docs(pacquet/fs): describe same_path_root match shape accurately
The previous doc claimed "same UNC share" as a match condition, but
the implementation requires identical `Prefix` after `dunce::simplified`
with only drive-letter case folding. UNC server/share differences in
case or `UNC`/`VerbatimUNC` variant fall back to absolute.
This commit is contained in:
1
Cargo.lock
generated
1
Cargo.lock
generated
@@ -2087,6 +2087,7 @@ name = "pacquet-fs"
|
||||
version = "0.0.1"
|
||||
dependencies = [
|
||||
"derive_more",
|
||||
"dunce",
|
||||
"junction",
|
||||
"miette 7.6.0",
|
||||
"pathdiff",
|
||||
|
||||
@@ -295,6 +295,7 @@ Rust-specific defaults:
|
||||
|
||||
- **Doc comments (`///`, `//!`) document the contract.** Preconditions, postconditions, panics, the reason the function exists. They are not a re-narration of the body.
|
||||
- **Do not restate at call sites what the callee's doc comment already says.** If `///` on the function says "no-op when …", the caller should not repeat that. Update the doc once; let every call site benefit.
|
||||
- **Tests are documentation. Do not duplicate them in prose.** If a behavioral scenario, edge case, failure mode, or worked example is already captured by a test (its name, its setup, its assertions), do not also narrate it in the doc comment on the implementation. The doc comment should state the contract once; the test demonstrates the behavior. The same applies in reverse: a test's own doc comment should not re-explain what the asserts already say, only the *why* if it is not obvious.
|
||||
- **`// SAFETY:`, `// TODO:`, and similar prefixes are the exception.** They signal hidden invariants or known follow-ups that a reader cannot recover from the code alone.
|
||||
|
||||
Prefer renaming, restructuring, or extracting a helper over leaving a comment. Reach for prose only when the right names and types genuinely cannot carry the information.
|
||||
|
||||
@@ -19,6 +19,7 @@ pathdiff = { workspace = true }
|
||||
tempfile = { workspace = true }
|
||||
|
||||
[target.'cfg(windows)'.dependencies]
|
||||
dunce = { workspace = true }
|
||||
junction = { workspace = true }
|
||||
|
||||
[lints]
|
||||
|
||||
@@ -36,17 +36,60 @@ pub fn symlink_dir(original: &Path, link: &Path) -> io::Result<()> {
|
||||
/// link's parent directory to `original`. Mirrors upstream's
|
||||
/// `resolveSrcOnTrueSymlink(src, dest) = path.relative(path.dirname(dest), src)`.
|
||||
///
|
||||
/// Callers in pacquet always pass absolute paths; if `pathdiff` cannot
|
||||
/// produce a relative form (one side is relative, or they live on
|
||||
/// different roots — only possible on Windows), fall back to the
|
||||
/// absolute `original` so the kernel still gets a valid path. This
|
||||
/// matches upstream's behavior when `path.relative` returns an
|
||||
/// absolute path because the two arguments share no common root.
|
||||
/// Returns an absolute path when no relative form exists between the
|
||||
/// two arguments. This matches Node.js's `path.win32.relative`, which
|
||||
/// returns the absolute `to` argument when the paths share no common
|
||||
/// root (different drives, different UNC shares).
|
||||
fn relative_target_for(original: &Path, link: &Path) -> PathBuf {
|
||||
let parent = link.parent().unwrap_or_else(|| Path::new(""));
|
||||
relative_target_inner(original, parent)
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
fn relative_target_inner(original: &Path, parent: &Path) -> PathBuf {
|
||||
let original = dunce::simplified(original);
|
||||
let parent = dunce::simplified(parent);
|
||||
if !same_path_root(original, parent) {
|
||||
return original.to_path_buf();
|
||||
}
|
||||
pathdiff::diff_paths(original, parent).unwrap_or_else(|| original.to_path_buf())
|
||||
}
|
||||
|
||||
#[cfg(not(windows))]
|
||||
fn relative_target_inner(original: &Path, parent: &Path) -> PathBuf {
|
||||
pathdiff::diff_paths(original, parent).unwrap_or_else(|| original.to_path_buf())
|
||||
}
|
||||
|
||||
/// Whether `a` and `b` have an identical `Component::Prefix` after
|
||||
/// `dunce::simplified`, with drive letters case-folded. UNC shares
|
||||
/// only match when their server/share are written with identical
|
||||
/// casing and variant — the check has to stay in lockstep with what
|
||||
/// `pathdiff::diff_paths` will tolerate, since a variant-tolerant or
|
||||
/// case-tolerant comparison here would let the downstream diff emit
|
||||
/// a re-anchored garbage path on a `Prefix` mismatch it cannot relate.
|
||||
#[cfg(windows)]
|
||||
fn same_path_root(a: &Path, b: &Path) -> bool {
|
||||
fn first_prefix(path: &Path) -> Option<std::path::Prefix<'_>> {
|
||||
match path.components().next()? {
|
||||
std::path::Component::Prefix(p) => Some(p.kind()),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
fn case_normalize(prefix: std::path::Prefix<'_>) -> std::path::Prefix<'_> {
|
||||
use std::path::Prefix::{Disk, VerbatimDisk};
|
||||
match prefix {
|
||||
Disk(d) => Disk(d.to_ascii_uppercase()),
|
||||
VerbatimDisk(d) => VerbatimDisk(d.to_ascii_uppercase()),
|
||||
other => other,
|
||||
}
|
||||
}
|
||||
match (first_prefix(a), first_prefix(b)) {
|
||||
(Some(pa), Some(pb)) => case_normalize(pa) == case_normalize(pb),
|
||||
(None, None) => true,
|
||||
_ => false,
|
||||
}
|
||||
}
|
||||
|
||||
/// Remove a symlink (or junction on Windows) previously created with
|
||||
/// [`symlink_dir`].
|
||||
///
|
||||
|
||||
@@ -7,7 +7,11 @@
|
||||
//! idempotent re-symlink, retargeting a stale link, and renaming a
|
||||
//! non-symlink occupant out of the way.
|
||||
|
||||
#[cfg(windows)]
|
||||
use super::relative_target_for;
|
||||
use super::{ForceSymlinkOutcome, force_symlink_dir, read_symlink_dir, symlink_dir};
|
||||
#[cfg(windows)]
|
||||
use std::path::Path;
|
||||
use std::{fs, path::PathBuf};
|
||||
use tempfile::tempdir;
|
||||
|
||||
@@ -145,6 +149,37 @@ fn force_symlink_dir_creates_missing_parent_directories() {
|
||||
assert_eq!(resolved_link, resolved_target);
|
||||
}
|
||||
|
||||
/// Regression for the Windows CI failure where the workspace lives
|
||||
/// on `D:` and the global store (installed by `setup-pnpm`) lives on
|
||||
/// `C:`: `pathdiff::diff_paths` produced a re-anchored garbage path
|
||||
/// that Windows rejected with `ERROR_INVALID_PARAMETER` (os error 87).
|
||||
#[cfg(windows)]
|
||||
#[test]
|
||||
fn windows_cross_drive_symlink_target_falls_back_to_absolute() {
|
||||
let target = Path::new(r"C:\Users\runneradmin\setup-pnpm\store\@babel\plugin-x");
|
||||
let link = Path::new(r"D:\a\pnpm\pnpm\node_modules\@babel\plugin-x");
|
||||
|
||||
assert_eq!(relative_target_for(target, link), target);
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
#[test]
|
||||
fn windows_same_drive_symlink_target_stays_relative() {
|
||||
let target = Path::new(r"C:\workspace\packages\pkg-a");
|
||||
let link = Path::new(r"C:\workspace\app\node_modules\pkg-a");
|
||||
|
||||
assert!(relative_target_for(target, link).is_relative());
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
#[test]
|
||||
fn windows_verbatim_and_plain_disk_resolve_to_same_root() {
|
||||
let target = Path::new(r"\\?\C:\workspace\packages\pkg-a");
|
||||
let link = Path::new(r"C:\workspace\app\node_modules\pkg-a");
|
||||
|
||||
assert!(relative_target_for(target, link).is_relative());
|
||||
}
|
||||
|
||||
/// After [`force_symlink_dir`] places a symlink, [`read_symlink_dir`]
|
||||
/// returns the same contents. The combination covers the round-trip
|
||||
/// pacquet's prune sweep needs (write a slot link with
|
||||
|
||||
Reference in New Issue
Block a user