Repository navigation
fix(cli): fix postinstall native binary rebuild regression (#321) - #327
diegosouzapw merged 4 commits into
Conversation
…ding (cheaper-inference#321) The standalone app/ directory created by Next.js only contains runtime files for better-sqlite3 (no binding.gyp, no source, no prebuild-install), so `npm rebuild` inside app/ is a no-op. The previous fix (cheaper-inference#312) added exit(1) on rebuild failure, which caused npm to rollback the entire package installation — leaving users with nothing to fix manually. New approach: 1. Check if existing binary is already compatible (dlopen) 2. Copy the correctly-built binary from root node_modules/ (npm already compiles it for the correct platform during install) 3. Fall back to npm rebuild if root binary is unavailable 4. Warn but don't fail the install if nothing works — the package stays installed and the CLI pre-flight check gives a clear error at startup
…dlopen Add native-binary-compat module that reads ELF/Mach-O/PE headers to determine the actual target platform/arch of the .node binary. This eliminates the macOS false-positive where dlopen loads a linux-x64 binary without throwing. - Parse ELF (linux), Mach-O (darwin), and PE (win32) binary formats - Use header-based check as primary signal, dlopen as secondary - Update pre-flight check in CLI to use the new module - Add unit tests for all binary formats and cross-platform scenarios
The module is imported by bin/omniroute.mjs but was missing from the files array in package.json, causing ERR_MODULE_NOT_FOUND on global installs.
…install - Read only first 4096 bytes of binary header instead of entire file - Add error logging to all catch blocks with specific failure messages - Separate copy vs dlopen catch blocks in postinstall Strategy 1 - Add archCount sanity cap (max 30) for fat Mach-O parsing - Distinguish timeout vs rebuild failure in Strategy 2
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 resolves a significant installation regression by overhauling how the 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
Activity
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
This pull request provides a robust fix for a postinstall script regression that caused installation rollbacks. The new approach of copying a pre-compiled binary before attempting a rebuild is a significant improvement. The introduction of a native binary compatibility checker that inspects file headers is an excellent and well-implemented solution to avoid platform-related issues. The changes greatly enhance the installation experience for users on different operating systems. I've added a couple of suggestions to refine the error handling in the postinstall script to provide clearer feedback if any step fails.
| try { | ||
| mkdirSync(dirname(appBinary), { recursive: true }); | ||
| copyFileSync(rootBinary, appBinary); | ||
| } catch (err) { | ||
| console.warn(` ⚠️ Failed to copy binary: ${err.message}`); | ||
| } | ||
|
|
||
| if (!needsRebuild) { | ||
| try { | ||
| process.dlopen({ exports: {} }, buildInfoPath); | ||
| process.dlopen({ exports: {} }, appBinary); | ||
| console.log(" ✅ Native module fixed successfully!\n"); | ||
| process.exit(0); | ||
| } catch { | ||
| // Same platform but binary still incompatible (e.g. Node.js ABI mismatch) — rebuild | ||
| } catch (err) { | ||
| console.warn(` ⚠️ Copied binary failed to load: ${err.message}`); | ||
| } |
There was a problem hiding this comment.
The current structure with two separate try...catch blocks can lead to confusing log messages. If copyFileSync fails, the script will still attempt to dlopen the (likely incompatible or non-existent) binary in appBinary and log 'Copied binary failed to load', which is misleading. Nesting the dlopen check inside the try block for the file copy would make the logic more robust and the error reporting clearer.
try {
mkdirSync(dirname(appBinary), { recursive: true });
copyFileSync(rootBinary, appBinary);
try {
process.dlopen({ exports: {} }, appBinary);
console.log(" ✅ Native module fixed successfully!\n");
process.exit(0);
} catch (err) {
console.warn(` ⚠️ Copied binary failed to load: ${err.message}`);
}
} catch (err) {
console.warn(` ⚠️ Failed to copy binary: ${err.message}`);
}| try { | ||
| const { execSync } = await import("node:child_process"); | ||
| execSync("npm rebuild better-sqlite3", { | ||
| cwd: join(ROOT, "app"), | ||
| stdio: "inherit", | ||
| timeout: 120_000, | ||
| }); | ||
| } catch (error) { | ||
| console.error(" ❌ Failed to rebuild better-sqlite3 automatically."); | ||
| console.error(" You can fix this manually by running:"); | ||
| console.error(` cd ${join(ROOT, "app")} && npm rebuild better-sqlite3`); | ||
| if (process.platform === "darwin") { | ||
| console.error(" If build tools are missing: xcode-select --install"); | ||
| } | ||
| console.error(""); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| // Verify the rebuilt binary actually loads | ||
| try { | ||
| process.dlopen({ exports: {} }, buildInfoPath); | ||
| process.dlopen({ exports: {} }, appBinary); | ||
| console.log(" ✅ Native module rebuilt successfully!\n"); | ||
| } catch { | ||
| console.error(" ❌ Rebuild completed but binary is still incompatible."); | ||
| console.error(" Try manually:"); | ||
| console.error(` cd ${join(ROOT, "app")} && npm rebuild better-sqlite3`); | ||
| if (process.platform === "darwin") { | ||
| console.error(" If build tools are missing: xcode-select --install"); | ||
| process.exit(0); | ||
| } catch (err) { | ||
| const isTimeout = err.killed || err.signal === "SIGTERM"; | ||
| if (isTimeout) { | ||
| console.warn(" ⚠️ npm rebuild timed out after 120s."); | ||
| } else { | ||
| console.warn(` ⚠️ npm rebuild failed: ${err.message}`); | ||
| } | ||
| console.error(""); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Similar to the copy logic, this try...catch block combines the execSync call for rebuilding and the dlopen verification. If the rebuild succeeds but dlopen fails, the error message 'npm rebuild failed' is inaccurate. Separating these into nested try...catch blocks will provide more precise feedback to the user about which step actually failed.
try {
const { execSync } = await import("node:child_process");
execSync("npm rebuild better-sqlite3", {
cwd: join(ROOT, "app"),
stdio: "inherit",
timeout: 120_000,
});
try {
process.dlopen({ exports: {} }, appBinary);
console.log(" ✅ Native module rebuilt successfully!\n");
process.exit(0);
} catch (err) {
console.warn(` ⚠️ Rebuilt binary failed to load: ${err.message}`);
}
} catch (err) {
const isTimeout = err.killed || err.signal === "SIGTERM";
if (isTimeout) {
console.warn(" ⚠️ npm rebuild timed out after 120s.");
} else {
console.warn(` ⚠️ npm rebuild failed: ${err.message}`);
}
}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
The PR implements a robust solution for native binary compatibility:
|
feat(claude): [1m] suffix for 1M extended context (PR #311 @DavyMassoneto) feat(registry): new models for iFlow, Qwen, Kimi (PR #326 @nyatoru) fix(cli): postinstall binary copy instead of rebuild (PR #327 @ardaaltinors, fixes #321) docs: English Remote OAuth guide in README (PR #329, fixes #318) test: 3 unit tests for parseModel [1m] suffix
|
✅ Merged in v2.3.2! Fantastic work @ardaaltinors — the ELF/Mach-O/PE header detection approach is exactly the right fix. Those 4 Docker test results seal it. Thank you! 🚀 |
…stall-copy-native-binary fix(cli): fix postinstall native binary rebuild regression (cheaper-inference#321)
feat(claude): [1m] suffix for 1M extended context (PR cheaper-inference#311 @DavyMassoneto) feat(registry): new models for iFlow, Qwen, Kimi (PR cheaper-inference#326 @nyatoru) fix(cli): postinstall binary copy instead of rebuild (PR cheaper-inference#327 @ardaaltinors, fixes cheaper-inference#321) docs: English Remote OAuth guide in README (PR cheaper-inference#329, fixes cheaper-inference#318) test: 3 unit tests for parseModel [1m] suffix
…stall-copy-native-binary fix(cli): fix postinstall native binary rebuild regression (cheaper-inference#321)
feat(claude): [1m] suffix for 1M extended context (PR cheaper-inference#311 @DavyMassoneto) feat(registry): new models for iFlow, Qwen, Kimi (PR cheaper-inference#326 @nyatoru) fix(cli): postinstall binary copy instead of rebuild (PR cheaper-inference#327 @ardaaltinors, fixes cheaper-inference#321) docs: English Remote OAuth guide in README (PR cheaper-inference#329, fixes cheaper-inference#318) test: 3 unit tests for parseModel [1m] suffix
Dependabot alerts #327 (high), #328 and #333 (critical) flag next >= 16.0.0, <= 16.4.0. Same version and lockfile delta as the Dependabot PR #17969 (next + @next/env + the @next/swc-* binaries 16.3.8 -> 16.4.2), whose Build, E2E 1-9/9, Electron package smoke and Ecosystem jobs passed with it. Landing it next to the dompurify bump clears all three critical npm audit findings on the base (dompurify, monaco-editor via dompurify, next), which the Lint job's `npm audit --audit-level=critical` blocks on; on separate PRs each one still carries the other's critical finding.
Fixes issue #321
Summary
PR #313 introduced
process.exit(1)on postinstall rebuild failure, which caused npm to rollback the entire package — leaving users with nothing installed. The root cause was thatnpm rebuild better-sqlite3inside the standaloneapp/directory is a no-op (Next.js file tracing only copies runtime files, notbinding.gyp, source, orprebuild-install).New approach
Instead of rebuilding, copy the correctly-built binary from root
node_modules/. npm already compilesbetter sqlite3for the correct platform duringnpm install -g(it's a top-level dependency). The postinstall now simply copies that binary into the standaloneapp/directory.Changes
scripts/postinstall.mjs— 3-tier strategy:node_modules/toapp/node_modules/(no rebuildneeded)
npm rebuildif root binary unavailableexit(1)if nothing works — package stays installedscripts/native-binary-compat.mjs(new) — Reads ELF/Mach-O/PE binary headersto detect actual target platform/arch, avoiding macOS dlopen false positives. Only
reads first 4KB of the file.
bin/omniroute.mjs— Pre-flight binary compatibility check using headerdetection before starting server
package.json— Addednative-binary-compat.mjstofilesarrayDocker test results (4/4 pass)
Test plan
npm test— 602/602 passnpm run lint— 0 errors