install: filter optionalDependencies by libc (glibc/musl) - #31123
Jarred-Sumner wants to merge 2 commits into
Conversation
`bun install` filtered optional dependencies by `os` and `cpu` but ignored the npm `libc` field, so a package published only for `musl` could be installed on a glibc system, and vice versa. Thread `libc` through the same path `os`/`cpu` already use: - record `libc` on `Package.Meta` (reusing an existing padding byte, so the binary lockfile layout and format version are unchanged; a lockfile written before this reads back as unconstrained and still installs) - copy it from the manifest in `Package::from_npm` and the refresh path - check it in `Meta::is_disabled`, treating an unset/empty `libc` as unconstrained so packages without a `libc` field keep installing - read/write it in the text lockfile - derive the host libc from `target_env` at compile time (a musl build runs on musl, a gnu build on glibc), replacing the hardcoded `Libc::CURRENT` - add a `--libc` flag mirroring `--os`/`--cpu` Adds tests covering `--libc` filtering, the combined os+cpu+libc case, and the invalid-value error.
|
Updated 2:21 AM PT - May 20th, 2026
❌ @autofix-ci[bot], your commit 4f55b07 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31123That installs a local version of the PR into your bun-31123 --bun |
WalkthroughThis PR adds ChangesLibc-based optional dependency filtering
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/lockfile/Tree.rs (1)
597-621:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVerbose skip reason misses libc-only mismatches.
When a package is disabled solely by libc, verbose mode emits no mismatch reason. Add a libc branch so filtered installs remain diagnosable.
Suggested patch
} else if !meta.arch.is_match(manager.options.cpu) { Output::pretty_errorln(format_args!( "<d>Skip installing<r> <b>{}<r> <d>- cpu mismatch<r>", bstr::BStr::new(name) )); + } else if !meta.libc.is_match(manager.options.libc) { + Output::pretty_errorln(format_args!( + "<d>Skip installing<r> <b>{}<r> <d>- libc mismatch<r>", + bstr::BStr::new(name) + )); }🤖 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 `@src/install/lockfile/Tree.rs` around lines 597 - 621, The verbose skip branch currently reports only cpu/os mismatches but omits libc-only cases; update the conditionals around pkg_metas[pkg_id as usize] in Tree.rs to include libc checks: extend the combined mismatch check to consider !meta.os.is_match(...), !meta.arch.is_match(...), and !meta.libc.is_match(manager.options.libc) (so the first branch covers multi-way mismatches), and add an else if that checks !meta.libc.is_match(manager.options.libc) and emits Output::pretty_errorln with a "<d>Skip installing<r> <b>{}<r> <d>- libc mismatch<r>" message (use the existing bstr::BStr::new(name) pattern); use the same pattern for any additional combined messages you deem necessary so libc-only and combination cases are diagnosable.
🤖 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.
Outside diff comments:
In `@src/install/lockfile/Tree.rs`:
- Around line 597-621: The verbose skip branch currently reports only cpu/os
mismatches but omits libc-only cases; update the conditionals around
pkg_metas[pkg_id as usize] in Tree.rs to include libc checks: extend the
combined mismatch check to consider !meta.os.is_match(...),
!meta.arch.is_match(...), and !meta.libc.is_match(manager.options.libc) (so the
first branch covers multi-way mismatches), and add an else if that checks
!meta.libc.is_match(manager.options.libc) and emits Output::pretty_errorln with
a "<d>Skip installing<r> <b>{}<r> <d>- libc mismatch<r>" message (use the
existing bstr::BStr::new(name) pattern); use the same pattern for any additional
combined messages you deem necessary so libc-only and combination cases are
diagnosable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75e151c4-2bb1-4092-9844-928196c56e0c
📒 Files selected for processing (15)
src/install/PackageManager/CommandLineArguments.rssrc/install/PackageManager/PackageManagerLifecycle.rssrc/install/PackageManager/PackageManagerOptions.rssrc/install/lockfile.rssrc/install/lockfile/Package.rssrc/install/lockfile/Package/Meta.rssrc/install/lockfile/Tree.rssrc/install/lockfile/bun.lock.rssrc/install/lockfile/printer/tree_printer.rssrc/install_types/resolver_hooks.rstest/cli/install/bun-install-cpu-os.test.tstest/cli/install/dep-glibc-1.0.0.tgztest/cli/install/dep-linux-x64-glibc-1.0.0.tgztest/cli/install/dep-linux-x64-musl-2.0.0.tgztest/cli/install/dep-musl-2.0.0.tgz
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| pub fn is_disabled(&self, cpu: Architecture, os: OperatingSystem, libc: Libc) -> bool { | ||
| !self.arch.is_match(cpu) | ||
| || !self.os.is_match(os) | ||
| // `libc` is NONE both for packages without a `libc` field and for | ||
| // lockfiles written before libc was recorded; treat that as unconstrained. | ||
| || (self.libc != Libc::NONE && !self.libc.is_match(libc)) | ||
| } |
There was a problem hiding this comment.
🔴 get_native_binlink_replacement_package_id still matches only on meta.arch + meta.os and ignores the new meta.libc, so for a package with paired -gnu/-musl optionalDependencies it can return the wrong-libc variant — which after this PR is filtered out by is_disabled and never installed, leaving the binlink pointing at nothing. Add a target_libc: npm::Libc parameter, gate line 126 with && (meta.libc == npm::Libc::NONE || meta.libc.is_match(target_libc)), and thread manager.options.libc through the three call sites (PackageInstaller.rs:543, isolated_install/Installer.rs:2208, postinstall_optimizer.rs:219).
Extended reasoning...
What the bug is
PostinstallOptimizer::get_native_binlink_replacement_package_id (src/install/postinstall_optimizer.rs:103-132) selects which platform-specific optionalDependency should replace a meta-package's bin (the optimization that lets bun skip esbuild's postinstall by linking directly to @esbuild/linux-x64's binary). It picks the replacement by iterating the lockfile's resolution list and returning the first entry where:
meta.arch.is_match(target_cpu) && meta.os.is_match(target_os)There is no meta.libc check. This PR adds libc to Meta and threads it through is_disabled, the tree filter, the preinstall-state check, and the printer — but this function and its three callers were missed.
The code path
All three call sites read manager.options.cpu / manager.options.os but not the newly-added manager.options.libc:
- src/install/PackageInstaller.rs:543-551 — hoisted installer, passes
pkg_resolutions_lists[package_id].get(...)(the lockfile resolution slice for the meta-package, which contains all variants regardless of whether they were filtered at install time) - src/install/isolated_install/Installer.rs:2208-2215 — isolated installer, same
- src/install/postinstall_optimizer.rs:219-224 —
should_ignore_lifecycle_scripts, which decides whether to skip the postinstall in the first place
Why nothing else prevents it
The function iterates the meta-package's resolution slice from the lockfile, not the set of packages that survived is_disabled filtering. Line 123 requires the candidate to declare a specific arch and a specific os (!= ALL), and line 126 checks they match the target — but a @foo/linux-x64-gnu variant and a @foo/linux-x64-musl variant both declare cpu: ["x64"], os: ["linux"] and both pass on a linux-x64 host. Whichever appears first in the resolution slice wins, regardless of host libc.
Step-by-step proof
Take a package native-pkg opted into nativeDependencies (or hypothetically added to the default list) with:
"optionalDependencies": {
"@native-pkg/linux-x64-gnu": "1.0.0", // cpu:[x64] os:[linux] libc:[glibc]
"@native-pkg/linux-x64-musl": "1.0.0" // cpu:[x64] os:[linux] libc:[musl]
}On an Alpine (musl) linux-x64 host:
- Both variants are resolved into the lockfile (lockfiles are portable).
- Install filtering (this PR's change):
Meta::is_disablednow returnstruefor the-gnuvariant (meta.libc == GLIBC, hostlibc == MUSL), sois_filtered_dependency_or_workspaceskips it and it is not written to node_modules. Before this PR, both were installed. - Binlink selection:
get_native_binlink_replacement_package_iditeratesnative-pkg's resolutions[gnu, musl]. Forgnu:arch == X64 != ALL✓,os == LINUX != ALL✓,arch.is_match(X64)✓,os.is_match(LINUX)✓ → returns the-gnupackage_id. - The installer then creates a binlink targeting
@native-pkg/linux-x64-gnu, which was never installed — broken bin.
Before this PR step 2 didn't filter by libc, so whichever variant the optimizer picked was at least present on disk; this PR makes the inconsistency observable.
Impact
The default native-binlink list is currently ["esbuild", "@anthropic-ai/claude-code"] (postinstall_optimizer.rs:25-30), neither of which ships glibc/musl-split variants today, so out-of-the-box installs are unaffected. The bug bites users who add a libc-split package (e.g. @swc/core, @rollup/rollup-*, @biomejs/biome) to nativeDependencies in package.json — exactly the population this PR is for. It's also a latent hazard for anyone extending the default list. Given the PR's stated goal is "thread libc through the same filtering path os/cpu already use", this is squarely in scope.
Fix
Add a target_libc: npm::Libc parameter and mirror Meta::is_disabled:
if meta.arch.is_match(target_cpu)
&& meta.os.is_match(target_os)
&& (meta.libc == npm::Libc::NONE || meta.libc.is_match(target_libc))
{
return Some(resolution);
}Then thread manager.options.libc through PackageInstaller.rs:543, isolated_install/Installer.rs:2208, and pass it down through should_ignore_lifecycle_scripts to the internal call at postinstall_optimizer.rs:219.
| let parent_res = &pkg_resolutions[parent_pkg_id as usize]; | ||
|
|
||
| if pkg_metas[pkg_id as usize].is_disabled(manager.options.cpu, manager.options.os) { | ||
| if pkg_metas[pkg_id as usize].is_disabled(manager.options.cpu, manager.options.os, manager.options.libc) { |
There was a problem hiding this comment.
🟡 Two minor follow-ups now that is_disabled() checks libc: (1) the verbose-logging block immediately below (lines ~601-616) only has branches for "cpu & os mismatch" / "os mismatch" / "cpu mismatch", so a package skipped solely for a libc mismatch prints no "Skip installing" message under --verbose — adding an else branch for "libc mismatch" would give parity; (2) the doc comment on BuilderMethod::Filter (~line 460) still reads "'libc' (TODO)" — the TODO is now done and can be dropped.
Extended reasoning...
What
Two small cleanups in src/install/lockfile/Tree.rs that fall out of this PR's libc filtering, both cosmetic / non-functional:
(1) Missing verbose-log branch for libc-only mismatch
At line 597, is_disabled() now returns true when meta.libc doesn't match the target libc. But the verbose-logging block immediately below (lines ~601-616) still only checks os/cpu:
if !meta.os.is_match(manager.options.os) && !meta.arch.is_match(manager.options.cpu) {
// "- cpu & os mismatch"
} else if !meta.os.is_match(manager.options.os) {
// "- os mismatch"
} else if !meta.arch.is_match(manager.options.cpu) {
// "- cpu mismatch"
}
// no branch for libcWhen a package is disabled purely because of libc (os matches AND cpu matches, but meta.libc doesn't), none of the three conditions fire, so no "Skip installing" line is printed under --verbose. The package is still correctly skipped (return true at line 618) — this is purely a verbose-output completeness gap.
Step-by-step example: on a linux-x64-glibc host, install a package whose optionalDependencies includes pkg-linux-x64-musl declaring "os": ["linux"], "cpu": ["x64"], "libc": ["musl"] (exactly the scenario this PR targets). Run bun install --verbose:
is_disabled(x64, linux, glibc)→arch.is_match(x64)✓,os.is_match(linux)✓,libc != NONE && !libc.is_match(glibc)→ returnstrue.- Enter the verbose block.
!meta.os.is_match(linux)→false;!meta.arch.is_match(x64)→false. All three branches are skipped. - Function returns
true(package correctly filtered) but nothing was logged, whereas a darwin-only or arm64-only sibling would have printed "Skip installing … - os/cpu mismatch".
Fix: add a final else (or else if !meta.libc.is_match(manager.options.libc)) branch printing "- libc mismatch".
(2) Stale TODO in BuilderMethod::Filter doc comment
Around line 460, the doc comment on BuilderMethod::Filter reads:
We skip dependencies based on 'os', 'cpu', 'libc' (TODO), and omitted dependency types
This PR implements the libc check at line 597, so the "(TODO)" marker is now stale and can simply be removed.
Impact
Both are nits with zero functional impact — filtering behavior is correct. (1) is a minor --verbose UX inconsistency introduced by this PR; (2) is pure doc-comment cleanup directly in scope of what the PR implements.
| // The libc of the running binary: a musl-target build can only run on musl, | ||
| // a gnu-target build on glibc, so the compile-time target_env is the host libc. | ||
| #[cfg(all(target_os = "linux", target_env = "musl"))] | ||
| pub const CURRENT: Self = Self(Self::MUSL); | ||
| #[cfg(not(all(target_os = "linux", target_env = "musl")))] | ||
| pub const CURRENT: Self = Self(Self::GLIBC); |
There was a problem hiding this comment.
🟡 On non-Linux targets (macOS, Windows), Libc::CURRENT falls through to Self(Self::GLIBC), but npm only applies the libc check on Linux — on other platforms it ignores the field entirely. A package declaring "libc": ["musl"] without an os constraint would now be filtered out by bun on macOS/Windows where npm would install it. In practice such packages almost always also set "os": ["linux"] so the OS filter catches them first, but for npm parity the non-Linux fallback should be Libc::ALL (or gate the libc check in Meta::is_disabled on target_os = "linux").
Extended reasoning...
What the issue is
The new Libc::CURRENT constant uses two #[cfg] arms: one for all(target_os = "linux", target_env = "musl") → MUSL, and a catch-all not(...) arm → GLIBC. On Linux that's correct (a gnu-target build runs on glibc, a musl-target build on musl). But on macOS and Windows there is no libc in the npm sense, and the catch-all still resolves to GLIBC. Since this PR also wires Libc::CURRENT into Meta::is_disabled (via Options::libc → is_disabled(cpu, os, libc)), the libc check is now active on every platform.
How it diverges from npm
Per the npm docs and the npm-install-checks implementation, the libc field is only validated when process.platform === 'linux'; on any other platform the check is skipped entirely (libcOk = true). So under npm, a package that declares only "libc": ["musl"] (with no os constraint) installs fine on macOS or Windows.
Concrete walk-through
Consider running bun install on macOS with an optional dependency whose package.json contains "libc": ["musl"] and no os field:
Options::default()/ CLI defaults setself.libc = Libc::CURRENT, which on macOS isLibc(GLIBC)(resolver_hooks.rs:921–924).Package::from_npmcopiespackage_version.libc→meta.libc = Libc(MUSL).Meta::is_disabledevaluates the new clause:self.libc != Libc::NONE(true, it's MUSL)&& !self.libc.is_match(libc)→!(MUSL & GLIBC != 0)→!(0b100 & 0b010 != 0)→!false→ true.- The package is treated as disabled and skipped.
Under npm on the same macOS host, checkPlatform sees process.platform !== 'linux', skips the libc comparison, and installs the package. So this is a newly-introduced behavioral divergence — before this PR is_disabled never looked at libc at all.
Why nothing else prevents it
The guard in is_disabled (self.libc != Libc::NONE) only handles the package side (no libc declared / old lockfile). There's no corresponding guard on the host side for "this platform has no meaningful libc". The os check would normally catch real-world musl packages first (they all declare "os": ["linux"]), but that's a convention, not a guarantee.
Impact and fix
Practical impact is low: every real musl-only package on npm (@swc/core-linux-*-musl, @esbuild/linux-*, sharp variants, etc.) also declares "os": ["linux"], so the OS filter already excludes them on macOS/Windows and the libc check is never the deciding factor. This only bites a package that sets libc without os, which is arguably misconfigured. Still, since the PR's stated goal is npm parity and the fix is one line, it's worth tightening: either add a third #[cfg(not(target_os = "linux"))] pub const CURRENT: Self = Self::ALL; arm (so is_match always passes on non-Linux), or gate the libc clause in Meta::is_disabled on Linux.
| if pkg_meta.libc == Npm::Libc::NONE { | ||
| pkg_meta.libc = pkg.package.libc; | ||
| } |
There was a problem hiding this comment.
🟡 A few more spots that thread os/cpu but weren't updated for libc (all default to Libc::NONE = unconstrained, so no false filtering, just parity gaps): (1) pnpm-lock.yaml migration — src/install/pnpm.rs:918-924 parses os/cpu per package but still has // TODO: libc, and since pnpm.rs:1152 calls fetch_necessary_package_metadata_after_yarn_or_pnpm_migration::<false> the new backfill at lockfile.rs:1654 doesn't run for pnpm, so libc constraints from pnpm-lock.yaml are dropped; (2) Package::from_package_json (Package.rs:687-688) copies package_json.arch/os but not libc, and PackageJsonView (resolver_hooks.rs:1640-1641) has fn arch()/fn os() but no fn libc(), so folder/local deps ignore a package.json libc field; (3) the debug JSON dumper (lockfile_json_stringify_for_debugging.rs:379-401) emits arch/os arrays but not libc (debug-only, lowest priority).
Extended reasoning...
What these are
This PR's stated scope is "thread libc through the same filtering path os/cpu already use", and it does so for Meta::is_disabled, from_npm, bun.lock serialization, the tree filter, the preinstall-state check, the printer, and the yarn-migration backfill. There are three remaining places where os/cpu are read or emitted side-by-side and libc was not added. None cause false filtering (the default Meta.libc == Libc::NONE is treated as unconstrained by is_disabled), so these are completeness nits rather than functional bugs.
(1) pnpm-lock.yaml migration — src/install/pnpm.rs:924
The pnpm-lock.yaml parser reads per-package os and cpu at lines 918-923 but leaves // TODO: libc at line 924. pnpm-lock.yaml does record per-package libc: arrays. The fix is a one-liner mirroring the two cases above it:
if let Some(libc_expr) = package_obj.get(b"libc") {
pkg.meta.libc = npm::negatable_from_json::<npm::Libc>(&libc_expr)?;
}Why the new backfill does not cover this: the backfill this PR adds at lockfile.rs:1654-1656 sits inside if UPDATE_OS_CPU { ... } (line 1645), and pnpm.rs:1152 calls fetch_necessary_package_metadata_after_yarn_or_pnpm_migration::<false> — i.e. UPDATE_OS_CPU = false. Only yarn.rs:2042 passes <true>. So for pnpm migration the libc backfill is never reached.
Step-by-step: migrate a pnpm-lock.yaml whose packages: section has an entry with libc: [musl]. (a) pnpm.rs parses os/cpu, skips libc → pkg.meta.libc stays Libc::NONE. (b) fetch_necessary_package_metadata_after_yarn_or_pnpm_migration::<false> runs; UPDATE_OS_CPU is false, so the pkg_meta.libc = pkg.package.libc line is skipped. (c) On a glibc host, Meta::is_disabled evaluates self.libc != Libc::NONE → false, so the libc clause is bypassed and the musl-only package installs. This is the pre-PR behavior (both variants installed), not a regression — but it is the one item here with an observable effect, and it's a 3-line fix squarely in scope.
(2) from_package_json / PackageJsonView — Package.rs:687-688, resolver_hooks.rs:1640-1641
from_npm now sets package.meta.libc = package_version.libc (Package.rs:945), but the parallel from_package_json path still only copies package_json.arch and package_json.os (lines 687-688). The PackageJsonView trait at resolver_hooks.rs has fn arch() and fn os() but no fn libc(), and the resolver's PackageJSON struct (resolver/package_json.rs:205-206) likewise has no libc field. So folder/link dependencies and the auto-install resolver path ignore any "libc" field in their package.json.
Step-by-step: a folder dependency whose package.json declares "libc": ["musl"] on a glibc host: from_package_json leaves meta.libc = Libc::NONE → is_disabled short-circuits the libc check → package installs. Again no false filtering, just a missed filter. Real-world impact is very low — libc-split packages are npm-published native binaries that go through from_npm (which IS updated), not folder deps — but it is a parity gap with the os/cpu fields directly above. This one is a multi-file change (struct + trait + parser + from_package_json), so reasonable to defer.
(3) Debug JSON dumper — lockfile_json_stringify_for_debugging.rs:379-401
The debug stringifier emits "arch" (379-391) and "os" (393-404) arrays from pkg.meta but has no block for pkg.meta.libc. Now that bun.lock serializes libc (bun.lock.rs:1054-1062), the debug dump no longer reflects the full Meta it's printing.
Addressing the objection that this is below the bar: it's fair that this is debug-only, zero-functional-impact, and not a "filtering path". I'm including it only because (a) it sits literally next to the arch/os blocks it would mirror, (b) bun.lock.rs was updated for the same reason, and (c) it's bundled here with two in-scope items rather than filed standalone. If you'd rather skip it, the first two stand on their own.
Impact and fix
All three default to unconstrained, so the worst case is the pre-PR behavior (no filtering) on those paths — never an incorrectly-skipped package. (1) is a 3-line addition with an observable effect on pnpm→bun migration and is the most worth folding in; (2) is a parity gap requiring touching 3-4 files; (3) is purely cosmetic.
RiskyMH
left a comment
RiskyMH
left a comment
There was a problem hiding this comment.
From when I looked at this some time ago, the abbreviated registry api doesnt provide libc. If you have minimumReleaseAge, then it'll work as it needs full.
In my older pr I think I had a simple word check for common keys in libc like packages to then use normal api.
|
Rebased onto current main in #38786 (same design: libc in the Meta padding byte, Libc::CURRENT from the target env, --libc flag, libc in bun.lock), plus two things that turned out to be needed for it to change anything against registry.npmjs.org: the abbreviated manifest never includes libc, so #38786 infers it from the package name for packages that declare os/cpu (as pnpm does) and also applies that to already-written lockfiles; and package-lock.json / pnpm-lock.yaml migrations carry the field over. This one can probably be closed in favor of it. |
|
#38797 picks this up on current main and also covers the point raised above about the abbreviated registry document not carrying |
|
Closing this one in favor of #38797, which carries its design forward: What it adds is the part this branch could not get to on its own: the abbreviated registry document bun installs from never contains |
3 participants
What
bun installfilters optional dependencies by the npmosandcpufields, but ignores thelibcfield. As a result, on a glibc Linux hostbun installcan pull a package variant published only formusl(and vice versa) instead of the matching one —npm/yarninstall only the matching variant.Repro
A package whose
optionalDependenciesare glibc/musl variants of the same native binary:{ "optionalDependencies": { "pkg-linux-x64-glibc": "1.0.0", "pkg-linux-x64-musl": "1.0.0" } }where the two sub-packages declare
"libc": ["glibc"]and"libc": ["musl"]. On a glibc host,bun installinstalls both; it should install only the glibc one.Fix
Thread
libcthrough the same filtering pathos/cpualready use:libconPackage.Meta, reusing an existing padding byte — the binary lockfile layout and format version are unchanged, and a lockfile written before this change reads back as unconstrained and keeps installingPackage::from_npm(and the metadata-refresh path), and persist it in the text lockfileMeta::is_disabled; an unset/emptylibcis treated as unconstrained, so packages without alibcfield are unaffectedLibc::CURRENTfromtarget_envat compile time (a musl build runs on musl, a gnu build on glibc) — consistent with howOperatingSystem::CURRENT/Architecture::CURRENTderive fromtarget_os/target_arch— replacing a hardcodedglibc--libcflag mirroring--os/--cpu, for parity and cross-target installsTests
test/cli/install/bun-install-cpu-os.test.tsgains libc cases: filtering by--libc, the combined os+cpu+libc case (a glibc target must not pull a musl-only variant), and the invalid-value error.