Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions .devcontainer/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,12 @@ RUN apt-get update \
&& apt-get clean -y \
&& rm -rf /var/lib/apt/lists/*

# node 22 to match CI: pnpm 11 requires >=22.13, and vitest 3 (via vite 7)
# requires >=20.19, so the old node 16 here could run neither.
RUN curl -fsSL https://deb.nodesource.com/setup_22.x | bash -
# node 24 to match CI, and to satisfy the root package.json engines.node
# floor (>=24) — pnpm warns "Unsupported engine" on anything below it. Only
# the node major is shared with CI: the emsdk pin above is deliberately older
# than the one tools/docker/Dockerfile and the build job use, so this image is
# for editing and running the JS tooling, not for reproducing a CI dist.
RUN curl -fsSL https://deb.nodesource.com/setup_24.x | bash -
RUN apt-get install -y nodejs
# Corepack reads the pinned pnpm version from the repo's package.json
# "packageManager" field on first use, so there is nothing to pin here.
Expand Down
16 changes: 9 additions & 7 deletions .github/workflows/bench.yml
Original file line number Diff line number Diff line change
Expand Up @@ -242,18 +242,18 @@ jobs:
persist-credentials: false
- uses: actions/setup-node@v4
with:
# EXACT version, not the '22' range every other job uses. For a range,
# EXACT version, not the '24' range every other job uses. For a range,
# setup-node takes any satisfying version already in the runner's tool
# cache without checking the network — and on a self-hosted box that
# cache persists, so the bench would silently freeze on whichever 22.x
# cache persists, so the bench would silently freeze on whichever 24.x
# landed there first and jump whenever the box is rebuilt. A V8 patch
# bump shifts instruction counts the same way a different CPU does.
# 22.23.1 is what the current main baseline was measured on; changing
# 24.20.0 is what the current main baseline was measured on; changing
# it is a deliberate re-seed event (see docs/ci/self-hosted-runner.md).
node-version: '22.23.1'
node-version: '24.20.0'
# nashua has no package manager beyond npm: setup-node ships node + npm
# only, and GitHub's hosted images are not what runs here. Corepack is
# bundled with node 22 and fetches over Node's own https, so it works
# still bundled with node 24 and fetches over Node's own https, so it works
# where `npm i -g <pm>` is unreliable on this box — the runner's bundled
# node has a corrupted npm ("Cannot find module '../lib/cli.js'"), which
# is why OHIF's workflow also went the Corepack route here. `corepack
Expand Down Expand Up @@ -296,8 +296,10 @@ jobs:
# pnpm-workspace.yaml is in the key because the lockfile does not
# record nodeLinker/allowBuilds/linkWorkspacePackages — without it a
# layout change hits the cache and the install step is skipped.
# Keep in step with the cache keys in pr-checks.yml.
key: pnpm-modules-node22-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('pnpm-lock.yaml', 'pnpm-workspace.yaml') }}
# Keep in step with the cache keys in pr-checks.yml — including the
# node major in the prefix, which is the only part of this key a
# node-only bump changes (see the build job's cache step there).
key: pnpm-modules-node24-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('pnpm-lock.yaml', 'pnpm-workspace.yaml') }}
- name: Install dependencies
if: steps.modules-cache.outputs.cache-hit != 'true'
run: pnpm install --frozen-lockfile
Expand Down
37 changes: 26 additions & 11 deletions .github/workflows/pr-checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -196,12 +196,13 @@ jobs:
# Shallow checkout: builds only need the working tree (full history
# is only required by detect-changes for the merge-base diff).
- uses: actions/setup-node@v4
# The emsdk image bundles node 20.18.0, which fails both pnpm 11's
# engine check (needs >=22.13) and vite 7's (via vitest 3). Put node 22
# on PATH for pnpm/webpack; emscripten is unaffected — emcc invokes the
# node binary pinned in its own .emscripten config, not the one on PATH.
# The emsdk image bundles node 20.18.0 — end of life since 2026-04-30,
# and it fails this repo's engines.node check (needs >=24) anyway. Put node
# 24 on PATH for pnpm/webpack; emscripten is unaffected — emcc invokes
# the node binary pinned in its own .emscripten config, not the one on
# PATH.
with:
node-version: '22'
node-version: '24'
- name: Provide pnpm via Corepack
# `corepack prepare --activate` with no argument installs exactly the
# version in the root package.json "packageManager" field, so CI and
Expand Down Expand Up @@ -244,6 +245,14 @@ jobs:
# stale cache entry. The root manifest also carries the
# packageManager pin, so a pnpm bump invalidates too.
# Keep the same hashFiles(...) inputs in all four pnpm-modules keys.
#
# This job's prefix is `build` rather than `node<major>` because it
# installs inside the emsdk container; the other three install on
# ubuntu-latest under setup-node and carry the node major in the key
# so a node bump forces a fresh install instead of restoring a tree
# built against the previous V8/ABI. BUMP THAT DISCRIMINATOR WITH THE
# node-version PINS — bench.yml's key hashes only the lockfile and
# workspace config, so nothing else in a node-only bump invalidates it.
key: pnpm-modules-build-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('package.json', 'packages/*/package.json', 'pnpm-lock.yaml', 'pnpm-workspace.yaml') }}
- name: Install dependencies
if: steps.modules-cache.outputs.cache-hit != 'true'
Expand Down Expand Up @@ -282,7 +291,7 @@ jobs:
persist-credentials: false
- uses: actions/setup-node@v4
with:
node-version: '22'
node-version: '24'
- name: Provide pnpm via Corepack
run: |
corepack enable pnpm
Expand Down Expand Up @@ -316,7 +325,7 @@ jobs:
packages/*/node_modules
# Manifests + workspace config in the key — see the build job's cache
# step for why the lockfile alone is not enough.
key: pnpm-modules-node22-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('package.json', 'packages/*/package.json', 'pnpm-lock.yaml', 'pnpm-workspace.yaml') }}
key: pnpm-modules-node24-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('package.json', 'packages/*/package.json', 'pnpm-lock.yaml', 'pnpm-workspace.yaml') }}
- name: Install dependencies
if: steps.modules-cache.outputs.cache-hit != 'true'
run: pnpm install --frozen-lockfile
Expand Down Expand Up @@ -347,6 +356,12 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-node@v4
# check.js needs no dependencies (fs/path/zlib only), so this is purely
# to stop the gate running on whatever node the runner image happens to
# ship — which drifts, and has been below this repo's engines.node floor.
with:
node-version: '24'
- name: Download all built dists
uses: actions/download-artifact@v4
with:
Expand All @@ -370,7 +385,7 @@ jobs:
- uses: actions/checkout@v4
- uses: actions/setup-node@v4
with:
node-version: '22'
node-version: '24'
- name: Provide pnpm via Corepack
run: |
corepack enable pnpm
Expand Down Expand Up @@ -400,7 +415,7 @@ jobs:
packages/*/node_modules
# Manifests + workspace config in the key — see the build job's cache
# step for why the lockfile alone is not enough.
key: pnpm-modules-node22-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('package.json', 'packages/*/package.json', 'pnpm-lock.yaml', 'pnpm-workspace.yaml') }}
key: pnpm-modules-node24-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('package.json', 'packages/*/package.json', 'pnpm-lock.yaml', 'pnpm-workspace.yaml') }}
- name: Install dependencies
if: steps.modules-cache.outputs.cache-hit != 'true'
run: pnpm install --frozen-lockfile
Expand Down Expand Up @@ -450,7 +465,7 @@ jobs:
# between patch releases move the numbers, and walltime is if anything
# more sensitive than instruction counts. Keep this in step with the
# codspeed-bench pin so the two instruments stay comparable.
node-version: '22.23.1'
node-version: '24.20.0'
- name: Provide pnpm via Corepack
run: |
corepack enable pnpm
Expand Down Expand Up @@ -483,7 +498,7 @@ jobs:
# (esbuild/rollup) and break vitest.
# Manifests + workspace config in the key — see the build job's cache
# step for why the lockfile alone is not enough.
key: pnpm-modules-node22-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('package.json', 'packages/*/package.json', 'pnpm-lock.yaml', 'pnpm-workspace.yaml') }}
key: pnpm-modules-node24-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('package.json', 'packages/*/package.json', 'pnpm-lock.yaml', 'pnpm-workspace.yaml') }}
- name: Install dependencies
if: steps.modules-cache.outputs.cache-hit != 'true'
run: pnpm install --frozen-lockfile
Expand Down
17 changes: 9 additions & 8 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,12 @@ jobs:
# .git/config cannot push or publish, and the submodule init a few steps
# down is the one place in this file that still wants a working remote.
- uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0
# The emsdk image bundles node 20.18.0; pnpm 11 requires >=22.13 and
# vite 7 (via vitest 3) requires >=20.19. emcc is unaffected — it uses
# the node binary pinned in its own .emscripten config, not PATH.
# The emsdk image bundles node 20.18.0, which reached end of life on
# 2026-04-30 and fails this repo's engines.node check (>=24) besides. emcc is
# unaffected — it uses the node binary pinned in its own .emscripten
# config, not PATH, so this only governs pnpm and the build scripts.
with:
node-version: '22'
node-version: '24'
- name: Provide pnpm via Corepack
run: |
corepack enable pnpm
Expand Down Expand Up @@ -173,12 +174,12 @@ jobs:
- uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0
with:
# Pinned exactly, not floating '24'. Node bundles npm, and this is
# the version that ends up publishing: v24.19.0 ships npm 11.17.0,
# the version that ends up publishing: v24.20.0 ships npm 11.19.0,
# comfortably past the 11.5.1 that OIDC trusted publishing needs. So
# bumping Node here is also how npm gets bumped — no separate
# `npm install --global` step, which would re-download an unpinned
# npm on the morning of every release.
node-version: '24.19.0'
node-version: '24.20.0'
- name: Provide pnpm via Corepack
run: |
corepack enable pnpm
Expand Down Expand Up @@ -426,8 +427,8 @@ jobs:
- uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0
with:
# Same exact pin as the release job: this is where its bundled npm
# (11.17.0, past the 11.5.1 OIDC floor) actually gets used.
node-version: '24.19.0'
# (11.19.0, past the 11.5.1 OIDC floor) actually gets used.
node-version: '24.20.0'
- name: Download built dists
uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0
with:
Expand Down
16 changes: 9 additions & 7 deletions docs/ci/self-hosted-runner.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ and the regression gate is trustworthy.
- **flock** (from `util-linux`, present on any stock Linux) — `tools/ci/with-nashua-lock.sh`
needs it for the shared-box mutex below.
- Node is provisioned per-job by `actions/setup-node@v4`, pinned to an **exact**
version (`22.23.1`) rather than the `'22'` range the other jobs use — see
version (`24.20.0`) rather than the `'24'` range the other jobs use — see
[Hardware hygiene](#hardware-hygiene-for-stable-numbers) for why. It does not
need to be pre-installed; the runner user only needs write access to the actions
tool cache. **The box provides no node on `PATH` of its own**, so anything a
Expand All @@ -52,8 +52,8 @@ and the regression gate is trustworthy.
```
With no argument, `corepack prepare --activate` installs exactly the version
in the root `package.json`'s `packageManager` field, so the bench box can
never drift from the build jobs. Corepack is bundled with node 22 and fetches
over Node's own https. That is deliberate rather than incidental: `npm i -g`
never drift from the build jobs. Corepack is still bundled with node 24 and
fetches over Node's own https. That is deliberate rather than incidental: `npm i -g`
is unreliable here because the npm reachable from the GitHub runner's
*bundled* node on this box is corrupted (`Cannot find module '../lib/cli.js'`)
— the same wall OHIF's workflow hit, which is why it provisions pnpm through
Expand Down Expand Up @@ -184,14 +184,16 @@ This runner hosts the **simulation** gate only, which changes what matters:
- The only hard requirement for cross-run stability is a **fixed CPU model**
(don't migrate the box between different physical CPUs), since the modeled
cache is derived from it.
- **Pin node exactly, never by range.** Given a range like `'22'`, setup-node uses
- **Pin node exactly, never by range.** Given a range like `'24'`, setup-node uses
any satisfying version already in the tool cache *without consulting the
network*. On a self-hosted box that cache persists, so the bench silently
freezes on the first 22.x it ever saw and then jumps whenever the box is
freezes on the first 24.x it ever saw and then jumps whenever the box is
rebuilt or the cache is cleared — and V8 changes between patch releases move
instruction counts. Both codspeed jobs pin `22.23.1`, the version the current
instruction counts. Both codspeed jobs pin `24.20.0`, the version the current
baseline was measured on. Changing it is a deliberate re-seed event, exactly
like a glibc or valgrind bump.
like a glibc or valgrind bump. The move from `22.23.1` to `24.20.0` was one
such event: node 24 is a different V8 major, so expect the first `main` run
after it to show large deltas across every bench and mean nothing by them.
- **Don't casually `apt upgrade` the box.** glibc is the sharpest example:
different glibc builds dispatch different code paths, so a bump shifts
instruction counts much as a different CPU would — that is the subject of
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
"playwright-core": "^1.49.0"
},
"engines": {
"node": ">=22.13",
"node": ">=24",
"pnpm": ">=11"
},
"scripts": {
Expand Down
4 changes: 2 additions & 2 deletions tools/docker/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ RUN apt-get update \
# what makes that fail now, with a readable message, instead of during a
# codec build later.
&& cmake --version \
# node 22 on PATH, matching CI's setup-node step. emcc is unaffected: it
# node 24 on PATH, matching CI's setup-node step. emcc is unaffected: it
# invokes the node pinned in its own .emscripten config, not this one.
&& curl -fsSL https://deb.nodesource.com/setup_22.x | bash - \
&& curl -fsSL https://deb.nodesource.com/setup_24.x | bash - \

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 42 in .devcontainer\Dockerfile still installs node 22. Deliberate, since this image intentionally tracks an older emsdk (3.1.53) and isn't the CI environment?

If so, worth a line in "Deliberately unchanged" alongside the consumer engines note.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — half deliberate, and the half that wasn't was worse than a missing doc line.

The emsdk pin in .devcontainer/Dockerfile is deliberate, and that's now written up under "Deliberately unchanged". The node there wasn't: it only ever followed CI, and the comment above it said so in as many words — "node 22 to match CI: pnpm 11 requires >=22.13". This PR falsified both clauses in the same commit, and since engines.node moved to >=24, pnpm had started warning Unsupported engine on every install in that container. So I bumped it to setup_24.x (1f5fc52) rather than documenting it as intentional, and rewrote the comment to separate what's shared with CI (the node major) from what deliberately isn't (the emsdk pin), so it can't go stale the same way again.

Chasing that turned up two more things still on 22, both in the same commit:

The node_modules cache keys all carried a literal node22 discriminator. In pr-checks.yml that was cosmetic — those keys hash package.json, which the engines bump edits. But bench.yml's key hashes only pnpm-lock.yaml and pnpm-workspace.yaml, neither of which a node-only bump touches. It would have hit the entry main's node-22 runs populated, and Install dependencies is gated on cache-hit != 'true' — so pnpm install would never have run on node 24, and the re-seed this PR is warning about would have been measured against a dependency tree installed by the previous V8 major. The bench run on be9a920 is exactly that, so it's worth discounting; 1f5fc52 is the first honest measurement. The reason for the discriminator is now recorded next to the build job's key so the next bump carries it.

dist-size had no setup-node step at all, contrary to what my table claimed — the gate ran on whichever node the runner image shipped. Harmless in practice (check.js is fs/path/zlib only) but unpinned, and possibly under the new floor. Pinned '24' like everything else, and the table now says what that row actually was.

Also fixed two setup-node comments that justified the step with "fails pnpm 11's engine check (>=22.13)" — that floor is this repo's engines.node, not pnpm's, and it's now >=24.

🤖 Generated with Claude Code

&& apt-get install -y nodejs \
&& apt-get autoremove -y \
&& apt-get clean -y \
Expand Down
2 changes: 1 addition & 1 deletion tools/release/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ token runs nothing but `npm`, the pinned actions and `publish-order.mjs` (node b
goes out after the six siblings whose ranges it carries. `--ignore-scripts` is deliberate:
`prepublishOnly` re-runs `bash build.sh`, and this job has no emscripten toolchain — the dist
being published is the artifact built in step 1 from the same commit. npm's version comes from
the exactly-pinned `node-version` (v24.19.0 → npm 11.17.0, past the 11.5.1 OIDC floor), so there
the exactly-pinned `node-version` (v24.20.0 → npm 11.19.0, past the 11.5.1 OIDC floor), so there
is no `npm install --global npm@latest` re-downloading an unpinned publisher every release.
4. **`github-releases`** — a GitHub Release per tag, from the package list `publish` uploaded. It is
a separate job so `gh release create`'s `contents: write` never coexists with the OIDC publish
Expand Down
Loading