fix(ci): drop redundant better-sqlite3 rebuild in CLI Linux build#4049
Conversation
build-dist.ts's fixNativeBinariesForNode() already downloads the matching Node-ABI better-sqlite3 prebuild from GitHub releases and overwrites build/Release/better_sqlite3.node unconditionally, so running `npm rebuild better-sqlite3` in the CI install step is wasted work whose only effect is to introduce flake. Recent runs: - cli-v0.2.4: prebuild-install ENOENT → gyp config.gypi missing → fail - cli-v0.2.2: prebuild-install file too short → gyp EEXIST symlink → fail - cli-v0.2.3: succeeded - cli-v0.2.0/0.2.1: succeeded linux-arm64 doesn't hit this because it follows the same path but better- sqlite3's prebuild-install download apparently lands cleanly on arm64; the flake is linux-x64 specific. Either way, neither needs the rebuild — the final binary in the tarball comes from build-dist's GitHub-release fetch. node-pty rebuild stays (no Linux prebuilds ship in the npm package).
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Linux dependency-install step in the CLI build workflow now uses ChangesCLI Build Workflow Optimization
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Greptile SummaryRemoves the Confidence Score: 5/5Safe to merge — the removed step was provably redundant and the smoke test validates the resulting binary. Single-line deletion with thorough justification backed by the source code of fixNativeBinariesForNode(). The existing smoke test exercises require('better-sqlite3') post-build, providing a direct regression guard. No logic changes, only CI workflow cleanup. No files require special attention.
|
| Filename | Overview |
|---|---|
| .github/workflows/build-cli.yml | Removes the redundant npm rebuild better-sqlite3 step from the Linux install and adds detailed comments explaining why it was redundant; all other rebuilds remain intact. |
Sequence Diagram
sequenceDiagram
participant CI as GitHub Actions (Linux)
participant Bun as bun install
participant NPM as npm rebuild
participant Script as build-dist.ts fixNativeBinariesForNode()
participant GH as GitHub Releases
CI->>Bun: bun install --frozen --ignore-scripts
Bun-->>CI: deps installed (scripts skipped)
CI->>NPM: npx node-gyp rebuild (node-pty)
NPM-->>CI: node-pty.node compiled
note over CI,NPM: better-sqlite3 rebuild REMOVED (was redundant + flaky)
CI->>NPM: npm rebuild @parcel/watcher
NPM-->>CI: watcher.node compiled
CI->>Script: bun run build:dist
Script->>GH: curl better-sqlite3 Node-ABI tarball
GH-->>Script: better_sqlite3.node prebuild
Script->>Script: overwrite lib/node_modules/better-sqlite3/build/Release/
Script-->>CI: dist/ artifact ready
CI->>CI: Smoke test: require('better-sqlite3') OK
Reviews (1): Last reviewed commit: "fix(ci): drop redundant npm rebuild bett..." | Re-trigger Greptile
Already integrated, superseded by current versions, or net-empty in this fork: superset-sh#3881 superset-sh#3887 superset-sh#3917 superset-sh#3925 superset-sh#3940 superset-sh#3956 superset-sh#3961 superset-sh#3974 superset-sh#4017 superset-sh#4048 superset-sh#4049 superset-sh#4055 superset-sh#4063 superset-sh#4070 superset-sh#4092 superset-sh#4110 superset-sh#4138 superset-sh#4159 superset-sh#4163 superset-sh#4164 superset-sh#4209 superset-sh#4210 superset-sh#4249 superset-sh#4349 superset-sh#4405 superset-sh#4462 superset-sh#4464 superset-sh#4494 superset-sh#4495 superset-sh#4500 superset-sh#4535 superset-sh#4541 superset-sh#4566 superset-sh#4580 superset-sh#4589 superset-sh#4593 superset-sh#4603 superset-sh#4637 superset-sh#4642 superset-sh#4655 superset-sh#4657 superset-sh#4659 superset-sh#4685 superset-sh#4692 superset-sh#4745 superset-sh#4789 superset-sh#4797 superset-sh#4824 superset-sh#4835 superset-sh#4847 superset-sh#4885 superset-sh#4896.
Why
The
cli-v0.2.4release just bricked onlinux-x64(run 25340826401) at theInstall dependencies (Linux)step:Same step has bricked before with a different mode each time:
prebuild-install ENOENTthen gyp config.gypi missingprebuild-install file too shortthen gypEEXISTonpython3symlinkprebuild-installis intermittently mis-extracting / failing the download, and when it falls through tonode-gyp rebuild --releasethe leftover state breaks the gyp run.What changes
Remove
npm rebuild better-sqlite3from the Linux install step. It was redundant:packages/cli/scripts/build-dist.ts:282-297(fixNativeBinariesForNode) already does— it unconditionally fetches the Node-ABI prebuild from GitHub releases and overwrites
lib/node_modules/better-sqlite3/build/Release/better_sqlite3.nodein the dist. Whatevernpm rebuildproduced was just thrown away.node-ptyrebuild stays (no Linux prebuilds ship in the npm package — node-pty really is compiled from source).@parcel/watcherrebuild also stays for now (haven't seen it flake; out of scope).Test plan
build-cli.ymlfromci.yml)release-cli.ymlviaworkflow_dispatchonmainto verify all three matrix targets land artifactsrequire('better-sqlite3')still passes against the bundled Node — that's what provesfixNativeBinariesForNode's GitHub fetch produced a usable binarySummary by cubic
Remove
npm rebuild better-sqlite3from the CLI Linux build to stop flakylinux-x64failures. The dist step already fetches and overwrites the correctbetter-sqlite3prebuild viafixNativeBinariesForNode().better-sqlite3rebuild that triggered intermittentprebuild-install/node-gyperrors.node-ptyand@parcel/watcherrebuilds;node-ptyrequires source build on Linux and watcher remains unchanged.Written for commit a71b7dc. Summary will update on new commits.
Summary by CodeRabbit