docs: x64 ships one Nehalem binary; drop Haswell/baseline distinction - #36465
docs: x64 ships one Nehalem binary; drop Haswell/baseline distinction#36465robobun wants to merge 3 commits into
Conversation
The x64 build was collapsed to a single Nehalem binary in #34782, with -baseline release assets and @oven/bun-*-x64-baseline npm packages published as aliases of the same file. SIMD is runtime-dispatched. docs/installation.mdx and docs/bundler/executables.mdx still described the old two-binary world: separate baseline download cards, a CPU Requirements section claiming the standard x64 build requires Haswell/AVX2, and --compile examples saying "modern is faster". All of that is now false. Also fixes the stale README anchor to the CPU requirements section. test/internal/release-asset-coherence.test.ts cross-checks that every zip name install.sh / install.ps1 / packages/bun-release/src/platform.ts can request is produced by .buildkite/scripts/upload-release.sh (primary artifacts + alias_baseline_artifact), and that the docs do not reintroduce the AVX2-required claim.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — docs now accurately describe the post-#34782 single-Nehalem-binary model; the inline nit about the packages/bun-release/npm/** READMEs is worth folding in but doesn't block.
What was reviewed:
- Verified the new test's regexes against the actual
.buildkite/scripts/upload-release.sh(artifacts=(...)= 24 zips,alias_baseline_artifact= 8 aliases) andpackages/bun-release/src/platform.ts(16 bins) — all assertions resolve. - Confirmed the new
#cpu-requirementsanchor matches the rewritten## CPU Requirementsheading. - Checked that dropping
-baseline/-modernfrom the docs'CompileTargetlisting doesn't contradictpackages/bun-types/bun.d.ts, which still accepts them via theSIMDtemplate — the docs comment covers that.
Extended reasoning...
Overview
This PR updates three docs files (docs/installation.mdx, docs/bundler/executables.mdx, root README.md) to reflect that #34782 collapsed x64 to a single Nehalem/SSE4.2 binary with runtime-dispatched AVX2/AVX-512, removing the stale "Haswell required" / "baseline is slower, pick it if you lack AVX2" guidance and the Modern/Baseline target-table columns. It also adds test/internal/release-asset-coherence.test.ts, which parses upload-release.sh and asserts every zip name reachable from install.sh / install.ps1 / platform.ts is produced by the release step, plus a regex guard against the docs re-introducing the old claims. No src/** changes.
Security risks
None. Pure docs prose plus a hermetic test that only readFileSyncs repo files. No auth, crypto, network, or user-input handling touched.
Level of scrutiny
Low-to-moderate. The load-bearing question is factual accuracy of the new prose, which I verified against the actual release script: local artifacts=(...) lists exactly one x64 zip per platform (no separate baseline build), and alias_baseline_artifact re-zips those under -baseline names — matching the PR's "rezipped aliases" description. The new test's regexes were traced against those file contents and all five test cases resolve (24 primary + 8 alias = 32 assets ≥ 24; all 22 install.sh names, 3 install.ps1 names, and 16 platform.ts bins present). The \n {2}\} terminator for the alias_baseline_artifact body correctly stops at the function's 2-space-indented closing brace since the case/esac body contains no braces.
Other factors
- The docs' illustrative
CompileTargetunion drops the-baseline/-modernentries but adds a comment that they're still accepted; the realpackages/bun-types/bun.d.tsstill includes them viatype SIMD = "baseline" | "modern", so no user-facing type regression. - The README anchor change (
#cpu-requirements-and-baseline-builds→#cpu-requirements) matches the new## CPU Requirementsheading slug. - The inline nit about
packages/bun-release/npm/**/README.mdstill saying "without AVX2" is a valid same-class sibling, but those are separately-published npm READMEs and can be a follow-up; they don't affect the correctness of what this PR ships. - No prior reviewer comments to address; timeline is empty.
…lease/npm/** The @oven/bun-*-x64-baseline READMEs and the bun package README on npmjs.com still described baseline as a separate build for CPUs lacking AVX2. They are aliases of the single x64 binary, so say that instead. Extends the coherence test to read these files and broadens the pattern to catch 'without AVX2' / 'do not support AVX2', so the gate's packages/** stash now produces a fail-before.
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
Comment |
There was a problem hiding this comment.
LGTM — the npm README follow-up from the earlier review is addressed in 9f31dd2, and no new issues on this pass.
What was reviewed:
- Re-grepped for stale Haswell/AVX2/"baseline is slower" prose across the repo; remaining hits are the test pattern itself, the intentional backward-compat probe in
install.sh, and internal build tooling — none user-facing. - Checked the new test's regexes against
.buildkite/scripts/upload-release.sh(artifacts=(...)+alias_baseline_artifactbody) andpackages/bun-release/src/platform.ts— all 16bin:entries (incl. android/freebsd/musl-baseline) resolve into the parsed asset set. - Confirmed the
#cpu-requirementsanchor in README.md matches the new## CPU Requirementsheading, and there's no@oven/bun-linux-x64-musl-baselinepackage dir needing a README update.
Extended reasoning...
Overview
Docs-only PR reflecting #34782's collapse of x64 into a single Nehalem binary with runtime-dispatched SIMD. Touches docs/installation.mdx, docs/bundler/executables.mdx, root README.md, four packages/bun-release/npm/**/README.md files, and adds test/internal/release-asset-coherence.test.ts. No src/** changes.
Security risks
None. Pure prose edits plus a test that readFileSyncs repo-local files and matches regexes against them. No network, no external input, no runtime behavior change.
Level of scrutiny
Low. The docs edits remove now-incorrect claims (Haswell required, baseline slower, separate download cards) and replace them with the single-binary model. I verified the underlying premise is already reflected in .buildkite/scripts/upload-release.sh (nehalem comment + alias_baseline_artifact) and the install.sh comment. The new test is static-analysis-style — it parses the artifacts list and asserts every download-path-reachable zip name (install.sh literal targets × -profile, install.ps1 targets, every bin: in platform.ts) is produced. I traced each of the 16 platform.ts bins and the 22 install.sh permutations against the 24 primary + 8 alias assets; all resolve.
Other factors
My prior review flagged the four npm READMEs still carrying "without AVX2" wording and the test not covering them; 9f31dd2 addressed both exactly (READMEs rewritten as alias notes, test file list + regex extended to without AVX2|do not support.{0,20}AVX2), and the thread is resolved. I re-grepped for the whole class: remaining Haswell/AVX2 hits are src/runtime/cli/install.sh (deliberate — pinned older versions may genuinely need -baseline, per its comment), scripts/verify-baseline-static/* (CI static-analysis allowlists, not user docs), and scripts/build/shims/cpu_model/x86.c (compiler-rt CPU-model shim). None are user-facing docs, so correctly out of scope. The @oven package directory listing shows no musl-baseline package needing a README, and the README anchor now matches the renamed ## CPU Requirements heading.
…slower' Addresses self-review: - install.sh targets are now parsed from the literal target=<os-arch> assignments rather than hand-listed, so a new platform case in install.sh without a matching release asset fails the test. - platform bins come from importing the platforms array (same as test/regression/issue/05545.test.ts) instead of regex-scraping the source, so quote style or formatting changes can't silently empty it. - The stale-docs regex gains a 'baseline ... slower' alternation so the old Warning block can't be re-pasted without tripping the guard.
There was a problem hiding this comment.
LGTM — the npm README follow-up in 9f31dd2 and the test hardening in 6171751 fully address the earlier feedback.
What was reviewed:
- Verified the
releaseAssetSet()/installShTargets()regexes match the currentupload-release.sh(artifacts=(...)+ 8alias_baseline_artifactechoes) andinstall.sh(6 base targets;-baselinecorrectly excluded forwindows-*since the AVX2 probe is darwin/linux-only). - Confirmed all 16
platforms[].binentries and every derived install.sh target resolve to an uploaded zip. - Grepped
docs/,packages/, and root README for remaining Haswell / "without AVX2" / "baseline is slower" prose — none left; the only hit is the intentional back-compat probe comment ininstall.sh. #cpu-requirementsanchor matches the new## CPU Requirementsheader; only 3@oven/*-baselineREADME dirs exist and all 3 were updated.
Extended reasoning...
Overview
Docs + internal-coherence-test PR that updates user-facing prose to reflect #34782's single-Nehalem-x64-binary model: docs/installation.mdx (drops the separate baseline download cards and Haswell/AVX2-required CPU table), docs/bundler/executables.mdx (drops -baseline/-modern --compile --target examples, the Modern/Baseline table columns, and the AVX2 warning), root README.md (anchor fix), four packages/bun-release/npm/**/README.md files, and a new test/internal/release-asset-coherence.test.ts that cross-checks upload-release.sh's asset list against every zip name reachable from install.sh, install.ps1, and packages/bun-release/src/platform.ts, plus a regex guard against the stale prose reappearing.
No src/** runtime code is touched.
Security risks
None. Pure documentation prose plus a read-only test that parses in-repo shell scripts and TS source. No auth, crypto, network, or filesystem-mutation surface.
Level of scrutiny
Low-to-moderate. The docs claims themselves needed fact-checking against the release pipeline, which the PR description does thoroughly and which I re-verified: .buildkite/scripts/upload-release.sh ships one x64 zip per platform in artifacts=(...) and rezips 8 -baseline aliases via alias_baseline_artifact; install.sh's AVX2 probe is now commented as a back-compat path for pinned older versions. The new test's regex parsing was the main correctness risk — I traced each pattern against the actual script contents and confirmed the size-floor assertions (>= 24 assets, >= 5 base targets, >= 14 platform bins) and spot-checks (linux-x64-musl-baseline-profile) make the parses non-vacuous.
Other factors
- My previous review's only finding (stale npm READMEs + test not covering them) was addressed in 9f31dd2; the thread is resolved. The subsequent commit 6171751 further strengthened the test by parsing
install.shtargets instead of hardcoding them, importingplatformsdirectly, and addingbaseline.{0,20}slowerto the stale-prose regex. - The one candidate the verifiers examined this run — that the
install.ps1check hardcodes 3 targets rather than parsing the PowerShell — is a reasonable trade-off (PS parsing from JS is brittle for a 3-entry list) and was ruled out. - Grep across
docs/,packages/, and*.mdconfirms no sibling site was missed; the only@oven/*-baselineREADME dirs that exist are the three updated here (there is no musl-baseline README dir). - The
test/internal/placement matches the existing repo pattern for cross-file invariant checks (macos-cross-config.test.ts,parallel-allowlist.test.ts, etc.).
1 participant
What
#34782 collapsed x64 to a single Nehalem (SSE4.2) binary per platform, with SIMD runtime-dispatched via Highway and the vendored libraries' own dispatchers. The
-baselinerelease assets and@oven/bun-*-x64-baselinenpm packages are now rezipped aliases of the same file, kept so olderbun upgradeclients and pinned install scripts keep resolving.The download/install chain is already coherent (verified against the current canary release: all 8
-baselinealias zips present, inner directories renamed byrezip_as,install.sh/install.ps1/upgrade_command.rs/platform.tsall resolve). The docs and npm READMEs, however, still described the pre-#34782 world:docs/installation.mdx: separate "x64 Baseline" download cards labeled "For older CPUs without AVX2"; a CPU Requirements section claiming the standard x64 build "targets the Haswell CPU architecture (AVX and AVX2 instructions required)" with a warning that "baseline builds are slower".docs/bundler/executables.mdx:--compile --targetexamples telling users "modern is faster, but baseline is more compatible", a target table with Modern/Baseline columns, and a<Warning>that x64 "uses SIMD optimizations that require a CPU with AVX2 instructions".packages/bun-release/npm/bun/README.mdand@oven/bun-{darwin,linux,windows}-x64-baseline/README.md(rendered on npmjs.com): "(without AVX2 instructions)" / "Baseline builds are for machines that do not support AVX2".README.md: stale#cpu-requirements-and-baseline-buildsanchor.This rewrites those sections to describe the single-binary model (Nehalem floor, AVX2/AVX-512 at runtime) and notes that
-baseline/-modernremain accepted for backward compatibility but produce the same binary.Test
test/internal/release-asset-coherence.test.tsparses.buildkite/scripts/upload-release.sh(theartifacts=(...)list plusalias_baseline_artifact) and asserts that every zip name reachable frominstall.sh(parsed from its literaltarget=<os-arch>assignments and the-musl/-baseline/-profilesuffix transforms),install.ps1, and theplatformsarray inpackages/bun-release/src/platform.tsis in that set. It also asserts the docs and npm READMEs don't reintroduce the Haswell/"AVX2 required"/"modern is faster"/"baseline is slower" claims.The fail-before-fix check applies via
packages/: withpackages/bun-release/npm/**/README.mdatmain, the stale-docs assertion fails on four "without AVX2" / "do not support AVX2" hits; with this PR, all 5 tests pass. The asset-coherence assertions pass on both, which is the point: they confirm the #34782 release mechanics are already correct and stay that way if someone adds a platform to one file but not the other.#34561 predates #34782 (different
upload-release.shlayout) and does not touch docs; not a duplicate.[stamp-90s] gate passed · iteration 0 · 8 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file