Skip to content

fix(desktop): verify better-sqlite3's shipped prebuild - #485

Open
lucas77778 wants to merge 1 commit into
masterfrom
fix/better-sqlite3-13-prebuilds
Open

fix(desktop): verify better-sqlite3's shipped prebuild#485
lucas77778 wants to merge 1 commit into
masterfrom
fix/better-sqlite3-13-prebuilds

Conversation

@lucas77778

Copy link
Copy Markdown
Member

The v0.27.0 release build failed on all three platforms with:

✗ mac/LinkCode.app/Contents/Resources: missing native binding node_modules/better-sqlite3/build/Release/better_sqlite3.node (better-sqlite3)

better-sqlite3 13 (#443) changed how the binding is distributed. v12 downloaded a per-Node-ABI
build into build/Release/better_sqlite3.node via prebuild-install; v13 ships eight N-API
(NAPI_VERSION=10) prebuilds in the tarball as prebuilds/<platform>-<arch>.node, and its
binding.gyp turns both targets into type: none whenever a prebuild exists. node-gyp therefore
only touches its stamps, @electron/rebuild is a no-op, and nothing is ever written to
build/Release — the packaged app is fine, verify-artifacts.mts was just looking in the old
place. Every other check in that run passed.

verify-artifacts.mts now asserts the target's prebuild instead, keeping the Mach-O/PE/ELF arch
check. Three stale comments about prebuild-install / build/Release are corrected with it.

Verification

  • The prebuild loads under Electron 43 (Node 24.18.0, N-API 10) and runs SQL — no rebuild needed.
  • Packed an arm64 dev shell: @electron/rebuild stays a no-op and
    app.asar.unpacked/node_modules/better-sqlite3/prebuilds/darwin-arm64.node is in place.
  • The updated check reports no problems for that app, and correctly reports the missing
    darwin-x64.node when pointed at an arm64 app as if it were the x64 one.
  • Launched the packaged app: the daemon boots and migrates daemon.db, so the shipped prebuild is
    what it loads.
  • pnpm check:ci passes.

Follow-up (not in this PR)

v13's tarball carries all eight prebuilds and electron-builder smart-unpacks the whole module, so
every artifact now ships ~15 MB of foreign-platform .node files. Pruning the staging dir down to
the target (plus the host's, which is what keeps binding.gyp from compiling from source) removes
that; left out here to keep the release unblocking minimal.

Copilot AI lite review requested due to automatic review settings August 27, 2026 13:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@lucas77778
lucas77778 requested a review from xiaoland August 27, 2026 13:03
@greptile-apps

greptile-apps Bot commented Aug 27, 2026

Copy link
Copy Markdown

Greptile Summary

The PR updates desktop artifact verification for better-sqlite3 v13’s shipped N-API prebuild layout instead of expecting the legacy rebuilt binding.

  • Maps electron-builder platform names to Node platform tokens and verifies the target architecture’s prebuild.
  • Retains native binary architecture validation for macOS, Windows, and Linux artifacts.
  • Corrects packaging documentation and comments describing native dependency handling.

Confidence Score: 5/5

The PR appears safe to merge, with the verifier aligned to the shipped better-sqlite3 v13 prebuilds on every supported desktop target.

The supported platform and architecture mappings resolve to the documented v13 prebuild filenames, unsupported platforms are rejected before verification, and the retained native-header checks continue to detect architecture mismatches.

Important Files Changed

Filename Overview
apps/desktop/scripts/verify-artifacts.mts Updates better-sqlite3 verification to inspect the target-specific v13 N-API prebuild while preserving binary architecture checks.
apps/desktop/electron-builder.yml Corrects comments to reflect that packaged applications load better-sqlite3’s shipped prebuild rather than a rebuilt Release binding.
apps/daemon/scripts/package-daemon.mts Updates packaging documentation to distinguish better-sqlite3’s target prebuilds from host-specific native dependencies.
apps/daemon/AGENTS.md Aligns daemon packaging guidance with the current native dependency distribution model.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Target platform and architecture] --> B[Map builder platform to Node platform]
    B --> C[Resolve better-sqlite3 prebuild path]
    C --> D[Check app.asar.unpacked]
    D --> E[Validate Mach-O, PE, or ELF architecture]
    E --> F[Accept or reject artifact]
Loading

Reviews (1): Last reviewed commit: "fix(desktop): verify better-sqlite3's sh..." | Re-trigger Greptile

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ℹ️ The fix itself is correct and I could not find a way for it to fail. Two comment-accuracy points inline, plus two more stale sites the sweep missed.

Reviewed changes — the whole 1-commit diff (4 files), plus independent verification of the better-sqlite3 v13 layout, its binding-resolution order, and electron-builder's smart-unpack behavior against the installed packages.

  • verify-artifacts.mts asserts the shipped prebuildNATIVE_BINDING is replaced by sqliteBinding(platform, arch) over a new NODE_PLATFORM map, targeting node_modules/better-sqlite3/prebuilds/<nodePlatform>-<arch>.node; the Mach-O/PE/ELF arch check is retained and the entry becomes conditional, mirroring keyringBinding.
  • Three comment correctionselectron-builder.yml, apps/daemon/scripts/package-daemon.mts, and apps/daemon/AGENTS.md drop their prebuild-install / build/Release claims.

What I verified independently, since the whole PR rests on it:

  • better-sqlite3@13.0.2 ships eight flat prebuilds/<platform>-<arch>.node files, gypfile: false, no install script, and binding.gyp sets both targets to type: none when a prebuild exists — so @electron/rebuild really is a no-op and build/ holds only node-gyp stamps.
  • lib/binding.js's getPrebuildPath() is tried before build/Release, so the asserted file is genuinely the one the daemon loads.
  • app-builder-lib/out/asar/unpackDetector.js adds stat.moduleRootPath to autoUnpackDirs on any .node hit, so the whole better-sqlite3 directory is smart-unpacked — the asserted path is correct for all six mac/win/linux × x64/arm64 targets, not just the arm64 mac the description covers.
  • Nothing in .github/workflows/, the packaging scripts, or the E2E suite still references build/Release/better_sqlite3.node.

ℹ️ The stale-comment sweep stops two sites short

The PR corrects three comments but leaves two more carrying the same pre-v13 premise, one of which now directly contradicts an edit in this diff.

Technical details
# Two more `@electron/rebuild` / `build/Release` claims left stale

## Affected sites
- `docs/RELEASE.md:257` — describes this exact gate as "the smartUnpacked `better-sqlite3` binding is
  present and its Mach-O/PE/ELF header targets the app's arch (**the tripwire for the CODE-107
  Windows rebuild miss** …)". There is no rebuild left to miss: `binding.gyp` short-circuits to
  `type: none` and the prebuild is named by arch, so the header check can no longer fail for that
  reason. The sentence should describe what the gate now covers (the prebuild for the target is
  present and smart-unpacked) rather than a tripwire that no longer exists.
- `apps/desktop/scripts/package-app.mts:116``pruneStaging`'s doc says "notably
  better-sqlite3/deps must stay HERE in staging because `@electron/rebuild` compiles from it before
  collection", presenting the from-source compile as the routine path. The `electron-builder.yml`
  edit in this diff (new lines 61-63) now says `deps` is "only ever compiled if a from-source
  `@electron/rebuild` happens". The two now disagree about the same fact.

## Required outcome
- Both sites state the post-v13 reality, and `electron-builder.yml` and `package-app.mts` agree on
  whether the `better-sqlite3/deps` compile is routine or conditional.

## Open questions for the human
- Given the compile is now unreachable in practice, is keeping `better-sqlite3/deps` in staging
  still worth stating as a constraint, or has it become a plain "harmless, excluded from the asar
  anyway"? That answer decides how both comments should read.

ℹ️ Nitpicks

  • NODE_PLATFORM (verify-artifacts.mts:79-83) has to stay key-for-key with EXPECTED; a fourth platform added to EXPECTED without a matching entry makes sqliteBinding return null and the better-sqlite3 gate disappear silently instead of failing. Partial<Record<string, string>> gives no help here. Not worth a runtime guard — but the two maps being coupled is invisible from either one.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment on lines +16 to +18
* this targets plain Node. better-sqlite3 carries a NAPI prebuild per target, but the host-only
* napi-rs optional deps and @sentry's ABI-pinned profiler do not: a same-platform artifact — build
* it on (or for) each target.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The @sentry half of this rationale doesn't hold. @sentry/node-cpu-profiler (pulled in by @sentry/profiling-node) ships 35 binaries in its own tarball — lib/sentry_cpu_profiler-{darwin,linux,win32}-{x64,arm64}[-glibc|-musl]-{108,115,127,137,147}.node — so it very much does carry a prebuild per target; it has no install-time platform scoping, and check-build.js prunes nothing. What it is pinned to is the Node module ABI, which is a same-Node-version constraint, not a same-platform one. The conclusion is still right, but it rests entirely on @napi-rs/keyring, whose optional deps really do install host-triple-only (only @napi-rs/keyring-linux-x64-gnu is present in this checkout). Worth naming the one true reason rather than two, since a future reader deciding whether the standalone daemon can be cross-built will act on this sentence. (If you want a second genuine platform constraint, the profiler ships no win32-arm64 binary at all.)

Comment thread apps/daemon/AGENTS.md
binary — a **same-platform** artifact, build per target), and it prunes the host-arch agent CLI
platform packages (the daemon downloads them at runtime via `@linkcode/assets`, as the desktop
distinct from the desktop bundle: it targets **plain Node** (still a **same-platform** artifact — the
napi-rs optional deps and @sentry's profiler resolve host-only — build per target), and it prunes

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same correction as package-daemon.mts — "@sentry's profiler resolve[s] host-only" is the clause that's wrong: it ships every platform × ABI in its tarball. @napi-rs/keyring is what makes this a same-platform artifact.

Comment on lines +85 to +88
* better-sqlite3's binding, smartUnpacked beside the asar; the daemon requires it at boot. Since
* v13 it is one NAPI prebuild per platform-arch shipped in the tarball (`build/Release` is only
* written when node-gyp actually compiles, which it now skips), so what breaks is the staging
* prune keeping the wrong target — every client then shows "Unable to connect to the daemon".

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"what breaks is the staging prune keeping the wrong target" isn't reachable in this repo yet: pruneStaging in package-app.mts:118 only deletes .map/.ts/.md files and never touches prebuilds/, and the files globs exclude only better-sqlite3/deps/** — so all eight prebuilds ship in every artifact today. That makes this gate an existence/smart-unpack check, with the arch-header assertion tautological (the file is named by arch and ships verbatim from the tarball). Worth saying that plainly instead, since the sentence as written describes the state after the prune you deferred to a follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants