Repository navigation
Conversation
The node-napi-tests harness already bypasses node-gyp on POSIX by invoking the platform compiler directly once the node headers are cached. On Windows it returned false unconditionally, so every one of the 55 do.test.ts wrappers paid the full bun x + python gyp + MSBuild cost (~4-5s warm, 30-40s for whichever file ran first in a shard and had to download headers). This teaches findNodeHeaders about the %LOCALAPPDATA%\node-gyp\Cache layout (including the per-arch node.lib), vendors node-gyp's win_delay_load_hook.cc, and adds a clang-cl command line that produces the same build\Debug\<target>.node that MSBuild would. clang-cl is on PATH on every Windows CI and dev image via scoop llvm and auto-detects the MSVC toolchain, so no Developer Command Prompt is required. Any failure (headers not yet cached, clang-cl missing, compile error) falls back to node-gyp exactly as before. Measured on a Windows 2019 x64 machine with warm headers: 7_factory_wrap/do.test.ts 4.06s -> 0.48s full node-napi-tests suite 377.7s -> 22.1s (123 pass, 0 fail) Linux and macOS are unchanged; the POSIX path is only pulled out into posixCompileArgs so tryBuildFast can branch on platform.
WalkthroughChangesThe N-API test harness now directly compiles addon targets on POSIX and Windows, using platform-specific Node header and compiler configuration. Windows builds include a delay-load hook for renamed host executables, temporary output promotion, failure logging, fallback, and cleanup. ChangesCross-platform fast compilation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Arrived at the same fix independently from a parallel handoff (6_object_wrap, same build 83901). Confirming the approach works end to end on both Windows arches:
Two small notes from comparing against node-gyp's own MSBuild output (
My branch ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit: the /MDd vs /MTd difference raised in the thread was examined — the debug UCRT DLLs ship with the VS toolchain present on every Windows CI image, and a load failure would fall through to the node-gyp path anyway. Also confirmed the per-target .{target}.{pid}.tmp scratch dir avoids the /Fo object-clobber for multi-target gyps (test_cannot_run_js, test_reference_by_node_api_version), and that the POSIX command line is byte-identical after the extraction into posixCompileArgs.
Extended reasoning...
Test-infrastructure-only change with a hard fallback to node-gyp on any failure, so the blast radius is bounded. One low-impact nit surfaced (scratch dir left behind on Windows compile failure). robobun's /MTd suggestion is still open in the thread, so leaving the final call to a human rather than approving over it.
There was a problem hiding this comment.
LGTM — both review notes (scratch-dir cleanup in finally, /MTd to match node-gyp) are addressed in 3e3d604.
What was reviewed:
- POSIX path is byte-identical after the extract-to-
posixCompileArgsrefactor;findNodeHeaderschange is additive for win32 only. - Fail-safe shape holds: missing headers/
node.lib/clang-cland any nonzero exit all returnfalse→buildWithNodeGyp. headers.lib!is safe — win32 branch offindNodeHeadersonly returns whennode.libexists.- Mixed C/C++ targets:
win_delay_load_hook.cccompiles by extension under clang-cl even when/std:c++20isn't passed for a pure-C target.
Extended reasoning...
Overview
Test-harness-only change extending the existing tryBuildFast direct-compile optimization (added in #33622 for POSIX) to Windows. Touches test/napi/node-napi-tests/harness.ts and adds a vendored win_delay_load_hook.cc copied verbatim from node-gyp (MIT). No runtime, native, or production code is affected.
The POSIX command line was lifted unchanged into posixCompileArgs; findNodeHeaders gained a %LOCALAPPDATA%\node-gyp\Cache probe and now also returns the <arch>/node.lib import library on Windows; windowsCompileArgs drives clang-cl with /DELAYLOAD:node.exe + the vendored hook; tryBuildFast branches on platform and cleans the per-target scratch dir in a finally.
Security risks
None. This is a test build harness that shells out to a local compiler with paths derived from checked-in binding.gyp files (which parseSimpleBindingGyp already strictly whitelists to target_name/sources/defines). No network, no user input, no runtime surface.
Level of scrutiny
Low-to-medium. It's test infrastructure with a hard fallback: every failure mode (headers not cached, clang-cl not on PATH, compile error, spawn/rename throw) returns false and drops through to the pre-existing buildWithNodeGyp, so the worst case is the status quo. The blast radius of a mistake is Windows CI test wall-clock, not correctness — the addons are still exercised by the same do.test.ts files either way.
Other factors
- My prior review's only finding (scratch dir leaked on the failure path) was fixed in commit 3e3d604 by moving
rm(tmp, {recursive, force})into afinally;force: truemakes it safe whenmkdirnever ran. - robobun's independent verification on windows-x64 (55 files, 123 pass/0 fail) and windows-aarch64 (
arm64/node.libpicked up), plus their/MTdnote, are both reflected in the current diff. Their per-target/Foclobber concern was already handled by the per-targettmpdir (they self-corrected). - Checked that
headers.lib!cannot be undefined on the win32 branch:findNodeHeaderson win32 only returns non-null afterexistsSync(lib). - Checked the mixed-language case: for pure-C targets the hook
.ccis still appended; clang-cl selects language by extension so it compiles as C++ without needing the conditionally-added/std:c++20. - stdout is now piped and drained alongside stderr (clang-cl writes diagnostics to stdout), satisfying the concurrent-drain rule.
|
Self-review surfaced two more things, both in 8e83bbc:
Re-ran the full suite on Windows x64 with warm headers: 123 pass / 25 todo / 0 fail in 27.0s, with a PDB next to every |
There was a problem hiding this comment.
LGTM — the earlier scratch-dir cleanup nit and the /MTd note are both addressed, and the POSIX path is byte-for-byte unchanged after the extraction into posixCompileArgs.
What was reviewed:
findNodeHeadersreturn-type change:headers.lib!is safe because the win32 branch only returns non-null whenexistsSync(lib)passes.- Fallback coverage: every new failure exit (
clang-clmissing, headers/lib missing, nonzero exit, thrown error) returnsfalseand reachesbuildWithNodeGyp, so worst case on a misconfigured Windows box is the pre-PR behavior. - stdout switched from
ignoretopipeand drained concurrently with stderr/exited — needed since cl-style drivers write diagnostics to stdout, and avoids the pipe-buffer deadlock. - Per-target
/Foscratch dir + pid handles the parallel-target.objclobber;/PDBwrites tooutDirso it survives thefinallyrm.
Extended reasoning...
Overview
Extends the existing tryBuildFast node-gyp bypass in test/napi/node-napi-tests/harness.ts to Windows: probes %LOCALAPPDATA%\node-gyp\Cache\<ver> for headers plus the arch-specific node.lib, then drives clang-cl directly with /link /DLL /DELAYLOAD:node.exe against a vendored copy of node-gyp's win_delay_load_hook.cc. The pre-existing POSIX command line is lifted verbatim into posixCompileArgs — I diffed it flag-for-flag against the removed inline array and it is identical. Two files touched, both under test/.
Security risks
None. This is test-harness plumbing that shells out to a local compiler with paths derived from checked-in binding.gyp files that parseSimpleBindingGyp already restricts to {target_name, sources, defines}-only shapes. No runtime, network, or user-facing surface.
Level of scrutiny
Low-to-moderate. Nothing here ships in the binary; the only risk is CI regression, and every failure path returns false to fall through to the exact node-gyp invocation that was the sole Windows path before this PR. The author and a second robobun run independently verified 123 pass / 0 fail across all 55 files on both Windows arches, and confirmed the Linux/ASAN suite is unchanged.
Other factors
My prior review flagged the scratch-dir leak on the failure path; 3e3d604 moved the rm into a finally and 8e83bbc added .catch(() => {}) so a transient EBUSY on cleanup can't override the try/catch return value. robobun's /MTd-vs-/MDd note was also applied. Both threads are resolved. The vendored win_delay_load_hook.cc is the standard node-gyp hook (MIT), guarded by #ifdef _MSC_VER which clang-cl defines, and HOST_BINARY is passed as "node.exe" to match node-gyp's own define.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/napi/node-napi-tests/harness.ts (1)
148-213: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding lightweight unit tests for the new pure builder/discovery functions.
findNodeHeaders,posixCompileArgs, andwindowsCompileArgsare deterministic, side-effect-light functions whose output (arg ordering, flag correctness, candidate-directory logic) directly drives whether the fast path silently falls back to node-gyp. A small test asserting expected arg arrays for representative inputs (C vs C++ target, with/withoutdefines) would guard against future regressions in this path without needing a real compiler.As per coding guidelines, "Every behavioral change must include an automated test in the same PR."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/napi/node-napi-tests/harness.ts` around lines 148 - 213, Add focused unit tests for the pure fast-build helpers findNodeHeaders, posixCompileArgs, and windowsCompileArgs. Cover representative C and C++ targets, targets with and without defines, expected argument ordering and flags, and candidate-directory selection; keep tests deterministic without invoking a compiler or node-gyp.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/napi/node-napi-tests/harness.ts`:
- Around line 114-146: Update windowsCompileArgs to emit the PDB into the
per-target scratch directory instead of the shared out directory, then extend
the successful artifact promotion flow to move the PDB alongside the .node file.
Ensure failed builds leave no PDB in outDir and rely on existing scratch
cleanup.
---
Outside diff comments:
In `@test/napi/node-napi-tests/harness.ts`:
- Around line 148-213: Add focused unit tests for the pure fast-build helpers
findNodeHeaders, posixCompileArgs, and windowsCompileArgs. Cover representative
C and C++ targets, targets with and without defines, expected argument ordering
and flags, and candidate-directory selection; keep tests deterministic without
invoking a compiler or node-gyp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8ae78854-829e-429f-af1d-bf1afd701117
📒 Files selected for processing (2)
test/napi/node-napi-tests/harness.tstest/napi/node-napi-tests/win_delay_load_hook.cc
There was a problem hiding this comment.
LGTM — test-harness-only change with a clean fallback to node-gyp on every failure path; all prior feedback (scratch-dir finally, /MTd, /Z7+PDB, PDB promote-from-scratch) is addressed.
What was reviewed:
- POSIX command line is byte-identical after the extract to
posixCompileArgs; only thestdout: "pipe"change touches non-Windows behavior, and both pipes are drained concurrently with.exited. headers.lib!is safe:findNodeHeaderson win32 only returns non-null when<arch>/node.libexists;process.arch(x64/arm64) matches node-gyp's cache layout, confirmed on both arches in-thread.- Per-target
.{name}.{pid}.tmpscratch dir +/Fotrailing separator avoids the parallel-target .obj clobber;finallycleans it with errors swallowed so a transientEBUSYcan't mask the real return value. win_delay_load_hook.ccis the node-gyp MIT copy;HOST_BINARYis supplied via/Dso_stricmpsees the string literal.
Extended reasoning...
Overview
Extends the existing tryBuildFast direct-compile path in test/napi/node-napi-tests/harness.ts to Windows. Two files touched: the harness (refactor POSIX args into posixCompileArgs, add windowsCompileArgs driving clang-cl, teach findNodeHeaders about %LOCALAPPDATA%\node-gyp\Cache and the <arch>/node.lib import library, add per-target scratch dir + promote-on-success + finally cleanup) and a vendored win_delay_load_hook.cc copied from node-gyp so /DELAYLOAD:node.exe resolves to the host process. No production code is touched.
Security risks
None. This is test-only infrastructure that spawns a local compiler over in-tree source files. No network, no untrusted input, no auth/crypto surface. The vendored hook is a well-known MIT-licensed node-gyp file.
Level of scrutiny
Low-to-moderate. The change is confined to a test harness and every new code path degrades to return false → buildWithNodeGyp, so the worst case for a mistake in the Windows args is "fast path doesn't fire, tests take as long as they do today." The POSIX path is unchanged after the mechanical extraction (verified line-by-line against the removed inline array). The one non-Windows behavioral change is stdout: "ignore" → "pipe", which is now drained concurrently alongside stderr and .exited, so no deadlock risk per the subprocess-drain rule.
Other factors
- All four review threads (my scratch-dir-leak note, robobun's
/MTdand per-target obj-dir observations, the self-review/Z7+PDB addition, and CodeRabbit's PDB-in-scratch note) are resolved and reflected in the final diff. - Verified in-thread on windows-x64 (123 pass / 25 todo / 0 fail, 22-27s vs 377s), windows-aarch64 (
arm64/node.libpicked up), and Linux debug+ASAN (unchanged 124 pass / 24 todo). - The
headers.lib!non-null assertion is guarded: on win32findNodeHeadersonly returns non-null whenexistsSync(lib)is true, and the win32 branch is the only one that reads.lib. - If the PDB
renameSyncwere ever to throw (e.g. linker didn't emit one), thecatchreturnsfalseand node-gyp runs — acceptable degradation, not a test failure.
|
Closing this PR because it has been inactive for more than 90 days. |
What does this PR do?
Speeds up the 55
do.test.tswrappers undertest/napi/node-napi-tests/on Windows by letting the existingtryBuildFastpath handle that platform too.#33622 added a fast path that bypasses node-gyp on POSIX by invoking the platform compiler directly once the node headers are cached. On Windows the function returned
falseimmediately, so every addon still went throughbun x node-gyp@11 configure build(python gyp + MSBuild), which is ~4-5s per directory once warm and 30-40s for whichever file runs first in a shard and has to install node-gyp and download headers. The 38s7_factory_wrap/do.test.tsobserved onwindows 2019 x64in build 83901 is that cold cost; the next file in the same shard took 4.55s.Change:
findNodeHeadersnow also probes%LOCALAPPDATA%\node-gyp\Cache\<version>and, on Windows, additionally requires the matching<arch>\node.libimport library.win_delay_load_hook.ccis vendored next to the harness (copied from node-gyp, MIT). It is the same hook node-gyp links into every addon so/DELAYLOAD:node.exeresolves to the host process.windowsCompileArgsdrivesclang-clto producebuild\Debug\<target>.nodedirectly: headers via/I, the gyp defines, the sources plus the delay-load hook, then/link /DLLagainstnode.libwith/DELAYLOAD:node.exeanddelayimp.lib.clang-clis onPATHon every Windows CI/dev image via the scoop LLVM install and auto-detects the MSVC toolchain, so it works outside a Developer Command Prompt.posixCompileArgssotryBuildFastcan branch on platform.clang-clmissing, compile error) returnsfalseand falls back tobuildWithNodeGypexactly as before.How did you verify your code works?
Timed on a Windows Server 2019 x64 machine, system Bun, node headers already cached:
7_factory_wrap/do.test.tsnode-napi-tests/suite (55 files)Both runs: 123 pass, 25 todo, 0 fail.
Cold start (no header cache) is unchanged: the fast path sees no headers, returns
false, node-gyp downloads them and builds the first addon; every subsequent file in that shard then takes the fast path.On Linux (debug+ASAN, headers cached) the full suite is still 124 pass / 24 todo / 0 fail and the POSIX fast path still fires for every addon;
7_factory_wraptiming is unchanged since the POSIX path already existed.no test proof · iteration 0 · docs-only change; test-proof not applicable