-
Notifications
You must be signed in to change notification settings - Fork 9.8k
fix(nix): handle missing .bun directory in canonicalize-node-modules #12636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(nix): handle missing .bun directory in canonicalize-node-modules #12636
Conversation
|
The following comment was made by an LLM, it may be inaccurate: No duplicate PRs found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Updates the Nix Bun helper script to avoid failing when Bun filtered installs don’t create node_modules/.bun, fixing nix build breakages related to Bun 1.3.8+.
Changes:
- Added a
safeReadDirhelper to avoid throwing on missing.bundirectory reads. - Added an early-exit path when
.bunis missing/empty to skip canonicalization.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async function safeReadDir(path: string) { | ||
| try { | ||
| return await readdir(path) | ||
| } catch { | ||
| return [] | ||
| } | ||
| } |
Copilot
AI
Feb 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
safeReadDir currently swallows all readdir errors and returns an empty list. That means non-ENOENT failures (e.g. EACCES, ENOTDIR, transient IO errors) will be treated as “no .bun entries” and the script will skip/exit successfully, which can hide real build problems. Consider only returning [] for expected missing-directory cases (e.g. err.code === "ENOENT") and rethrowing otherwise.
| // Early exit if .bun directory doesn't exist or is empty (bun 1.3.8+ with --filter may not create it) | ||
| if (directories.length === 0) { | ||
| console.log("[canonicalize-node-modules] no .bun directory found, skipping") |
Copilot
AI
Feb 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The early-exit log message says "no .bun directory found", but this branch also triggers when .bun exists but is empty (and also if safeReadDir returns [] for other errors). Consider updating the message to reflect the actual condition (missing or empty/unreadable) so build logs are accurate.
| // Early exit if .bun directory doesn't exist or is empty (bun 1.3.8+ with --filter may not create it) | |
| if (directories.length === 0) { | |
| console.log("[canonicalize-node-modules] no .bun directory found, skipping") | |
| // Early exit if .bun directory is missing, empty, or unreadable (bun 1.3.8+ with --filter may not create it) | |
| if (directories.length === 0) { | |
| console.log("[canonicalize-node-modules] .bun directory missing, empty, or unreadable; skipping") |
|
your root cause is wrong, see #12493 as the root cause |
5fe98be to
bbfd805
Compare
Use safeReadDir and isDirectory helpers (consistent with normalize-bun-binaries.ts) and replace dynamic semver import with Bun.semver.order. Fixes anomalyco#12632, anomalyco#12603, anomalyco#12602
bbfd805 to
9789de6
Compare
It's not, just test bun with and without the filter option. |
|
but... only because of #12493 as the root cause |
|
#12493 only triggered the nix-hashes workflow which exposed the latent bug. The actual regression is #11660 ( Either way, both would have been caught if the nix-desktop workflow hadn't been disabled in a92b792 without replacement. See the Root Cause section in the PR description. |
|
the issue is that #12493 changed from isolated installs to hoisted - nothing to do with the hashes. The reason that the filter exposed it is that the filter expects isolated installs and does not install the root package, only opencode and desktop subpackages. changing to hoisted bloats the install and is unnecessary the nix-desktop workflow was disabled since it was by far the most expensive workflow in the entire repo for little benefit. the builds werent cached and the dev branch moves far too fast for it to be any better of a smoke test vs nix users just seeing a failure and reporting it. plus opencode isnt a nix-first project, the native nix packaging is entirely community supported so bogging them down with heavy CI isnt in their interests |
I do not think doing a PR referencing the people in the community working on the nix packaging, exposing the changes and the issue and asking for reviews to find out the best solution at keeping a good CI coverage is too much asking. But that's not how code flow in opencode, so community is fixing things afterwards instead ... |
|
As far as that pr giga referenced, yeah that definitely shouldn't have been merged. Especially considering it had random build script changes that were unrelated to that person's pr. Thank u @gigamonster256 for fixing. I feel like I need to familiarize myself w/ nix more to understand all these prs and who is doing what. |
|
Superseded by #12694. A lightweight nix CI, even at smoke test level, would help catch obvious issues like this without requiring deep nix expertise. #12175 adds exactly that - it's currently failing due to git dependencies in cargo for opencode desktop (#11755), but the workflow is non-required for merging PRs. |
Summary
safeReadDirandisDirectoryhelpers (consistent withnormalize-bun-binaries.ts)semverimport withBun.semver.orderProblem
Since d29dfe3 ("chore: reduce nix fetching (#11660)" by @gigamonster256), bun install uses
--filterflags which may not create thenode_modules/.bundirectory. This causescanonicalize-node-modules.tsto fail with:Root Cause
The nix-desktop workflow was disabled in a92b792 by @thdxr without replacement. Nix package builds are no longer tested in CI, allowing regressions like this to be merged.
A pre-merge nix-eval workflow like #12175 would have caught this.
Solution
safeReadDirto gracefully handle missing directory (same pattern asnormalize-bun-binaries.ts)Bun.semver.orderinstead of importing semver from.bun/node_modulesisDirectoryhelper for consistencyFixes #12632, #12603, #12602