-
Notifications
You must be signed in to change notification settings - Fork 804
fix: make headless api-key auth work end-to-end (CLI + server org resolution) #3493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
f303c11
a507841
a27eaf9
89e159b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -232,8 +232,10 @@ function copyNativePackages(libDir: string, target: Target): void { | |
| * | ||
| * 1. `better-sqlite3`: download the Node-ABI prebuild from GitHub and | ||
| * overwrite `build/Release/better_sqlite3.node`. | ||
| * 2. `node-pty`: delete `build/Release/` so the `bindings` loader falls | ||
| * through to the N-API prebuild in `prebuilds/<target>/pty.node`. | ||
| * 2. `node-pty`: on darwin, fall back to the in-package N-API prebuild | ||
| * by removing `build/`. On linux-x64 there is no upstream prebuild, | ||
| * so rebuild from source via node-gyp; node-pty uses node-addon-api | ||
| * so the resulting binding is ABI-stable across Node versions. | ||
| */ | ||
| async function fixNativeBinariesForNode( | ||
| libDir: string, | ||
|
|
@@ -262,7 +264,22 @@ async function fixNativeBinariesForNode( | |
| join(bsqDest, "better_sqlite3.node"), | ||
| ); | ||
|
|
||
| const nodePtyBuild = join(destModules, "node-pty", "build"); | ||
| const nodePtyDir = join(destModules, "node-pty"); | ||
| const nodePtyBuild = join(nodePtyDir, "build"); | ||
|
|
||
| if (target === "linux-x64") { | ||
| if (existsSync(nodePtyBuild)) { | ||
| rmSync(nodePtyBuild, { recursive: true, force: true }); | ||
| } | ||
| console.log("[build-dist] compiling node-pty from source for linux-x64"); | ||
| await exec("npx", ["--yes", "node-gyp", "rebuild"], nodePtyDir); | ||
| const builtBinding = join(nodePtyBuild, "Release", "pty.node"); | ||
| if (!existsSync(builtBinding)) { | ||
| throw new Error(`node-pty build did not produce ${builtBinding}`); | ||
| } | ||
| return; | ||
| } | ||
|
Comment on lines
+278
to
+291
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
After The darwin path avoids this cleanly by removing const builtBinding = join(nodePtyBuild, "Release", "pty.node");
if (!existsSync(builtBinding)) {
throw new Error(`node-pty build did not produce ${builtBinding}`);
}
// Strip intermediate artifacts — only the .node file is needed at runtime
const objTarget = join(nodePtyBuild, "Release", "obj.target");
if (existsSync(objTarget)) rmSync(objTarget, { recursive: true, force: true });
// Also remove non-Release subdirectories that node-gyp generates
for (const entry of readdirSync(nodePtyBuild)) {
if (entry !== "Release") {
rmSync(join(nodePtyBuild, entry), { recursive: true, force: true });
}
}
return;Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/cli/scripts/build-dist.ts
Line: 276-281
Comment:
**Build artifacts bloat the linux tarball**
After `node-gyp rebuild` succeeds, only `build/Release/pty.node` is needed at runtime. The entire `build/` directory — which includes `obj.target/` (compiled `.o` files), `Makefile`, `config.gypi`, and other intermediate artifacts — stays in the staging tree and is packed into the tarball. Depending on the machine, this can easily add 5–20 MB.
The darwin path avoids this cleanly by removing `build/` entirely (darwin falls back to the in-package prebuilds). The linux path should do the same with a targeted cleanup after the verification:
```ts
const builtBinding = join(nodePtyBuild, "Release", "pty.node");
if (!existsSync(builtBinding)) {
throw new Error(`node-pty build did not produce ${builtBinding}`);
}
// Strip intermediate artifacts — only the .node file is needed at runtime
const objTarget = join(nodePtyBuild, "Release", "obj.target");
if (existsSync(objTarget)) rmSync(objTarget, { recursive: true, force: true });
// Also remove non-Release subdirectories that node-gyp generates
for (const entry of readdirSync(nodePtyBuild)) {
if (entry !== "Release") {
rmSync(join(nodePtyBuild, entry), { recursive: true, force: true });
}
}
return;
```
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| if (existsSync(nodePtyBuild)) { | ||
| console.log( | ||
| "[build-dist] removing node-pty build/ so bindings falls back to prebuilds/", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node-gyp rebuildrequires Python 3 and a C++ toolchainnode-gyp rebuildwill fail with a cryptic error ifpython3orgcc/g++are not available on the build machine. This path is new to the script and could surprise first-time linux contributors. Consider adding a preflight check or a clearer failure message:Or check explicitly before calling
exec:Prompt To Fix With AI