Repository navigation
fix(cli): improve better-sqlite3 postinstall rebuild for cross-platform installs - #313
Conversation
…rm installs (cheaper-inference#312) Replace unreliable process.dlopen() platform detection with explicit platform/arch comparison against the build target (linux-x64). On macOS, dlopen can load an incompatible binary without throwing, causing the postinstall script to skip the rebuild entirely. - Detect platform mismatch via process.platform/arch instead of dlopen - Fail the install (exit 1) if rebuild fails, instead of warning silently - Verify rebuilt binary loads correctly after rebuild - Add pre-flight binary check in CLI entry point as a safety net
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness of Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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
The pull request significantly improves the cross-platform compatibility and reliability of the better-sqlite3 native module by enhancing its post-installation rebuild process and adding a pre-flight compatibility check. The changes introduce more robust platform detection, ensure rebuild failures are explicitly reported, and verify the rebuilt binary's loadability, preventing silent failures and providing clearer user guidance.
| console.warn(" You can fix this manually by running:"); | ||
| console.warn(` cd ${join(ROOT, "app")} && npm rebuild better-sqlite3\n`); | ||
| // Don't fail the install — the user can fix manually | ||
| console.error(" ❌ Failed to rebuild better-sqlite3 automatically."); |
There was a problem hiding this comment.
The catch block for the execSync command currently logs a generic error message. Including the actual error.message from the execSync failure could provide more specific debugging information to the user, which would be helpful for diagnosing rebuild issues.
| console.error(" ❌ Failed to rebuild better-sqlite3 automatically."); | |
| console.error(" ❌ Failed to rebuild better-sqlite3 automatically." + (error.message ? ` ${error.message}` : "")); |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // ── Pre-flight: verify better-sqlite3 native binary ─────── | ||
| // The published binary targets linux-x64. Check both platform match AND | ||
| // dlopen — on macOS, dlopen alone may succeed on an incompatible binary | ||
| // (false positive), so we check platform first as the primary signal. |
There was a problem hiding this comment.
WARNING: Misleading comment - The comment states "Check both platform match AND dlopen" and "we check platform first as the primary signal", but the actual implementation (lines 208-215) only performs the dlopen check without any platform verification. Either add the platform check or update the comment to accurately describe the runtime verification behavior.
| "\x1b[31m✖ better-sqlite3 native module is incompatible with this platform.\x1b[0m" | ||
| ); | ||
| console.error(` Run: cd ${APP_DIR} && npm rebuild better-sqlite3`); | ||
| if (platform() === "darwin") { |
There was a problem hiding this comment.
SUGGESTION: Consider using process.platform instead of platform() from the os module for consistency with the rest of the codebase. The process.platform values (linux, darwin, win32) are more commonly used and directly comparable to the BUILD_PLATFORM check in postinstall.mjs.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Other Observations (not in diff)The changes in
No issues found in the postinstall.mjs changes. Files Reviewed (2 files)
Positive aspects of this PR:
The issues found are minor documentation and consistency improvements. The core functionality is sound. |
|
Thanks @ardaaltinors for this excellent contribution! 🎉 The improved cross-platform detection is now merged and shipped in v2.2.9. Your fix for the silent macOS ARM rebuild failure will save a lot of frustration for Apple Silicon users. We really appreciate the thorough analysis and the pre-flight check in |
…-sqlite3-postinstall-rebuild fix(cli): improve better-sqlite3 postinstall rebuild for cross-platform installs
fix(aliases): custom model aliases applied to routing + restored on startup (cheaper-inference#315 cheaper-inference#316, PR cheaper-inference#317) fix(cli): better-sqlite3 postinstall rebuild cross-platform macOS ARM (cheaper-inference#312, PR cheaper-inference#313 @ardaaltinors)
…-sqlite3-postinstall-rebuild fix(cli): improve better-sqlite3 postinstall rebuild for cross-platform installs
fix(aliases): custom model aliases applied to routing + restored on startup (cheaper-inference#315 cheaper-inference#316, PR cheaper-inference#317) fix(cli): better-sqlite3 postinstall rebuild cross-platform macOS ARM (cheaper-inference#312, PR cheaper-inference#313 @ardaaltinors)
Fixes Dependabot alerts #312 and #313 (GHSA-q3jh-wrc6-jgq8, GHSA-8r9p-f939-6h3c).
Fixes #312
Summary
process.dlopen()platform detection inscripts/postinstall.mjswith explicitprocess.platform/process.archcomparison against the build target (linux-x64). On macOS,dlopencan load an incompatible binary without throwing, so the postinstall script skips the rebuild entirely.exit 1) if rebuild fails, instead of warning silently — users now see a clear error with platform-specific fix instructions duringnpm install, rather than discovering the issue at runtime.npm rebuild.bin/omniroute.mjsas a safety net before starting the server.