mirror of
https://github.com/pnpm/pnpm.git
synced 2026-06-29 18:35:18 -04:00
fix(bins.linker): skip redundant .exe warning when hardlink is already correct (#12284)
Closes [pnpm/pnpm#12203](https://github.com/pnpm/pnpm/issues/12203). On Windows, `node` is linked by hardlinking `node.exe` directly rather than through a cmd-shim. The early-exit in `linkBin()` only recognized an existing cmd-shim, so on every warm install the existing (already correct) `node.exe` was treated as a conflict: pnpm warned `The target bin directory already contains an exe called node` and then removed and re-linked it. Because many commands re-link `node`, the warning was spammed. ### `@pnpm/bins.linker` Before warning and replacing an existing `.exe`, check whether it already refers to the link target via a new `isSameFile()` helper: - Compares the OS file identity (inode/device), read as `BigInt` to avoid the precision loss NTFS 64-bit file IDs suffer when cast to a `Number`. A zero/unreliable inode (common on Windows) is not treated as a match. - Falls back to a streaming, chunked (64 KB) content comparison when identity can't be established, so a byte-identical copy still counts as the same file without ever buffering a whole executable. A read error during the comparison is treated as "not the same file" so it can never abort bin linking. The early-return is scoped to the `node.exe` path, so non-`node` commands still fall through to the existing warn + remove + `cmdShim()` regeneration and never end up with a partially populated bins directory. ### pacquet pacquet never emitted this warning, but its Windows `link_node_bin` unconditionally removed and re-linked `node.exe` on every install. Ported the same same-file early-return to `cmd-shim` so warm installs skip that churn, using `same_file::Handle` for the cheap identity check (promoted from a transitive to a workspace dependency) with the same chunked content fallback. --------- Co-authored-by: Zoltan Kochan <z@kochan.io>
This commit is contained in:
6
.changeset/fix-bin-linker-node-exe-warning.md
Normal file
6
.changeset/fix-bin-linker-node-exe-warning.md
Normal file
@@ -0,0 +1,6 @@
|
||||
---
|
||||
"@pnpm/bins.linker": patch
|
||||
"pnpm": patch
|
||||
---
|
||||
|
||||
Skip the redundant "target bin directory already contains an exe called node" warning on Windows when the existing `node.exe` already matches the target (same hard link or identical content) [pnpm/pnpm#12203](https://github.com/pnpm/pnpm/issues/12203).
|
||||
1
Cargo.lock
generated
1
Cargo.lock
generated
@@ -3548,6 +3548,7 @@ dependencies = [
|
||||
"pacquet-fs",
|
||||
"pipe-trait",
|
||||
"rayon",
|
||||
"same-file",
|
||||
"serde_json",
|
||||
"tempfile",
|
||||
"walkdir",
|
||||
|
||||
@@ -135,6 +135,7 @@ pgp = { version = "0.19.0", default-features = false }
|
||||
rayon = { version = "1.12.0" }
|
||||
rmp-serde = { version = "1.3.0" }
|
||||
rusqlite = { version = "0.39.0", features = ["bundled"] }
|
||||
same-file = { version = "1.0.6" }
|
||||
serde = { version = "1.0.228", features = ["derive"] }
|
||||
serde_json = { version = "1.0.150", features = ["preserve_order", "raw_value"] }
|
||||
serde-saphyr = { version = "0.0.27" }
|
||||
|
||||
@@ -280,15 +280,23 @@ async function linkBin (cmd: CommandInfo, binsDir: string, opts?: LinkBinOptions
|
||||
} catch {}
|
||||
if (IS_WINDOWS) {
|
||||
const exePath = path.join(binsDir, `${cmd.name}${getExeExtension()}`)
|
||||
if (existsSync(exePath)) {
|
||||
globalWarn(`The target bin directory already contains an exe called ${cmd.name}, so removing ${exePath}`)
|
||||
await rimraf(exePath)
|
||||
}
|
||||
// node.exe must exist as a real executable, not a cmd-shim wrapper.
|
||||
// node.exe is the only bin pnpm links directly as a real executable rather
|
||||
// than through a cmd-shim, so the existing-exe handling only applies to it.
|
||||
// We could update our own cmd shims to support node.cmd, but we can't
|
||||
// control npm's cmd shims, which break when node resolves to node.cmd.
|
||||
// npm's cmd shims use `IF EXIST "%~dp0\node.exe"` to find the node binary.
|
||||
if (cmd.name === 'node' && cmd.path.toLowerCase().endsWith('.exe')) {
|
||||
const isNodeExe = cmd.name === 'node' && cmd.path.toLowerCase().endsWith('.exe')
|
||||
if (existsSync(exePath)) {
|
||||
// Skip warning and re-linking when the existing node.exe already matches
|
||||
// the target, otherwise every command that re-links node would spam the
|
||||
// warning below on warm installs.
|
||||
if (isNodeExe && await isSameFile(exePath, cmd.path)) {
|
||||
return
|
||||
}
|
||||
globalWarn(`The target bin directory already contains an exe called ${cmd.name}, so removing ${exePath}`)
|
||||
await rimraf(exePath)
|
||||
}
|
||||
if (isNodeExe) {
|
||||
try {
|
||||
await fs.link(cmd.path, exePath)
|
||||
} catch {
|
||||
@@ -361,6 +369,67 @@ async function linkBin (cmd: CommandInfo, binsDir: string, opts?: LinkBinOptions
|
||||
}
|
||||
}
|
||||
|
||||
// Reports whether two paths refer to the same file. A matching inode/device
|
||||
// pair (read as BigInts to avoid the precision loss of NTFS 64-bit file IDs)
|
||||
// proves a hard link cheaply. Whenever identity can't be established that way —
|
||||
// because the inodes genuinely differ or because Windows reports an unreliable
|
||||
// zero inode — we fall back to comparing the file contents after a quick size
|
||||
// check, which also treats a byte-identical copy as the same file.
|
||||
async function isSameFile (pathA: string, pathB: string): Promise<boolean> {
|
||||
const [statA, statB] = await Promise.all([
|
||||
fs.stat(pathA, { bigint: true }).catch(() => null),
|
||||
fs.stat(pathB, { bigint: true }).catch(() => null),
|
||||
])
|
||||
if (statA == null || statB == null) return false
|
||||
if (statA.ino && statB.ino && statA.ino === statB.ino && statA.dev === statB.dev) {
|
||||
return true
|
||||
}
|
||||
if (statA.size !== statB.size) return false
|
||||
return haveEqualContents(pathA, pathB)
|
||||
}
|
||||
|
||||
const FILE_COMPARE_CHUNK_SIZE = 64 * 1024
|
||||
|
||||
// Compares two equally-sized files chunk by chunk, so an executable is never
|
||||
// fully buffered in memory and a mismatch returns as early as possible.
|
||||
async function haveEqualContents (pathA: string, pathB: string): Promise<boolean> {
|
||||
const [fhA, fhB] = await Promise.all([
|
||||
fs.open(pathA, 'r').catch(() => null),
|
||||
fs.open(pathB, 'r').catch(() => null),
|
||||
])
|
||||
if (fhA == null || fhB == null) {
|
||||
await fhA?.close().catch(() => {})
|
||||
await fhB?.close().catch(() => {})
|
||||
return false
|
||||
}
|
||||
try {
|
||||
const bufA = Buffer.alloc(FILE_COMPARE_CHUNK_SIZE)
|
||||
const bufB = Buffer.alloc(FILE_COMPARE_CHUNK_SIZE)
|
||||
let position = 0
|
||||
for (;;) {
|
||||
// Reading sequentially is intentional: each iteration compares one chunk
|
||||
// and stops early on a mismatch or EOF.
|
||||
const [readA, readB] = await Promise.all([ // eslint-disable-line no-await-in-loop
|
||||
fhA.read(bufA, 0, FILE_COMPARE_CHUNK_SIZE, position),
|
||||
fhB.read(bufB, 0, FILE_COMPARE_CHUNK_SIZE, position),
|
||||
])
|
||||
if (readA.bytesRead !== readB.bytesRead) return false
|
||||
if (readA.bytesRead === 0) return true
|
||||
if (!bufA.subarray(0, readA.bytesRead).equals(bufB.subarray(0, readB.bytesRead))) {
|
||||
return false
|
||||
}
|
||||
position += readA.bytesRead
|
||||
}
|
||||
} catch {
|
||||
// A transient read error must not abort bin linking: treat the files as
|
||||
// different so the caller falls back to the warn + remove + relink path.
|
||||
return false
|
||||
} finally {
|
||||
await fhA.close().catch(() => {})
|
||||
await fhB.close().catch(() => {})
|
||||
}
|
||||
}
|
||||
|
||||
// `fixBin` chmods the bin's source file (which lives inside the store) to make
|
||||
// it executable and rewrites a Windows CRLF shebang to LF. Under the global
|
||||
// virtual store that source is `{storeDir}/links/...`, so on a read-only store
|
||||
|
||||
@@ -733,6 +733,70 @@ describe('node binary linking', () => {
|
||||
// No cmd-shim should be created since we return early
|
||||
expect(fs.existsSync(path.join(binTarget, `node${CMD_EXTENSION}`))).toBe(false)
|
||||
})
|
||||
|
||||
testOnWindows('linkBinsOfPackages() does not warn when node.exe is already the correct hardlink', async () => {
|
||||
const binTarget = temporaryDirectory()
|
||||
const nodeDir = temporaryDirectory()
|
||||
|
||||
fs.writeFileSync(path.join(nodeDir, 'node.exe'), 'fake-node-binary', 'utf8')
|
||||
|
||||
const pkgs = [
|
||||
{
|
||||
location: nodeDir,
|
||||
manifest: {
|
||||
name: 'node',
|
||||
version: '20.0.0',
|
||||
bin: { node: 'node.exe' },
|
||||
},
|
||||
},
|
||||
]
|
||||
|
||||
// First call creates the hardlink
|
||||
await linkBinsOfPackages(pkgs, binTarget)
|
||||
jest.mocked(globalWarn).mockClear()
|
||||
|
||||
// Second call should not warn because the .exe is already the correct hardlink
|
||||
await linkBinsOfPackages(pkgs, binTarget)
|
||||
|
||||
// The absence of a warning is the regression signal: the warn and the
|
||||
// `rimraf` that recreates node.exe live in the same branch, so no warning
|
||||
// means node.exe was left untouched. We don't assert on inode identity
|
||||
// because node.exe may legitimately be a copy rather than a hardlink (the
|
||||
// implementation falls back to copyFile when hardlinking fails).
|
||||
expect(globalWarn).not.toHaveBeenCalled()
|
||||
const exePath = path.join(binTarget, 'node.exe')
|
||||
expect(fs.existsSync(exePath)).toBe(true)
|
||||
expect(fs.readFileSync(exePath, 'utf8')).toBe('fake-node-binary')
|
||||
})
|
||||
|
||||
testOnWindows('linkBinsOfPackages() does not warn when node.exe has identical content but is not a hardlink', async () => {
|
||||
const binTarget = temporaryDirectory()
|
||||
const nodeDir = temporaryDirectory()
|
||||
|
||||
fs.writeFileSync(path.join(nodeDir, 'node.exe'), 'fake-node-binary', 'utf8')
|
||||
// Pre-place an independent copy with identical content (different inode), as
|
||||
// happens on filesystems where Windows reports a zero inode and the previous
|
||||
// link fell back to a copy.
|
||||
fs.writeFileSync(path.join(binTarget, 'node.exe'), 'fake-node-binary', 'utf8')
|
||||
|
||||
const pkgs = [
|
||||
{
|
||||
location: nodeDir,
|
||||
manifest: {
|
||||
name: 'node',
|
||||
version: '20.0.0',
|
||||
bin: { node: 'node.exe' },
|
||||
},
|
||||
},
|
||||
]
|
||||
|
||||
await linkBinsOfPackages(pkgs, binTarget)
|
||||
|
||||
expect(globalWarn).not.toHaveBeenCalled()
|
||||
const exePath = path.join(binTarget, 'node.exe')
|
||||
expect(fs.existsSync(exePath)).toBe(true)
|
||||
expect(fs.readFileSync(exePath, 'utf8')).toBe('fake-node-binary')
|
||||
})
|
||||
})
|
||||
|
||||
test('linkBins() resolves conflicts using BIN_OWNER_OVERRIDES (npx owned by npm)', async () => {
|
||||
|
||||
@@ -17,6 +17,7 @@ derive_more = { workspace = true }
|
||||
miette = { workspace = true }
|
||||
pipe-trait = { workspace = true }
|
||||
rayon = { workspace = true }
|
||||
same-file = { workspace = true }
|
||||
serde_json = { workspace = true }
|
||||
walkdir = { workspace = true }
|
||||
|
||||
|
||||
@@ -588,6 +588,13 @@ fn link_node_bin(target_path: &Path, shim_path: &Path) -> Result<bool, LinkBinsE
|
||||
return Ok(false);
|
||||
}
|
||||
let exe_path = with_extension_appended(shim_path, "exe");
|
||||
// Skip the remove + relink churn on warm installs when `node.exe`
|
||||
// already refers to the source binary. Mirrors pnpm's same-file
|
||||
// early-return in
|
||||
// [`bins/linker/src/index.ts`](https://github.com/pnpm/pnpm/blob/06d2d3deb2/bins/linker/src/index.ts#L281-L308).
|
||||
if is_same_file(&exe_path, target_path) {
|
||||
return Ok(true);
|
||||
}
|
||||
remove_stale_bin(&exe_path)?;
|
||||
if fs::hard_link(target_path, &exe_path).is_err() {
|
||||
fs::copy(target_path, &exe_path).map_err(|error| LinkBinsError::LinkNodeBin {
|
||||
@@ -599,6 +606,72 @@ fn link_node_bin(target_path: &Path, shim_path: &Path) -> Result<bool, LinkBinsE
|
||||
Ok(true)
|
||||
}
|
||||
|
||||
/// Whether `a` and `b` are the same file. [`same_file::Handle`] proves a hard
|
||||
/// link cheaply via the OS file identity (device + inode on Unix, file index +
|
||||
/// volume serial on Windows). When that identity can't be obtained — a missing
|
||||
/// file, or a filesystem that doesn't expose a stable index — we fall back to
|
||||
/// comparing the file contents after a quick size check, which also treats a
|
||||
/// byte-identical copy as the same file. Mirrors `isSameFile` in pnpm's
|
||||
/// `bins.linker`.
|
||||
#[cfg(windows)]
|
||||
fn is_same_file(a: &Path, b: &Path) -> bool {
|
||||
if let (Ok(handle_a), Ok(handle_b)) =
|
||||
(same_file::Handle::from_path(a), same_file::Handle::from_path(b))
|
||||
&& handle_a == handle_b
|
||||
{
|
||||
return true;
|
||||
}
|
||||
match (std::fs::metadata(a), std::fs::metadata(b)) {
|
||||
(Ok(meta_a), Ok(meta_b)) => meta_a.len() == meta_b.len() && have_equal_contents(a, b),
|
||||
_ => false,
|
||||
}
|
||||
}
|
||||
|
||||
/// Compare two equally-sized files chunk by chunk, so an executable is never
|
||||
/// fully buffered in memory and a mismatch returns as early as possible.
|
||||
#[cfg(windows)]
|
||||
fn have_equal_contents(a: &Path, b: &Path) -> bool {
|
||||
const CHUNK_SIZE: usize = 64 * 1024;
|
||||
let (Ok(mut file_a), Ok(mut file_b)) = (std::fs::File::open(a), std::fs::File::open(b)) else {
|
||||
return false;
|
||||
};
|
||||
let mut buf_a = vec![0u8; CHUNK_SIZE];
|
||||
let mut buf_b = vec![0u8; CHUNK_SIZE];
|
||||
loop {
|
||||
let (Ok(read_a), Ok(read_b)) =
|
||||
(read_chunk(&mut file_a, &mut buf_a), read_chunk(&mut file_b, &mut buf_b))
|
||||
else {
|
||||
return false;
|
||||
};
|
||||
if read_a != read_b {
|
||||
return false;
|
||||
}
|
||||
if read_a == 0 {
|
||||
return true;
|
||||
}
|
||||
if buf_a[..read_a] != buf_b[..read_b] {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Read up to `buf.len()` bytes, looping over short reads so a full chunk is
|
||||
/// only short at end of file. Like [`std::io::Read::read_exact`] but tolerant
|
||||
/// of EOF.
|
||||
#[cfg(windows)]
|
||||
fn read_chunk(reader: &mut impl std::io::Read, buf: &mut [u8]) -> io::Result<usize> {
|
||||
let mut filled = 0;
|
||||
while filled < buf.len() {
|
||||
match reader.read(&mut buf[filled..]) {
|
||||
Ok(0) => break,
|
||||
Ok(n) => filled += n,
|
||||
Err(error) if error.kind() == io::ErrorKind::Interrupted => {}
|
||||
Err(error) => return Err(error),
|
||||
}
|
||||
}
|
||||
Ok(filled)
|
||||
}
|
||||
|
||||
/// Remove an existing dirent at `path`, swallowing `NotFound`. Used by
|
||||
/// [`link_node_bin`] to clear any prior shim / symlink / hardlink
|
||||
/// before laying down the new one. Any other IO error (`PermissionDenied`,
|
||||
|
||||
@@ -1272,6 +1272,55 @@ fn link_node_bin_hardlinks_node_exe_on_windows() {
|
||||
);
|
||||
}
|
||||
|
||||
/// Windows-only: when `node.exe` already has identical content to the
|
||||
/// source binary (e.g. a prior copy-fallback install), the linker must
|
||||
/// leave it in place instead of removing and relinking it. Exercises the
|
||||
/// content-comparison fallback, since the pre-existing copy has a different
|
||||
/// file identity than the source. Mirrors pnpm's same-file early-return at
|
||||
/// <https://github.com/pnpm/pnpm/blob/06d2d3deb2/bins/linker/src/index.ts#L281-L308>.
|
||||
#[cfg(windows)]
|
||||
#[test]
|
||||
fn link_node_bin_skips_relink_when_node_exe_already_correct() {
|
||||
use same_file::Handle;
|
||||
let tmp = tempdir().unwrap();
|
||||
let bin_target = tmp.path().join("bin_target");
|
||||
create_dir_all(&bin_target).unwrap();
|
||||
let node_dir = tmp.path().join("node_pkg");
|
||||
create_dir_all(&node_dir).unwrap();
|
||||
write_file(node_dir.join("node.exe"), "fake-node-binary").unwrap();
|
||||
// Pre-place an independent copy with identical content (a different file
|
||||
// identity), as an earlier copy-fallback install would leave behind.
|
||||
write_file(bin_target.join("node.exe"), "fake-node-binary").unwrap();
|
||||
|
||||
write_file(
|
||||
node_dir.join("package.json"),
|
||||
json!({"name": "node", "version": "20.0.0", "bin": {"node": "node.exe"}}).to_string(),
|
||||
)
|
||||
.unwrap();
|
||||
let manifest: Value =
|
||||
serde_json::from_slice(&read_file(node_dir.join("package.json")).unwrap()).unwrap();
|
||||
link_bins_of_packages::<Host>(
|
||||
&[PackageBinSource::new(node_dir.clone(), Arc::new(manifest))],
|
||||
&bin_target,
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let exe = bin_target.join("node.exe");
|
||||
assert_eq!(read_to_string(&exe).unwrap(), "fake-node-binary");
|
||||
// The pre-existing copy is left in place rather than removed and relinked:
|
||||
// node.exe must not become a hardlink to the source binary. When file
|
||||
// identity can't be obtained (the production code tolerates this), treat
|
||||
// them as distinct rather than panicking on a failed handle lookup.
|
||||
let relinked_to_source = matches!(
|
||||
(Handle::from_path(&exe), Handle::from_path(node_dir.join("node.exe"))),
|
||||
(Ok(exe_handle), Ok(source_handle)) if exe_handle == source_handle,
|
||||
);
|
||||
assert!(
|
||||
!relinked_to_source,
|
||||
"node.exe must stay the independent copy, not be relinked to the source",
|
||||
);
|
||||
}
|
||||
|
||||
/// Windows-only: when the node manifest declares a non-`.exe` source
|
||||
/// (uncommon but possible — e.g. a wrapper script), pnpm falls through
|
||||
/// to the regular cmd-shim path. Pacquet must too.
|
||||
|
||||
Reference in New Issue
Block a user