fix(binding): drop node-PATH dependency in binding.gyp (#153)#154
Merged
Conversation
Replace 5 <!(node -p "...") shell-outs with a Python helper invoked via
<(python). node-gyp injects an absolute python path into build/config.gypi,
so the substitution works regardless of whether `node` is in PATH.
Root cause: npm spawns install scripts in a fresh shell (Git Bash on
Windows) whose PATH does not include mise/aube/pnpm-managed node.exe.
When gyp evaluated `<!(node -p "process.env.USE_GLOBAL || ...")` inside
that shell, `node` could not be resolved and gyp aborted with
`'node' is not recognized as an internal or external command`.
The bridge preserves the existing env-var API (USE_GLOBAL, RUNTIME_LINK,
ENABLE_THREAD_SUPPORT, npm_config_python) by reading them from Python's
os.environ instead of process.env, and resolves node-addon-api via
filesystem walk (returns the same `node_api.gyp:nothing` reference the
deprecated `require('node-addon-api').gyp` API produced).
…llback Address codex pre-PR review findings on the #153 fix: - M: quote <(python) and <(module_root_dir)/scripts/... in every <!() / <!@() shell-out so a Python interpreter at a path containing spaces (e.g. C:\Program Files\Python311\python.exe — the literal value seen in the issue logs) is not split by the shell into multiple argv tokens. - L: define `python%` via an OS-conditional default (python on Windows, python3 elsewhere) so the binding still parses on legacy node-gyp versions (<10) whose generated config.gypi did not emit a `python` variable. node-gyp 10+ (shipped with node>=20) overrides this fallback with the absolute path it resolves itself. The `action: [...]` invocations elsewhere in the file already use gyp's array form, which bypasses the shell, so no quoting is needed for those.
Codex re-review of commit e30935b flagged L: the `conditions: [...python%...]` block inside `variables` does not resolve before the sibling `<!()` calls expand `<(python)` in gyp's variable-resolution pass order. So on legacy node-gyp (<10) where config.gypi lacks `python`, the fallback was never reached — gyp would fail at parse time with "Undefined variable python" before merging the conditional default. Since the project pins `engines.node >= 22.0.0` and node 22+ ships with npm 10+ (bundling node-gyp 10+, which always emits `python` into config.gypi), the fallback is unreachable in supported configurations. Drop it and document the requirement in the comment.
There was a problem hiding this comment.
Pull request overview
This PR fixes Windows install failures caused by binding.gyp shelling out to node -p ... when node.exe is not on the PATH inherited by node-gyp’s subprocess (notably under Git Bash with mise/pnpm/aube). It replaces those Node-based evaluations with a Python helper invoked via gyp’s <(python) variable so configuration can be resolved without requiring node on PATH.
Changes:
- Added
scripts/binding_config.pyto resolve env-var defaults and locatenode-addon-apipaths without invokingnode. - Updated
binding.gypto replace<!(node -p ...)calls with<!( "<(python)" ... binding_config.py ...)and adjusted source discovery invocation to use<(python).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
scripts/binding_config.py |
New Python helper to compute binding.gyp variables and resolve node-addon-api include/gyp paths without Node shell-outs. |
binding.gyp |
Switches variable resolution and node-addon-api discovery from node -p to the Python helper / <(python) to remove PATH dependency on Windows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add scripts/binding_config.py to package.json files array so it ships with npm tarball, fixing install-time resolution failures on consumer machines where binding.gyp invokes the helper - Remove unused py3() function and COMMANDS entry; binding.gyp resolves the python interpreter via gyp-injected <(python) variable, never calling binding_config.py py3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #153 —
npm i -g node-liblzma(and the pnpm / aube variants) fails on Windows with'node' is not recognized as an internal or external commandbecausebinding.gypinvoked<!(node -p "...")from inside gyp's shell-out. npm spawns install scripts in Git Bash on Windows, whose inheritedPATHdoes not include the mise/aube/pnpm-managednode.exe.The fix replaces every
<!(node -p ...)call with<!("<(python)" "<(module_root_dir)/scripts/binding_config.py" ...)— a Python helper invoked via thepythonvariable that node-gyp ≥10 always emits intobuild/config.gypi. NonodePATH dependency remains inbinding.gyp.Why Python
<(python), so substitution works regardless of shell PATH.engines.node >= 22.0.0pins users to npm ≥ 10 → node-gyp ≥ 10, which is guaranteed to provide<(python).Behavior parity
The Python helper preserves the existing env-var API:
USE_GLOBALtruefalseRUNTIME_LINKsharedstaticENABLE_THREAD_SUPPORTyesyesnpm_config_pythonpython3pythonnode-addon-apiresolution walks upward throughnode_modules(handles pnpm-isolated layouts) and returns the samenode_api.gyp:nothingreference the deprecatedrequire('node-addon-api').gypAPI produced — preserving the existing dependency declaration semantics.Quoting
Every
<(python)and<(module_root_dir)/scripts/...reference inside a<!()/<!@()shell command is wrapped in double quotes so a Python interpreter at a path containing spaces (e.g.C:\Program Files\Python311\python.exe— the literal value in the issue logs) is not split into multiple argv tokens by cmd.exe / sh. Theaction: [...]invocations elsewhere already use gyp's array form (bypasses shell) and are unchanged.Local verification
pnpm exec node-gyp configure→gyp info okpnpm exec node-gyp build→ emitsRelease/node_lzma.node<(python)resolves to/bin/python3perbuild/config.gypiafter configureUSE_GLOBAL=false RUNTIME_LINK=static node-gyp configureactivates the CMake vendored target chainPre-PR review
Two rounds of codex xhigh review:
Test plan
windows-latest(Node 24) passes — exercisesnode-gyp rebuildunder Git Bash on Windows runnersubuntu-latest+macos-latestcontinues to pass (no regression on system-liblzma path)aube a -g node-liblzma --allow-build=node-liblzmaon a Windows host with mise — should no longer error on'node' is not recognized. (Note: the separatenode_api.gyp not founderror under aube's virtual store layout — visible in the issue logs — is a downstream aube path-resolution bug and is not in scope here.)Closes #153.