-
Notifications
You must be signed in to change notification settings - Fork 5.1k
install: filter optionalDependencies by libc (glibc/musl) #31123
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| use bun_install::integrity::Integrity; | ||
| use bun_install::npm::{Architecture, OperatingSystem}; | ||
| use bun_install::npm::{Architecture, Libc, OperatingSystem}; | ||
| use bun_install::{INVALID_PACKAGE_ID, Origin, PackageID}; | ||
| use bun_semver::String; | ||
|
|
||
|
|
@@ -18,7 +18,8 @@ pub struct Meta { | |
|
|
||
| pub arch: Architecture, | ||
| pub os: OperatingSystem, | ||
| pub _padding_os: u16, | ||
| pub libc: Libc, | ||
| pub _padding_os: u8, | ||
|
|
||
| pub id: PackageID, | ||
|
|
||
|
|
@@ -52,6 +53,7 @@ impl Default for Meta { | |
| _padding_origin: 0, | ||
| arch: Architecture::ALL, | ||
| os: OperatingSystem::ALL, | ||
| libc: Libc::NONE, | ||
| _padding_os: 0, | ||
| id: INVALID_PACKAGE_ID, | ||
| man_dir: String::default(), | ||
|
|
@@ -63,10 +65,14 @@ impl Default for Meta { | |
| } | ||
|
|
||
| impl Meta { | ||
| /// Does the `cpu` arch and `os` match the requirements listed in the package? | ||
| /// Does the `cpu` arch, `os`, and `libc` match the requirements listed in the package? | ||
| /// This is completely unrelated to "devDependencies", "peerDependencies", "optionalDependencies" etc | ||
| pub fn is_disabled(&self, cpu: Architecture, os: OperatingSystem) -> bool { | ||
| !self.arch.is_match(cpu) || !self.os.is_match(os) | ||
| 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)) | ||
| } | ||
|
Comment on lines
+70
to
76
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Extended reasoning...What the bug is
meta.arch.is_match(target_cpu) && meta.os.is_match(target_os)There is no The code pathAll three call sites read
Why nothing else prevents itThe function iterates the meta-package's resolution slice from the lockfile, not the set of packages that survived Step-by-step proofTake a package "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:
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. ImpactThe default native-binlink list is currently FixAdd a 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 |
||
|
|
||
| pub fn has_install_script(&self) -> bool { | ||
|
|
@@ -111,6 +117,7 @@ impl Meta { | |
| integrity: self.integrity, | ||
| arch: self.arch, | ||
| os: self.os, | ||
| libc: self.libc, | ||
| origin: self.origin, | ||
| has_install_script: self.has_install_script, | ||
| ..Meta::default() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -916,9 +916,18 @@ impl Libc { | |
|
|
||
| pub const ALL_VALUE: u8 = Self::GLIBC | Self::MUSL; | ||
|
|
||
| // TODO: (matches Zig — runtime libc detection) | ||
| // 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); | ||
|
Comment on lines
+919
to
924
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 On non-Linux targets (macOS, Windows), Extended reasoning...What the issue isThe new How it diverges from npmPer the npm docs and the Concrete walk-throughConsider running
Under npm on the same macOS host, Why nothing else prevents itThe guard in Impact and fixPractical impact is low: every real musl-only package on npm ( |
||
|
|
||
| #[cfg(all(target_os = "linux", target_env = "musl"))] | ||
| pub const CURRENT_NAME: &'static str = "musl"; | ||
| #[cfg(not(all(target_os = "linux", target_env = "musl")))] | ||
| pub const CURRENT_NAME: &'static str = "glibc"; | ||
|
|
||
| #[inline] | ||
| pub const fn none() -> Self { | ||
| Self::NONE | ||
|
|
||
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.
🟡 A few more spots that thread
os/cpubut weren't updated forlibc(all default toLibc::NONE= unconstrained, so no false filtering, just parity gaps): (1) pnpm-lock.yaml migration —src/install/pnpm.rs:918-924parsesos/cpuper package but still has// TODO: libc, and since pnpm.rs:1152 callsfetch_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) copiespackage_json.arch/osbut not libc, andPackageJsonView(resolver_hooks.rs:1640-1641) hasfn arch()/fn os()but nofn libc(), so folder/local deps ignore a package.jsonlibcfield; (3) the debug JSON dumper (lockfile_json_stringify_for_debugging.rs:379-401) emitsarch/osarrays but notlibc(debug-only, lowest priority).Extended reasoning...
What these are
This PR's stated scope is "thread
libcthrough the same filtering pathos/cpualready use", and it does so forMeta::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 whereos/cpuare read or emitted side-by-side andlibcwas not added. None cause false filtering (the defaultMeta.libc == Libc::NONEis treated as unconstrained byis_disabled), so these are completeness nits rather than functional bugs.(1) pnpm-lock.yaml migration —
src/install/pnpm.rs:924The pnpm-lock.yaml parser reads per-package
osandcpuat lines 918-923 but leaves// TODO: libcat line 924. pnpm-lock.yaml does record per-packagelibc:arrays. The fix is a one-liner mirroring the two cases above it: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 callsfetch_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 withlibc: [musl]. (a)pnpm.rsparsesos/cpu, skipslibc→pkg.meta.libcstaysLibc::NONE. (b)fetch_necessary_package_metadata_after_yarn_or_pnpm_migration::<false>runs;UPDATE_OS_CPUis false, so thepkg_meta.libc = pkg.package.libcline is skipped. (c) On a glibc host,Meta::is_disabledevaluatesself.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-1641from_npmnow setspackage.meta.libc = package_version.libc(Package.rs:945), but the parallelfrom_package_jsonpath still only copiespackage_json.archandpackage_json.os(lines 687-688). ThePackageJsonViewtrait at resolver_hooks.rs hasfn arch()andfn os()but nofn libc(), and the resolver'sPackageJSONstruct (resolver/package_json.rs:205-206) likewise has nolibcfield. 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_jsonleavesmeta.libc = Libc::NONE→is_disabledshort-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 throughfrom_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 frompkg.metabut has no block forpkg.meta.libc. Now that bun.lock serializeslibc(bun.lock.rs:1054-1062), the debug dump no longer reflects the fullMetait'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.