npm package: don't move binary from optional one & instead link (or copy if can't hardlink) #25303

RiskyMH · 2025-12-02T09:53:34Z

In theory fixes #25031 because the original file will still be present. This isn't a perfect solution as it is a lot of wasted effort in checking and moving again, but it should fix the intimidate issue.

…opy if can't hardlink)

robobun · 2025-12-02T09:53:44Z

^{Updated 1:53 AM PT - Dec 2nd, 2025}

❌ @RiskyMH, your commit 2775cb6 has some failures in Build #32838 (All Failures)

🧪 To try this PR locally:

bunx bun-pr 25303

That installs a local version of the PR into your bun-25303 executable, so you can run:

bun-25303 --bun

coderabbitai · 2025-12-02T09:58:11Z

Walkthrough

Replaced two-step relocation pattern for bun binaries with direct hard links. Changed from rename(path, ...bun.exe) followed by link(...bun.exe, ...bunx.exe) to link(path, ...bun.exe) and link(path, ...bunx.exe). Removes intermediate rename operation while maintaining equivalent control flow and error handling.

Changes

Cohort / File(s)	Summary
Binary linking implementation `packages/bun-release/src/npm/install.ts`	Replaced two-step relocation pattern with direct hard links from original path to both bun.exe and bunx.exe targets, eliminating intermediate rename operation

Suggested reviewers

Jarred-Sumner
cirospaciari

Pre-merge checks

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description is incomplete; it lacks the required 'What does this PR do?' and 'How did you verify your code works?' sections from the template.	Add the missing template sections: explain what the PR does and how the changes were verified to work correctly.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: replacing binary movement with linking (or copying as fallback) instead of moving, which directly addresses the issue.
Linked Issues check	✅ Passed	The PR directly addresses issue #25031 by replacing the move operation with linking/copying, which preserves the original file and should prevent subsequent install failures.
Out of Scope Changes check	✅ Passed	All changes are scoped to the install.ts file and directly relate to the linking mechanism, with no extraneous modifications outside the stated objective.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between e9e9324 and 2775cb6.

📒 Files selected for processing (1)

packages/bun-release/src/npm/install.ts (1 hunks)

🧰 Additional context used

🧠 Learnings (15)

📓 Common learnings

Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24880
File: packages/bun-vscode/package.json:382-385
Timestamp: 2025-11-20T19:51:32.288Z
Learning: In the Bun repository, dependencies may be explicitly added to package.json files (even when not directly imported in code) to force version upgrades on transitive dependencies, particularly as part of Aikido security scanner remediation to ensure vulnerable transitive dependencies resolve to patched versions.

Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: src/bun.js/api/BunObject.zig:2514-2521
Timestamp: 2025-10-08T13:56:00.875Z
Learning: For Bun codebase: prefer using `bun.path` utilities (e.g., `bun.path.joinAbsStringBuf`, `bun.path.join`) over `std.fs.path` functions for path operations.

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/building-bun.mdc:0-0
Timestamp: 2025-11-24T18:34:55.173Z
Learning: Applies to src/**/*.{cpp,zig} : Execute files using `bun bd <file> <...args>`; never use `bun <file>` directly as it will not include your changes

Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/js/node/test/{parallel,sequential}/*.js : For test/js/node/test/{parallel,sequential}/*.js files without a .test extension, use `bun bd <file>` instead of `bun bd test <file>` since these expect exit code 0 and don't use bun's test runner

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Add tests for new Bun runtime functionality

📚 Learning: 2025-10-19T02:52:37.412Z

Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/tsconfig.json:1-15
Timestamp: 2025-10-19T02:52:37.412Z
Learning: In the Bun repository, packages under packages/ (e.g., bun-otel) can follow a TypeScript-first pattern where package.json exports point directly to .ts files (not compiled .js files). Bun natively runs TypeScript, so consumers import .ts sources directly and receive full type information without needing compiled .d.ts declaration files. For such packages, adding "declaration": true or "outDir" in tsconfig.json is unnecessary and would break the export structure.
<!-- [remove_learning]
ceedde95-980e-4898-a2c6-40ff73913664

Applied to files:

packages/bun-release/src/npm/install.ts

📚 Learning: 2025-11-24T18:37:11.466Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes

Applied to files:

packages/bun-release/src/npm/install.ts

📚 Learning: 2025-09-12T18:16:50.754Z

Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.

Applied to files:

packages/bun-release/src/npm/install.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced

Applied to files:

packages/bun-release/src/npm/install.ts

📚 Learning: 2025-11-24T18:37:11.466Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `process.platform` and `process.arch` for platform detection; these values are inlined and dead-code eliminated at build time

Applied to files:

packages/bun-release/src/npm/install.ts

📚 Learning: 2025-11-20T19:51:32.288Z

Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24880
File: packages/bun-vscode/package.json:382-385
Timestamp: 2025-11-20T19:51:32.288Z
Learning: In the Bun repository, dependencies may be explicitly added to package.json files (even when not directly imported in code) to force version upgrades on transitive dependencies, particularly as part of Aikido security scanner remediation to ensure vulnerable transitive dependencies resolve to patched versions.

Applied to files:

packages/bun-release/src/npm/install.ts

📚 Learning: 2025-10-08T13:56:00.875Z

Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: src/bun.js/api/BunObject.zig:2514-2521
Timestamp: 2025-10-08T13:56:00.875Z
Learning: For Bun codebase: prefer using `bun.path` utilities (e.g., `bun.path.joinAbsStringBuf`, `bun.path.join`) over `std.fs.path` functions for path operations.

Applied to files:

packages/bun-release/src/npm/install.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/js/node/test/{parallel,sequential}/*.js : For test/js/node/test/{parallel,sequential}/*.js files without a .test extension, use `bun bd <file>` instead of `bun bd test <file>` since these expect exit code 0 and don't use bun's test runner

Applied to files:

packages/bun-release/src/npm/install.ts

📚 Learning: 2025-10-19T02:44:46.354Z

Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.

Applied to files:

packages/bun-release/src/npm/install.ts

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

Applied to files:

packages/bun-release/src/npm/install.ts

📚 Learning: 2025-11-20T01:27:21.509Z

Learnt from: RiskyMH
Repo: oven-sh/bun PR: 0
File: :0-0
Timestamp: 2025-11-20T01:27:21.509Z
Learning: In Bun's documentation, curl-based install commands should use `bun.com/install`, while Windows PowerShell irm (Invoke-RestMethod) commands should use `bun.sh/install` due to a bug with .com on Windows.

Applied to files:

packages/bun-release/src/npm/install.ts

📚 Learning: 2025-11-24T18:35:50.422Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/cli/**/*.{js,ts,jsx,tsx} : When testing Bun as a CLI, use the `spawn` API from `bun` with the `bunExe()` and `bunEnv` from `harness` to execute Bun commands and validate exit codes, stdout, and stderr

Applied to files:

packages/bun-release/src/npm/install.ts

📚 Learning: 2025-11-24T18:34:55.173Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/building-bun.mdc:0-0
Timestamp: 2025-11-24T18:34:55.173Z
Learning: Applies to src/**/*.{cpp,zig} : Execute files using `bun bd <file> <...args>`; never use `bun <file>` directly as it will not include your changes

Applied to files:

packages/bun-release/src/npm/install.ts

📚 Learning: 2025-10-26T01:32:04.844Z

Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.

Applied to files:

packages/bun-release/src/npm/install.ts

🧬 Code graph analysis (1)

packages/bun-release/src/npm/install.ts (1)

packages/bun-release/src/fs.ts (2)

link (161-171)

join (7-9)

🔇 Additional comments (1)

packages/bun-release/src/npm/install.ts (1)

127-128: Switch from rename to link/copy correctly preserves the optional binary

Using link(path, ...) here instead of rename(...) ensures the optional package’s binary stays in place while still creating bin/bun.exe and bin/bunx.exe (with the built‑in hard‑link‑with‑copy‑fallback behavior from fs.link), which matches the PR’s goal and should avoid breaking pnpm’s store on subsequent installs.

To be confident this fixes #25031 and stays fixed, please re-run the original pnpm monorepo reproduction (first and second pnpm install with bun as a devDependency) and, if feasible, add a small regression test around optimizeBun() in the packages/bun-release package to cover the “don’t move, only link/copy” behavior.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

npm package: don't move binary from optional one & instead link (or c…

2775cb6

…opy if can't hardlink)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

npm package: don't move binary from optional one & instead link (or copy if can't hardlink) #25303

npm package: don't move binary from optional one & instead link (or copy if can't hardlink) #25303

Uh oh!

RiskyMH commented Dec 2, 2025

Uh oh!

robobun commented Dec 2, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

npm package: don't move binary from optional one & instead link (or copy if can't hardlink) #25303

Are you sure you want to change the base?

npm package: don't move binary from optional one & instead link (or copy if can't hardlink) #25303

Uh oh!

Conversation

RiskyMH commented Dec 2, 2025

Uh oh!

robobun commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Dec 2, 2025

Walkthrough

Changes

Suggested reviewers

Pre-merge checks

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robobun commented Dec 2, 2025 •

edited

Loading