Repository navigation
install: honour npm's libc field for optional native packages, add --libc
#40013
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
e649df8
c6345cf
cdbc65f
145b081
290e9dc
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 |
|---|---|---|
|
|
@@ -125,6 +125,9 @@ const SHARED_TAIL_PARAMS: &[ParamType] = &[ | |
| clap::param!( | ||
| "--os <STR>... Override operating system for optional dependencies (e.g., linux, darwin, * for all)" | ||
| ), | ||
| clap::param!( | ||
| "--libc <STR>... Override libc for optional dependencies (glibc, musl, * for all)" | ||
| ), | ||
| clap::param!("-h, --help Print this help menu"), | ||
| ]; | ||
|
|
||
|
|
@@ -593,6 +596,7 @@ pub struct CommandLineArguments { | |
|
|
||
| // CPU and OS overrides for optional dependencies | ||
| pub(crate) cpu: Npm::Architecture, | ||
| pub(crate) libc: Npm::Libc, | ||
| pub(crate) os: Npm::OperatingSystem, | ||
| } | ||
|
|
||
|
|
@@ -687,6 +691,7 @@ impl Default for CommandLineArguments { | |
| audit_ignore_list: &[], | ||
|
|
||
| cpu: Npm::Architecture::CURRENT, | ||
| libc: Npm::Libc::CURRENT, | ||
| os: Npm::OperatingSystem::CURRENT, | ||
| } | ||
| } | ||
|
|
@@ -1570,6 +1575,28 @@ Full documentation is available at <magenta>https://bun.com/docs/pm/cli/prune<r> | |
| cli.os = os_negatable.combine(); | ||
| } | ||
|
|
||
| let libc_values = args.options(b"--libc"); | ||
| if !libc_values.is_empty() { | ||
| let mut libc_negatable = Npm::Libc::NONE.negatable(); | ||
| for libc_str in libc_values { | ||
| libc_negatable.apply(libc_str); | ||
| if *libc_str == *b"*" { | ||
| libc_negatable.had_wildcard = true; | ||
| libc_negatable.had_unrecognized_values = false; | ||
| } else if libc_negatable.had_unrecognized_values | ||
| && *libc_str != *b"any" | ||
| && *libc_str != *b"none" | ||
| { | ||
| Output::err_generic( | ||
| "Invalid libc: '{}'. Valid values are: *, any, glibc, musl. Use !name to negate.", | ||
| (bstr::BStr::new(libc_str),), | ||
| ); | ||
| Global::crash(); | ||
| } | ||
| } | ||
| cli.libc = libc_negatable.combine(); | ||
| } | ||
|
|
||
|
Comment on lines
+1578
to
+1599
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. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win Extract the repeated negatable-flag parsing logic. This block duplicates the Extract a small generic helper (parameterized by the flag name, the error message, and the ♻️ Proposed helper shapefn parse_negatable_flag<T>(
args: &clap::Args<clap::Help>,
flag: &[u8],
invalid_value_fmt: &str,
) -> Option<T>
where
T: NegatableExt + Copy,
{
let values = args.options(flag);
if values.is_empty() {
return None;
}
let mut negatable = T::NONE.negatable();
for s in values {
negatable.apply(s);
if *s == *b"*" {
negatable.had_wildcard = true;
negatable.had_unrecognized_values = false;
} else if negatable.had_unrecognized_values && *s != *b"any" && *s != *b"none" {
Output::err_generic(invalid_value_fmt, (bstr::BStr::new(s),));
Global::crash();
}
}
Some(negatable.combine())
}Then: if let Some(libc) = parse_negatable_flag::<Npm::Libc>(
args,
b"--libc",
"Invalid libc: '{}'. Valid values are: *, any, glibc, musl. Use !name to negate.",
) {
cli.libc = libc;
}🤖 Prompt for AI Agents
Collaborator
Author
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. Reasonable, but it would rewrite the pre-existing
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.
🐇 You are interacting with an AI system. |
||
| if matches!(subcommand, Subcommand::Add | Subcommand::Install) { | ||
| cli.dependency_group = if args.flag(b"--development") || args.flag(b"--dev") { | ||
| Options::DependencyGroup::DEV | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1507,6 +1507,9 @@ | |
| if pkg_meta.arch == Npm::Architecture::ALL { | ||
| pkg_meta.arch = pkg.package.cpu; | ||
| } | ||
| if pkg_meta.libc == Npm::Libc::NONE { | ||
| pkg_meta.libc = pkg.package.libc; | ||
| } | ||
|
Check warning on line 1512 in src/install/lockfile.rs
|
||
|
Comment on lines
+1510
to
+1512
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. 🟡 A fifth Extended reasoning...What the bug is
npm's Why this is not a duplicateThe existing sibling-site comment at Step-by-step trigger
Why existing code doesn't prevent itThe metadata-refresh block anchored here at Impact and severityNit — same as the four sibling sites already flagged. Functional but narrow: only affects the one-time npm→bun migration path, and only for a subsequent FixAfter the libc: if let Some(libc_array) = pkg.get(b"libc") {
'libc: {
let mut libc = Npm::Libc::NONE.negatable();
let Some(arr) = libc_array.as_array() else {
break 'libc Npm::Libc::NONE;
};
for item in arr {
let Some(s) = item.as_string() else { continue };
libc.apply(s);
}
libc.combine()
}
} else {
Npm::Libc::NONE
},(Exact shape should match the neighboring
Collaborator
Author
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. |
||
| } | ||
| } | ||
| _ => {} | ||
|
|
||
| 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; | ||
|
|
||
|
|
@@ -14,7 +14,9 @@ use crate::lockfile_real::StringBuilder as LockfileStringBuilder; | |
| #[derive(Clone, Copy)] | ||
| pub struct Meta { | ||
| pub(crate) origin: Origin, | ||
| pub(crate) _padding_origin: u8, | ||
| /// npm `libc` field (glibc/musl). Occupies what used to be a padding byte, so | ||
| /// binary lockfiles written before it read back as `Libc::NONE` (unspecified). | ||
| pub(crate) libc: Libc, | ||
|
Comment on lines
+17
to
+19
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. 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Description: Locate the binary lockfile (de)serializer for Meta and any old-format compatibility fixtures.
set -euo pipefail
echo "--- Meta struct and padding fields ---"
rg -n -C5 '_padding_os|_padding_integrity|struct Meta' src/install/lockfile/Package/Meta.rs
echo "--- Candidate serializer files ---"
fd -e rs . src/install/lockfile | xargs rg -l 'Meta' | sort -u
echo "--- Binary lockfile fixtures ---"
fd -HI 'bun.lockb'Repository: oven-sh/bun Length of output: 2525 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "--- Meta definition and methods ---"
cat -n src/install/lockfile/Package/Meta.rs | sed -n '1,180p'
echo "--- Binary lockfile serializer/deserializer references ---"
rg -n -C8 'Package::Meta|Meta::|struct Meta|impl.*Meta|read.*Meta|write.*Meta|serialize|deserialize|from_bytes|to_bytes|read_unaligned|size_of|transmute' \
src/install/lockfile/Package.rs \
src/install/lockfile/bun.lock.rs \
src/install/lockfile/bun.lockb.rs \
src/install/lockfile/Package
echo "--- Compatibility tests and fixture usage ---"
rg -n -C8 'bun\.lockb\.v2|bun\.lockb|lockb|libc|Libc::NONE|NONE' \
test/cli/install/bun-lockb.test.ts \
test/cli/install/migrate-bun-lockb-v2.test.ts \
test/cli/install/fixtures \
src/install/lockfileRepository: oven-sh/bun Length of output: 50368 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "--- PackageField order and Meta column serialization ---"
sed -n '354,410p' src/install/lockfile/Package.rs
sed -n '3080,3185p' src/install/lockfile/Package.rs
sed -n '3280,3420p' src/install/lockfile/Package.rs
echo "--- Binary loader version and package migration paths ---"
sed -n '390,470p' src/install/lockfile/bun.lockb.rs
rg -n -C6 'migrate_from_v2|Version::V2|v2|Meta' src/install/lockfile/Package.rs src/install/lockfile/bun.lockb.rs
echo "--- Relevant type layouts ---"
rg -n -C5 'enum (Architecture|OperatingSystem|Libc)|struct (Architecture|OperatingSystem|Libc)' src node_modules 2>/dev/null | head -n 240
echo "--- Tests that inspect parsed package metadata ---"
rg -n -C5 'parseLockfile|libc|cpu|os|architecture|platform' test/cli/install test/integration | head -n 300Repository: oven-sh/bun Length of output: 48319 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "--- Current diff and history for Meta ---"
git diff --stat -- src/install/lockfile/Package/Meta.rs
git log --all --oneline -S'pub(crate) libc' -- src/install/lockfile/Package/Meta.rs | head -n 20
git log --all --oneline -S'_padding_os' -- src/install/lockfile/Package/Meta.rs | head -n 20
echo "--- Old and current Meta definitions from repository history ---"
commit="$(git log --all --format='%H' -S'pub(crate) libc' -- src/install/lockfile/Package/Meta.rs | head -n 1 || true)"
if [ -n "$commit" ]; then
echo "libc-introducing commit: $commit"
git show --format= --find-renames "$commit" -- src/install/lockfile/Package/Meta.rs | sed -n '1,180p'
parent="$(git rev-parse "$commit^")"
echo "parent: $parent"
git show "$parent:src/install/lockfile/Package/Meta.rs" | sed -n '1,80p'
fi
echo "--- parseLockfile helper and snapshot metadata ---"
rg -n -C10 'function parseLockfile|parseLockfile\s*=|parseLockfile\(' test src | head -n 240
rg -n -C4 'libc|Libc' test/cli/install/__snapshots__ test/cli/install/fixtures 2>/dev/null | head -n 160
echo "--- Compatibility fixture file types and sizes ---"
file test/cli/install/fixtures/bun.lockb.v2 test/cli/install/fixtures/bun.lockb.v2-most-features
wc -c test/cli/install/fixtures/bun.lockb.v2 test/cli/install/fixtures/bun.lockb.v2-most-featuresRepository: oven-sh/bun Length of output: 27505 🏁 Script executed: #!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
import subprocess
path = "src/install/lockfile/Package/Meta.rs"
current = open(path, encoding="utf-8").read()
commit = "e649df82ca4fa1304bf58bcf97f8f2dca1e0e332"
parent = subprocess.check_output(
["git", "show", f"{commit}^:{path}"], text=True
)
def meta_fields(text):
body = text.split("pub struct Meta {", 1)[1].split("}", 1)[0]
return [
(name, typ)
for name, typ in re.findall(
r"pub\(crate\)\s+(\w+):\s*([^,]+),", body
)
]
old = meta_fields(parent)
new = meta_fields(current)
print("old Meta fields:", old)
print("new Meta fields:", new)
print("same field count:", len(old) == len(new))
print("same field types:", [typ for _, typ in old] == [typ for _, typ in new])
print("second field changed from padding to libc:", old[1] == ("_padding_origin", "u8") and new[1] == ("libc", "Libc"))
serializer = open("src/install/lockfile/Package.rs", encoding="utf-8").read()
loader = serializer.split("fn load_fields", 1)[1].split("pub use serializer", 1)[0]
print("Meta loader uses current Meta field size:", "field_size" in loader and "size_of::<Meta>()" in loader)
print("Meta loader copies bytes into the current column:", "bytes.copy_from_slice(src)" in loader)
PY
echo "--- Migration test assertions ---"
sed -n '81,110p' test/cli/install/migrate-bun-lockb-v2.test.ts
echo "--- Migration snapshot names ---"
rg -n 'migrate-bun-lockb-v2|libc' test/cli/install/__snapshots__/migrate-bun-lockb-v2.test.ts.snap || trueRepository: oven-sh/bun Length of output: 2247 Add an explicit The migration test parses only the rewritten lockfile and does not assert the value loaded from the old fixture. 🤖 Prompt for AI Agents
Collaborator
Author
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. The old-format fixture loads with the padding byte zeroed, which is
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.
The explicit assertion is not required for correctness today. It would protect the old-format compatibility contract if 🐇 You are interacting with an AI system. |
||
|
|
||
| pub(crate) arch: Architecture, | ||
| pub(crate) os: OperatingSystem, | ||
|
|
@@ -49,7 +51,7 @@ impl Default for Meta { | |
| fn default() -> Self { | ||
| Self { | ||
| origin: Origin::Npm, | ||
| _padding_origin: 0, | ||
| libc: Libc::NONE, | ||
| arch: Architecture::ALL, | ||
| os: OperatingSystem::ALL, | ||
| _padding_os: 0, | ||
|
|
@@ -63,10 +65,23 @@ impl Default for Meta { | |
| } | ||
|
|
||
| impl Meta { | ||
| /// Does the `cpu` arch and `os` match the requirements listed in the package? | ||
| /// Do the `cpu` arch, `os` and (on Linux) `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) | ||
| !self.arch.is_match(cpu) || !self.os.is_match(os) || !self.libc_matches_host() | ||
| } | ||
|
Comment on lines
70
to
+72
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. 🟡 The Extended reasoning...What the bug is
if pkg_metas[pkg_id as usize].is_disabled(manager.options.cpu, manager.options.os) {
if manager.options.log_level.is_verbose() {
let meta = &pkg_metas[pkg_id as usize];
...
if !meta.os.is_match(...) && !meta.arch.is_match(...) {
// "cpu & os mismatch"
} else if !meta.os.is_match(...) {
// "os mismatch"
} else if !meta.arch.is_match(...) {
// "cpu mismatch"
}
// no else — libc-only falls through silently
}
return true;
}There is no final Code path that triggers it
Why existing code doesn't prevent itBefore this PR, ImpactDiagnostic-only. The package is correctly skipped; the only effect is that Step-by-step proof
FixAdd a libc arm (and/or a fall-through } else if !meta.libc.is_match(manager.options.libc) {
bun_core::pretty_errorln!(
"<d>Skip installing<r> <b>{}<r> <d>- libc mismatch<r>",
bstr::BStr::new(name)
);
}Optionally also mention libc in the combined-mismatch messages when it applies.
Collaborator
Author
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. |
||
|
|
||
| /// npm's `libc` constraint: only consulted for packages that target Linux, against | ||
| /// the host libc (or the `--libc` override). | ||
| #[inline] | ||
| fn libc_matches_host(&self) -> bool { | ||
| if self.libc == Libc::NONE { | ||
| return true; | ||
| } | ||
| let target = crate::package_manager_real::PackageManager::try_get() | ||
| .map(|pm| pm.options.libc) | ||
| .unwrap_or(Libc::CURRENT); | ||
| self.libc.is_match(target) | ||
| } | ||
|
Comment on lines
+77
to
85
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
pub fn is_disabled(&self, cpu: Architecture, os: OperatingSystem) -> bool {
!self.arch.is_match(cpu) || !self.os.is_match(os) || !self.libc_matches_host()
}
fn libc_matches_host(&self) -> bool {
if self.libc == Libc::NONE { return true; }
let target = crate::package_manager_real::PackageManager::try_get()
.map(|pm| pm.options.libc)
.unwrap_or(Libc::CURRENT);
self.libc.is_match(target)
}This is asymmetric: CallersI traced every caller of
Every one of them already has the Why it matters (and why it's only a nit)No wrong behavior today. In every current call path, the But it's inconsistent with the file's own convention (REVIEW.md: "Match the exact file's local conventions", "Use the simplest mechanism the invariants allow"). The existing design deliberately made The PR description says Step-by-step (hypothetical divergence)
Today no constructor sets Suggested fixAdd
Collaborator
Author
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.
Comment on lines
+68
to
85
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. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Description: Find all callers of is_disabled / is_resolved_dependency_disabled to check whether options.libc is already reachable there.
set -euo pipefail
rg -n -C4 '\bis_disabled\s*\(|\bis_resolved_dependency_disabled\s*\(' src/installRepository: oven-sh/bun Length of output: 5813 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Meta and call sites ---'
sed -n '1,105p' src/install/lockfile/Package/Meta.rs
sed -n '700,740p' src/install/lockfile.rs
sed -n '60,85p' src/install/PackageManager/PackageManagerLifecycle.rs
sed -n '175,195p' src/install/lockfile/reachable.rs
sed -n '565,590p' src/install/lockfile/Tree.rs
sed -n '238,260p' src/install/lockfile/printer/tree_printer.rs
printf '%s\n' '--- libc option definitions and uses ---'
rg -n -C3 '\blibc\b' src/install src | head -n 300Repository: oven-sh/bun Length of output: 29763 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- reachable.rs context ---'
sed -n '1,80p' src/install/lockfile/reachable.rs
sed -n '150,205p' src/install/lockfile/reachable.rs
printf '%s\n' '--- all relevant call sites ---'
rg -n -C8 '\.(is_disabled|is_resolved_dependency_disabled)\s*\(' src/install
printf '%s\n' '--- PackageManager singleton definition and try_get ---'
rg -n -C8 'fn try_get|struct PackageManager|static .*PackageManager|package_manager_real' src/install src | head -n 240Repository: oven-sh/bun Length of output: 30601 🏁 Script executed: #!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
root = Path("src/install")
files = [
root / "PackageManager/PackageManagerLifecycle.rs",
root / "lockfile.rs",
root / "lockfile/Package.rs",
root / "lockfile/Tree.rs",
root / "lockfile/printer/tree_printer.rs",
root / "lockfile/reachable.rs",
]
call_re = re.compile(r"\.(is_disabled|is_resolved_dependency_disabled)\s*\(")
fn_re = re.compile(r"\b(?:pub(?:\([^)]*\))?\s+)?fn\s+([A-Za-z0-9_]+)\s*\(")
for path in files:
lines = path.read_text().splitlines()
print(f"{path}:")
current_fn = None
depth = 0
for number, line in enumerate(lines, 1):
match = fn_re.search(line)
if match:
current_fn = match.group(1)
if call_re.search(line):
start = max(0, number - 3)
end = min(len(lines), number + 5)
context = "\n".join(lines[start:end])
print(f" line {number}: {current_fn}\n{context}\n")
reachable = (root / "lockfile/reachable.rs").read_text()
options = re.search(r"pub struct Options\s*\{(.*?)\n\}", reachable, re.S)
assert options, "reachable::Options was not found"
fields = options.group(1)
print("reachable::Options contains libc:", bool(re.search(r"\blibc\b", fields)))
meta = (root / "lockfile/Package/Meta.rs").read_text()
assert "PackageManager::try_get()" in meta
assert "self.libc_matches_host()" in meta
print("Meta::is_disabled performs a PackageManager::try_get lookup:", True)
PYRepository: oven-sh/bun Length of output: 2665 Thread
🤖 Prompt for AI Agents
Collaborator
Author
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.
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.
🐇 ✏️ Learnings added
You are interacting with an AI system. |
||
|
|
||
| pub(crate) fn has_install_script(&self) -> bool { | ||
|
|
@@ -110,6 +125,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 |
|---|---|---|
|
|
@@ -786,7 +786,7 @@ impl Stringifier { | |
| }); | ||
| } | ||
|
|
||
| // INFO = { prod/dev/optional/peer dependencies, os, cpu, libc (TODO), bin, binDir } | ||
| // INFO = { prod/dev/optional/peer dependencies, os, cpu, libc, bin, binDir } | ||
|
|
||
| // first index is resolution for each type of package | ||
| // npm -> [ "name@version", registry (TODO: remove if default), INFO, integrity] | ||
|
|
@@ -1063,7 +1063,7 @@ impl Stringifier { | |
| Ok(()) | ||
| } | ||
|
|
||
| /// Writes a single line object. Contains dependencies, os, cpu, libc (soon), and bin | ||
| /// Writes a single line object. Contains dependencies, os, cpu, libc, and bin | ||
| /// { "devDependencies": { "one": "1.1.1", "two": "2.2.2" }, "os": "none" } | ||
| fn write_package_info_object( | ||
| writer: &mut Writer, | ||
|
|
@@ -1178,14 +1178,15 @@ impl Stringifier { | |
| writer.write_all(b" \"bundled\": true")?; | ||
| } | ||
|
|
||
| // TODO(dylan-conway) | ||
| // if (meta.libc != .all) { | ||
| // try writer.writeAll( | ||
| // \\"libc": [ | ||
| // ); | ||
| // try Negatable(Npm.Libc).toJson(meta.libc, writer); | ||
| // try writer.writeAll("], "); | ||
| // } | ||
| if meta.libc != Npm::Libc::NONE && meta.libc != Npm::Libc::ALL { | ||
| if any { | ||
| writer.write_byte(b',')?; | ||
| } else { | ||
| any = true; | ||
| } | ||
| writer.write_all(b" \"libc\": ")?; | ||
| Negatable::<Npm::Libc>::to_json(meta.libc, &mut AsFmt::new(writer))?; | ||
| } | ||
|
|
||
| if meta.os != Npm::OperatingSystem::ALL { | ||
| if any { | ||
|
|
@@ -2844,10 +2845,9 @@ pub(crate) fn parse_into_binary_lockfile( | |
| pkg.meta.arch = | ||
| Npm::negatable_from_json_value::<Npm::Architecture>(arch); | ||
| } | ||
| // TODO(dylan-conway) | ||
| // if (os_cpu_libc_obj.get("libc")) |libc| { | ||
| // pkg.meta.libc = Negatable(Npm.Libc).fromJson(allocator, libc); | ||
| // } | ||
| if let Some(libc) = deps_os_cpu_libc_bin_bundle_obj.get(b"libc") { | ||
| pkg.meta.libc = Npm::negatable_from_json_value::<Npm::Libc>(libc); | ||
| } | ||
|
Comment on lines
+2848
to
+2850
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. 🟡 Four more Extended reasoning...What this isThe PR wires Site 1 —
|
||
| } | ||
| } | ||
| ResolutionTag::Root => { | ||
|
|
||
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.
🟡
PRUNE_HELP_PARAMSlists--osand--cpu("Prune for a different … than the current one") but wasn't given a matching--libcentry. The flag works forbun prune(it's inSHARED_TAIL_PARAMS→PRUNE_PARAMS) and is functional sinceis_disabled()now consults libc — only the curatedbun prune --helpoutput is missing it.Extended reasoning...
What the bug is
bun prune --helpprints a curated flag table (PRUNE_HELP_PARAMS) rather than the fullSHARED_PARAMSlist, so that only flags relevant to pruning are shown. That table explicitly surfaces the two platform-override flags:This PR adds
--libctoSHARED_TAIL_PARAMS(sobun pruneaccepts and honours it —PRUNE_PARAMSincludesSHARED_PARAMSwhich includesSHARED_TAIL_PARAMS), andMeta::is_disabled()now consultslibc, which the prune reachable-set walk uses. But the curated help table wasn't updated with a matching--libc <STR>... Prune for a different libc than the current oneentry.Step-by-step
bun prune --help.Subcommand::Prunearm callsclap::simple_help(PRUNE_HELP_PARAMS)(notPRUNE_PARAMS).PRUNE_HELP_PARAMScontains--osand--cpuentries (added specifically because prune's disabled-package check consults them) but no--libc.--os/--cpubut not--libc, even though all three now participate in the sameis_disabled()check that decides what prune removes.Why existing code doesn't prevent it
PRUNE_HELP_PARAMSis a hand-maintained parallel table, distinct from thePRUNE_PARAMStable that clap actually parses against. Adding a flag toSHARED_TAIL_PARAMSmakes it accepted, but doesn't make it appear in the curated help — that requires a separate edit.Why this is in scope
Per REVIEW.md: "Fix the whole class in the same PR — grep for every sibling site sharing the pattern: parallel switch arms…" The
--os/--cpuentries inPRUNE_HELP_PARAMSare exactly the sibling sites of the new--libcflag; the PR updated theSHARED_TAIL_PARAMStriple and the docs (--cpu,--osand--libcflags) but missed this one parallel table.DEDUPE_HELP_PARAMSdoesn't list--os/--cpuso it needs no change.Impact
Cosmetic only — the flag is fully functional for
bun prune; a user reading--helpjust won't discover it there. Not merge-blocking.Fix
Insert one line after the
--cpuentry inPRUNE_HELP_PARAMS: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.
Acknowledged — this is part of completing
libcacross the sibling sites, which #38797 already does (it also fetches the full manifest solibcis populated from registry.npmjs.org). Holding further changes here pending the decision on which of the two PRs proceeds; see #39906 for the comparison.