-
-
Notifications
You must be signed in to change notification settings - Fork 64
feat(sea): let users override the base Node binary #279
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 4 commits
408dfde
8dd9bc1
9d9d4d3
cc6e680
f5a1d3a
f47221b
32b66d4
1648fc2
a2834a7
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 |
|---|---|---|
|
|
@@ -329,6 +329,133 @@ async function getNodeVersion( | |
| return latest.version as NodeVersion; | ||
| } | ||
|
|
||
| /** | ||
| * The custom base Node binary to embed for SEA, or `undefined` to download one. | ||
| * | ||
| * Precedence matches standard mode: an explicit `opts.nodePath` (from the | ||
| * `--sea-node-path` CLI flag or the `seaNodePath` pkg-config key) wins over the | ||
| * `PKG_NODE_PATH` environment variable. `PKG_NODE_PATH` is the same env var | ||
| * pkg-fetch honours for the traditional build path (`localPlace()`); folding it | ||
| * in here makes it work for SEA too, instead of a separate SEA-only mechanism. | ||
| */ | ||
| function resolveCustomBaseNode( | ||
| opts: GetNodejsExecutableOptions, | ||
| ): string | undefined { | ||
| if (opts.nodePath) return opts.nodePath; | ||
| if (process.env.PKG_NODE_PATH) return resolve(process.env.PKG_NODE_PATH); | ||
| return undefined; | ||
| } | ||
|
|
||
| /** Target-suffix -> the value Node reports as `process.platform`. */ | ||
| const PROCESS_PLATFORM: Record<string, string> = { | ||
|
Member
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. [Major] · Correctness The comment claims alpine/linuxstatic "report their own name verbatim" — they don't: a musl/static Node binary reports Why: the PR's own docs promise a single alpine/linuxstatic target works ("matching that to linux/alpine/linuxstatic remains your responsibility") — exactly the musl / old-glibc custom-runtime case this PR exists for. The guard runs before any Fix: map |
||
| macos: 'darwin', | ||
| win: 'win32', | ||
| // linux / alpine / linuxstatic / freebsd report their own name verbatim, so | ||
| // they need no mapping. (ELF can't reveal the glibc/musl/static flavor | ||
| // either, so that sub-distinction stays the user's responsibility.) | ||
| }; | ||
|
|
||
| /** What a Node binary reports about itself when run. */ | ||
| async function probeNode( | ||
|
Member
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. [Minor] · Robustness Returning Fix: keep the probe (it's free — you already exec for the version), but wrap the exec failure with context: "couldn't run the custom base Node on this host; it must be runnable here to validate platform/arch/version (got …)." Turns a cryptic |
||
| binPath: string, | ||
| ): Promise<{ version: string; platform: string; arch: string }> { | ||
| if (binPath === process.execPath) { | ||
| return { | ||
| version: process.version, | ||
| platform: process.platform, | ||
| arch: process.arch, | ||
| }; | ||
| } | ||
| const { stdout } = await execFileAsync(binPath, [ | ||
| '-p', | ||
| "process.version + ' ' + process.platform + ' ' + process.arch", | ||
| ]); | ||
| const [version, platform, arch] = stdout.trim().split(' '); | ||
| return { version, platform, arch }; | ||
| } | ||
|
|
||
| /** | ||
| * Guard a custom base Node binary against multi-target / wrong-platform / | ||
| * version-skew footguns. | ||
| * | ||
| * A supplied binary (via `--sea-node-path` / `seaNodePath` / `PKG_NODE_PATH`, or | ||
| * `useLocalNode`) is returned by {@link getNodejsExecutable} for *every* target, | ||
| * so a multi-target run would bake that one binary into outputs for other | ||
| * platforms/arches — silently producing broken artifacts. We: | ||
| * | ||
| * 1. Reject if the requested targets span more than one distinct | ||
| * `platform`+`arch` — one binary can't be several mutually-exclusive things | ||
| * at once (incl. linux vs alpine vs linuxstatic, which we can't tell apart | ||
| * from the binary but the user clearly can't have meant simultaneously). | ||
| * 2. Run the binary and reject if what it reports (`process.platform` / | ||
| * `process.arch`) disagrees with that single target (e.g. a macOS binary | ||
| * for a `linux` target, or x64 for `arm64`). We exec it for its version | ||
| * regardless (here and in {@link resolveTargetNodeVersion}), so asking it | ||
| * everything at once is simpler and stricter than sniffing its header — at | ||
| * the cost of requiring the base to be runnable on the build host, which is | ||
| * already true in practice. The glibc/musl/static flavor isn't reported, so | ||
| * that sub-distinction stays the user's responsibility. | ||
| * 3. Verify the binary's major matches the requested `nodeRange` | ||
| * (`assertSingleTargetMajor` only compares the targets to each other). | ||
| */ | ||
| async function assertCustomBaseNodeTarget( | ||
|
Member
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. [Nit] · Types
|
||
| targets: (NodeTarget & Partial<Target>)[], | ||
| opts: GetNodejsExecutableOptions, | ||
| ): Promise<void> { | ||
| const customNode = resolveCustomBaseNode(opts); | ||
| if (!customNode && !opts.useLocalNode) return; | ||
| const binPath = customNode ?? process.execPath; | ||
|
|
||
| // 1. A single binary maps to exactly one platform+arch. Key on the raw target | ||
| // suffix strings so linux / alpine / linuxstatic stay distinct (they collapse | ||
| // under getNodeOs, but they're mutually exclusive runtimes). | ||
| const combos = new Map<string, { platform: string; arch: string }>(); | ||
| for (const t of targets) { | ||
| const platform = String(t.platform); | ||
| const arch = String(t.arch); | ||
| combos.set(`${platform}-${arch}`, { platform, arch }); | ||
| } | ||
| if (combos.size > 1) { | ||
| throw wasReported( | ||
| `A custom base Node binary applies to a single platform/arch, but the ` + | ||
| `requested targets span ${combos.size}: ${[...combos.keys()].join(', ')}. ` + | ||
| `One binary can't be all of them — run pkg once per target with a ` + | ||
| `matching binary.`, | ||
| ); | ||
| } | ||
| const { platform, arch } = [...combos.values()][0]; | ||
|
|
||
| // 2. Ask the binary what it actually is and reject any disagreement. | ||
| const node = await probeNode(binPath); | ||
|
|
||
| const expectedPlatform = PROCESS_PLATFORM[platform] ?? platform; | ||
| if (node.platform !== expectedPlatform) { | ||
| throw wasReported( | ||
| `Custom base Node binary is for "${node.platform}", but target ` + | ||
| `"${platform}" needs "${expectedPlatform}".`, | ||
| ); | ||
| } | ||
| if (node.arch !== arch) { | ||
|
Member
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. [Major] · Correctness Same defect as the platform map, but arch has no mapping at all. pkg's Why: blocks a valid 32-bit-ARM custom-runtime build with a confusing error. Fix: add an arch map ( |
||
| throw wasReported( | ||
| `Custom base Node binary is ${node.arch}, but target arch is "${arch}". ` + | ||
| `The binary must match the target's architecture.`, | ||
| ); | ||
| } | ||
|
|
||
| // 3. Major version match (assertSingleTargetMajor only compares the targets | ||
| // to each other, never to the supplied binary). | ||
| const targetMajor = parseInt(targets[0].nodeRange.replace('node', ''), 10); | ||
| if (Number.isNaN(targetMajor)) return; // 'latest' / unparseable: nothing to compare | ||
| const binMajor = parseInt(node.version.replace(/^v/, ''), 10); | ||
| if (binMajor !== targetMajor) { | ||
| throw wasReported( | ||
| `Custom base Node binary is ${node.version} (major ${binMajor}), but ` + | ||
| `target "${targets[0].nodeRange}" requests Node ${targetMajor}. The ` + | ||
| `binary's major version must match the target.`, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Resolve the concrete Node.js version (e.g. `v22.22.2`) pkg will use | ||
| * for `target` — mirrors the version selection done inside | ||
|
|
@@ -341,11 +468,11 @@ async function resolveTargetNodeVersion( | |
| opts: GetNodejsExecutableOptions, | ||
| ): Promise<NodeVersion> { | ||
| if (opts.useLocalNode) return process.version as NodeVersion; | ||
| if (opts.nodePath) { | ||
| const customNode = resolveCustomBaseNode(opts); | ||
| if (customNode) { | ||
| // A user-supplied binary can be any version — don't assume it | ||
| // matches the host. Ask it directly. | ||
| const { stdout } = await execFileAsync(opts.nodePath, ['--version']); | ||
| return stdout.trim() as NodeVersion; | ||
| return (await probeNode(customNode)).version as NodeVersion; | ||
|
Member
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. [FYI] · Performance
|
||
| } | ||
| const os = getNodeOs(target.platform); | ||
| const arch = getNodeArch(target.arch); | ||
|
|
@@ -357,15 +484,16 @@ async function getNodejsExecutable( | |
| target: NodeTarget, | ||
| opts: GetNodejsExecutableOptions, | ||
| ): Promise<string> { | ||
| if (opts.nodePath) { | ||
| // check if the nodePath exists | ||
| if (!(await exists(opts.nodePath))) { | ||
| const customNode = resolveCustomBaseNode(opts); | ||
| if (customNode) { | ||
| // check if the custom base binary exists | ||
| if (!(await exists(customNode))) { | ||
| throw new Error( | ||
| `Priovided node executable path "${opts.nodePath}" does not exist`, | ||
| `Provided node executable path "${customNode}" does not exist`, | ||
| ); | ||
| } | ||
|
|
||
| return opts.nodePath; | ||
| return customNode; | ||
| } | ||
|
|
||
| if (opts.useLocalNode) { | ||
|
|
@@ -765,6 +893,7 @@ export async function seaEnhanced( | |
| } | ||
|
|
||
| assertSingleTargetMajor(opts.targets); | ||
| await assertCustomBaseNodeTarget(opts.targets, opts); | ||
|
|
||
| const minTargetMajor = resolveMinTargetMajor(opts.targets); | ||
| if (minTargetMajor < 22) { | ||
|
|
@@ -883,6 +1012,7 @@ export async function seaEnhanced( | |
| export default async function sea(entryPoint: string, opts: SeaOptions) { | ||
| assertHostSeaNodeVersion(); | ||
| assertSingleTargetMajor(opts.targets); | ||
| await assertCustomBaseNodeTarget(opts.targets, opts); | ||
|
|
||
| entryPoint = resolve(process.cwd(), entryPoint); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| 'use strict'; | ||
|
|
||
| // Packaged into a SEA on top of a custom base Node binary (the host's own node, | ||
| // supplied via --sea-node-path or PKG_NODE_PATH). Output is asserted by main.js; | ||
| // process.pkg confirms the enhanced SEA bootstrap is active. | ||
| console.log('custom-node SEA OK:' + (process.pkg != null)); |
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.
[Nit] · Consistency
PKG_NODE_PATHis passed throughresolve()(line 345) butopts.nodePath(the--sea-node-pathflag) is returned verbatim — so a relative--sea-node-pathis left un-normalized. Harmless today (exec/exists resolve against cwd) but inconsistent.