[BLOCKED — DO NOT MERGE] build(deps): better-sqlite3 13.0.3 pin + prebuild prune - #337
Open
KnockOutEZ wants to merge 2 commits into
Open
[BLOCKED — DO NOT MERGE] build(deps): better-sqlite3 13.0.3 pin + prebuild prune#337KnockOutEZ wants to merge 2 commits into
KnockOutEZ wants to merge 2 commits into
Conversation
The desktop shell and the core must load the SAME native module once the DB broker retires, and only v13 can be loaded by both: its prebuilds are Node-API (no ABI suffix, napi 10), so one binary spans Node ABI 127 and the shell's 148. The 12.9.0 pin is V8-ABI-bound via node-gyp and fails under the shell with NODE_MODULE_VERSION 127 vs 148 — measured, not inferred. v13 ships all eight targets in one tarball, so the bump alone costs 16 MiB of prebuilds/ and REDS the blocking install-size gate. Measured on the gate's own protocol, darwin-arm64: 371 MiB at the old pin, 387 unpruned (FAIL, limit 383), 372 with the prune. Re-measured as a back-to-back pair on the current base: 372 -> 374, so the bump costs +2 MiB net and the anchor moves 375 -> 377. The limit deliberately does not move, and the derivation changes hands: at the new anchor this prune lapsing (worth 14 MiB, landing at 391) is a SMALLER regression than the browser driver returning to dependencies (17 MiB, 394), so it now sets the window at 377..391. Accommodating the unpruned 387 would have needed a limit above 388 and blinded the gate to a regression this very change introduces the possibility of. Also fixes the single-file binary channel, which the bump would otherwise break silently: pkg.config.json named build/Release/better_sqlite3.node, a path v13 does not ship. v13's prebuild name is computed at runtime, so pkg cannot find it by static analysis and it must be declared per target — and it moves out of the composer's platform-agnostic set, since that path is no longer the same on every platform. The prune keeps both libc builds on linux: the loader's musl probe is a genuine runtime check, but this runs at install time in a different process, and a multi-stage container can populate under glibc and run under musl. Removal is allowlisted rather than pattern-matched because `linuxmusl` is not a value process.platform ever takes, so interpolation would delete the file a musl host is the only one that can load. The planner refuses outright unless it has positively identified a loadable prebuild: unlike the other prunes here, a wrong removal does not degrade a capability, it throws on require and the cache database never opens. Corrects studio-mcp-server.ts's stated reason for existing, which this pin falsifies. It claimed the subsystem graph CANNOT load in the desktop shell because that shell rejects 12.9.0. That was true of the old pin; 13.0.3 loads there. The module stays separate on boot cost, not on an ABI wall.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CI measured what a green macOS run could not: every macOS and Linux row passes and every Windows row fails, at `npm ci`, with node-gyp unable to find Visual Studio. v12 shipped an install script (prebuild-install) that ran in pure Node and exited 0 as soon as it found the bundled binary, so node-gyp was never invoked. v13 removed it, and npm supplies its own implicit `node-gyp rebuild` for any package carrying a binding.gyp — ignoring the `gypfile: false` better-sqlite3 publishes to suppress exactly that (npm/cli#9837, WiseLibs/better-sqlite3#1503 and #1505). binding.gyp's own prebuild_exists guard cannot help, because gyp must configure before it can evaluate a variable and configure is the step that needs the toolchain. This is a user-facing install regression, not a CI-image detail: a Windows user without Python and VC++ Build Tools could not `npm i wigolo` at all. The clean-machine job exists to catch precisely that, and it did. Retracts the claim in the previous commit's ci.yml comment that v13 "never falls back to a source compile on any row". It was written from a macOS result and the Windows matrix falsifies it.
KnockOutEZ
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
⛔ Do not merge. The v13 pin breaks Windows installs.
CI on my own head SHA measured a clean platform split: every macOS and Linux row green, every Windows row red.
Base
ee346f77is 16/16 green including all Windows jobs, so these are mine.Cause
npm cion Windows runsnode-gyp rebuildfor better-sqlite3 and dies atfind VS could not find a version of Visual Studio 2017 or newer.v12 shipped an
installscript (prebuild-install) that ran in pure Node and exited 0 the moment it found the bundled binary — node-gyp was never invoked. v13 removed it, and npm then supplies its own implicitnode-gyp rebuildfor any package carrying abinding.gyp, ignoring thegypfile: falsebetter-sqlite3 publishes to suppress exactly that. Upstream: npm/cli#9837, WiseLibs/better-sqlite3#1503, #1505.binding.gyp's own
prebuild_existsguard cannot save it — gyp must configure before it can evaluate any variable, and configure is the step needing the toolchain.Why this is not a CI-image problem to paper over
A Windows user without Python + VC++ Build Tools could not
npm i wigoloat all. The clean-machine gate exists to catch that class, and it did.What is still good here, if v13 ever becomes viable
scripts/prune/sqlite-prebuilds.mjs+ 22 tests (2 mutations verified to kill 3 tests each). v13 ships all 8 targets in one tarball; unpruned it reds the blocking G-DIET gate (372 → 387, limit 383), pruned it passes (374).packaging/binary/pkg.config.jsonfix: it namedbuild/Release/better_sqlite3.node, a path v13 does not ship, so the single-file binary channel would have broken silently.Revisit when npm/cli#9837 lands or better-sqlite3 restores a JS-only install script.