ci: expand native portability checks#9642
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the check_native_portability.sh script to verify native package target coverage for @chainsafe/lodestar-z in addition to scanning prebuilt native binaries for unconditional AVX/AVX2 usage. Feedback suggests failing the check in CI environments if the package is not installed, rather than silently skipping it, to prevent false positives.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Performance Report✔️ no performance regression detected Full benchmark results
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #9642 +/- ##
=========================================
Coverage 52.49% 52.49%
=========================================
Files 848 848
Lines 60485 60485
Branches 4466 4466
=========================================
Hits 31754 31754
Misses 28670 28670
Partials 61 61 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94bc653d6c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| requiredTargets.some((target) => target.aliases.some((alias) => name.endsWith(`-${alias}`))) | ||
| ); | ||
|
|
||
| if (hasTargetPackages || prebuildFiles.length > 0) { |
There was a problem hiding this comment.
For zapi packages such as @chainsafe/lodestar-z, the supported platform matrix is declared as package.json#zapi.targets and the built .node can live under zig-out/lib, so both hasTargetPackages and prebuildFiles.length are false. In that install shape this condition skips the package entirely for target coverage, so a missing ARM64/musl zapi target passes CI—the exact regression this check is meant to catch. Please treat manifest.zapi?.targets as a native package signal and compare it with requiredTargets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@lodekeeper can you check if this is relevant, if so open a PR against mine
There was a problem hiding this comment.
Relevant — verified. A zapi package with no per-target optionalDependencies and no prebuilds/ dir (.node under zig-out/lib) hits hasTargetPackages === false && prebuildFiles.length === 0 and is skipped entirely, so a dropped target passes CI. lodestar-z@0.1.2 is only caught today because it also ships per-target optionalDeps.
Opened #9644 against this branch: adds zapi.targets to native-package detection + validates it against the required matrix (fallback after optionalDeps/prebuilds, so lodestar-z keeps its current behavior). Verified byte-identical output on the real beacon-node/validator graph, and a synthetic pure-zapi fixture missing aarch64-unknown-linux-musl now fails where the base script silently passed (exit 0).
…#9644) Follow-up to #9642 (against `nflaig/check-native-portability`), addressing the Codex bot P2 comment (`r3561870210`). ## Relevant? Yes `check_native_portability.sh` detects native packages via **(a)** per-target `optionalDependencies` naming or **(b)** a `prebuilds/` dir. zapi packages (e.g. `@chainsafe/lodestar-z`) declare their platform matrix as `package.json#zapi.targets` and build the `.node` under `zig-out/lib`. A zapi package with **neither** per-target optionalDependencies **nor** a `prebuilds/` dir is `hasTargetPackages === false && prebuildFiles.length === 0` → **skipped entirely** for target coverage, so a dropped target (e.g. `aarch64-unknown-linux-musl`) passes CI — the exact regression this check exists to catch. `lodestar-z@0.1.2` happens to also ship per-target `optionalDependencies` today, so it is detected via (a) — but that relies on the optionalDependency naming convention rather than the authoritative `zapi.targets` declaration. ## Fix Treat `zapi.targets` as a native-target signal: - add it to native-package **detection** (`hasTargetPackages || prebuildFiles.length > 0 || zapiTargets.length > 0`) so zapi packages are never silently skipped; - **validate** it against the required matrix as a fallback, after optionalDependencies and prebuilds. Precedence is optionalDeps → prebuilds → `zapi.targets`, so packages that ship both (like `lodestar-z`) keep their existing verified behavior; `zapi.targets` only kicks in for the otherwise-skipped shape. ## Verification - **No regression on the real graph:** running the patched script against the installed `beacon-node`/`validator` dependency graph produces byte-identical output to the base branch (exit 0). - **Catches the gap:** a synthetic pure-zapi package (only `zapi.targets`, missing `aarch64-unknown-linux-musl`, `.node` under `zig-out/lib`, no optionalDeps/prebuilds) alongside a complete native package: - base script → **exit 0** (silently passes; package never appears in target coverage) - patched script → **exit 1**, `FAIL: @chainsafe/fake-zapi@1.0.0 / missing zapi target for aarch64-unknown-linux-musl` 🤖 Generated with AI assistance Co-authored-by: lodekeeper <lodekeeper@users.noreply.github.com>
matthewkeil
left a comment
matthewkeil
left a comment
There was a problem hiding this comment.
@lodekeeper I would like to convert this script into a node script, instead of having node code inlined into a bash/shell script and then some mixed in shell commands. Please convert the shell portion of the file over to node format and then get the new node code integrated so its all nice and tidy. Might also need to update the trigger in the CI.
I had my codex that was working on this re-write the script, no idea if it's correct though, especially the |
| const knownMissingTargets = new Set([ | ||
| "@chainsafe/hashtree@1.0.2:x86_64-apple-darwin", | ||
| "@crate-crypto/node-eth-kzg@0.9.1:aarch64-unknown-linux-musl", | ||
| "@crate-crypto/node-eth-kzg@0.9.1:x86_64-unknown-linux-musl", | ||
| "classic-level@1.4.1:aarch64-unknown-linux-musl", | ||
| "classic-level@3.0.0:aarch64-unknown-linux-musl", | ||
| ]); |
There was a problem hiding this comment.
this makes me question if we can even run on arm64 musl right now, at least natively, classic-level seems to build locally, but node-eth-kzg it doesn't seem like it, I wonder if that's relevant, eg. rocketpool users likely run via docker, and we switched from alpine to debian a while ago, not sure when we switched to rust-eth-kzg, was it before fusaka, I would think so
There was a problem hiding this comment.
Sooo, that means we can drop support for musl? If we can, I would prefer to do so. This is one less thing we need to test. (We already aren't testing it in our fleet, so its not like its "well supported" in the first place)
There was a problem hiding this comment.
not sure if we can fix the blind spots in the testing fleet, would probably be a stretch, what I am hoping is at least with this script we have a way to document what platforms we need and want to support, and be bit more deliberate about it, native dependencies breaking user setups was by far the most common reason we had to cut a hotfix, I think we also had an issue with hashtree, or maybe it just fell back to a less performant implementation? arguably that is even worse. but regarding the question, I don't see a setup we would break right now not shipping arm64 musl, most seem debian or docker based
There was a problem hiding this comment.
Native musl is already effectively unsupported for a beacon node today, so dropping it is mostly formalizing reality.
Per-target, from the current production graph:
- x86_64-musl:
blst✓,hashtree✓,classic-level✓ — but@crate-crypto/node-eth-kzg✗ (no musl prebuild). - aarch64-musl:
blst✓,hashtree✓ — butclassic-level✗ andnode-eth-kzg✗.
node-eth-kzg (our rust-eth-kzg, required for blobs/PeerDAS) ships zero musl builds — still true on latest 0.10.0, not just the pinned 0.9.1, so it's an upstream gap we can't just bump our way out of. Without a musl KZG binary there's no npm install-able native beacon node on Alpine (short of building the Rust crate from source, which we don't wire up), so there's no working setup to lose.
Given that + the official image is node:24-slim (Debian/glibc since #7004, Aug 2024, so docker/RocketPool users are glibc) + it's not in the fleet — agreed, dropping it is reasonable. In this script that's just removing the two *-linux-musl entries from requiredTargets (and the now-moot musl knownMissingTargets). Can push that once you and Nico settle on it.
There was a problem hiding this comment.
+1 — documenting the matrix deliberately is the point; native-dep breakage being the top hotfix cause is exactly why this check earns its keep.
On hashtree: it's not a musl gap — @chainsafe/hashtree@1.0.2 ships both linux-x64-musl and linux-arm64-musl; its only missing target is macOS-Intel (x86_64-apple-darwin), which the script flags. But the "fell back to a less performant impl, which is worse" instinct is right and real: if the hashtree native binary isn't loaded (missing prebuild, or the module gets duplicated in the tree), persistent-merkle-tree silently drops to the JS hasher — much slower, and it's caused GC regressions before (we added a CI dedup-lint after one such incident). Silent degradation is worse than a hard failure, so a gate like this is the right call.
One clarification on the runtime idea: this script is a build/CI-time gate — it inspects the installed graph in CI, it never runs on a user's node, so it won't warn an Alpine user whose binary "works" until it hits a missing native dep. Catching that needs a small separate startup check — detect musl at runtime (absence of glibcVersionRuntime in process.report, or detect-libc) and warn/refuse to start. Happy to draft that as a follow-up, plus add the supported-platform matrix to the install docs so musl users aren't surprised in the first place.
| errors.push({target: target.name, message: `missing bundled prebuild for ${target.name}`}); | ||
| } | ||
|
|
||
| // zapi.targets is the build matrix, while optionalDependencies describes |
There was a problem hiding this comment.
seems sketchy that zapi requires this special handling
5 participants
What changed
@chainsafe/lodestar-zdeclares every native target supported by LodestarWhy
The existing check only inspected installed x64 binaries, so it could not detect a missing prebuilt target such as ARM64 musl/Alpine.
Validation
bash -n scripts/ci/check_native_portability.shscripts/ci/check_native_portability.sh(correctly reports the currently installedlodestar-zpackage is missingaarch64-unknown-linux-musl; existing CPU-dispatch checks pass)