fix: put node-gyp on PATH for dependency build scripts - #10554
Conversation
The pnpm engine spawns dependency lifecycle scripts with the PATH of the bit process plus the relevant node_modules/.bin dirs, and ships no node-gyp of its own. A native package that shells out to `node-gyp rebuild` — node-gyp-build falling back, node-pre-gyp, or a plain `"install": "node-gyp rebuild"` — therefore fails with `spawn node-gyp ENOENT`, e.g. bufferutil on darwin-arm64, which has no prebuild. Depend on node-gyp and expose it the way npm does: a wrapper script in a cache directory that is put on PATH before the install runs. The directory is appended rather than prepended, so a node-gyp the user installed themselves still wins, and it is keyed by a hash of the node and node-gyp paths baked into the wrapper, so an upgrade gets a fresh directory instead of rewriting a script a concurrent install may be executing. node-gyp has to be declared in workspace.jsonc for the aspect, since nothing imports it and dependency detection would never pick it up. 11.x rather than 13.x: node-gyp 13 requires Node ^22.22.2 and bvm ships 22.22.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR Summary by QodoFix pnpm dependency builds by adding a node-gyp shim to PATH
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. PATH override not portable
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Two things would have let the e2e test pass without the fix, so it guards against both. `npm run` puts the repo's node_modules/.bin — which now carries a node-gyp bin link — on PATH, and dependency build scripts inherit it, so the install runs with a PATH stripped of every directory holding a node-gyp. And the pnpm store's side-effects cache reproduces the build output of a package built by an earlier install without ever running node-gyp, so the workspace gets a store of its own. Also address review feedback on the wrapper itself: report a setup failure at warning level rather than only in the debug log, drop the memoized failure so a later install retries, and write the wrapper to a fixed directory that is atomically replaced when it goes stale, instead of a new hash-keyed directory per Bit or Node upgrade. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed an e2e test plus the two review fixes. e2e test —
|
| bit | result |
|---|---|
| released 2.0.59 (no fix) | ✗ @pnpm.e2e/has-binding-gyp@1.0.0 install: node-gyp rebuild exited with exit status: 127 |
| this branch | ✓ passing |
Review comments
1. Cached shim setup failure — fixed. The failure is no longer memoized, so each install retries, and it is reported with logger.consoleWarning (plus the full error to the debug log) instead of a debug-only line. Not made fatal: a missing wrapper only matters for a dependency that actually builds with node-gyp, and it may still find one elsewhere on PATH.
2. Stale shim dirs accumulate — fixed at the source rather than with pruning. The wrapper now lives in one fixed <cache>/node-gyp-bin, rewritten only when its content is stale. The write is still temp-file-then-rename, which is what made the hash-keyed directory unnecessary: the rename replaces the directory entry atomically and a script already executing the old wrapper keeps reading the inode it opened, so there is nothing to race and nothing to accumulate.
|
Code review by qodo was updated up to the latest commit ea9f3b9 |
A dry run is documented as skipping the installation, so it has no business writing the wrapper or touching PATH. Move the setup inside the guard, and into `rebuild`, which is reachable without an install of its own. Restore the wrapper directory keyed by the Node and node-gyp paths it hardcodes. A single shared directory made two Bit installs overwrite each other, pointing a concurrent install at the wrong node-gyp — and it rewrote the wrapper on every install when two Bit versions are used side by side, which is the normal state of this repo (`bit` and `bbit`). What accumulates instead is two ~100-byte files per distinct pair. The unit test now removes the wrapper it wrote to the real cache. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All three addressed in 40f74a4. 1. Dry-run has side effects — fixed. 2. Global shim race — fixed, by reverting the change I made for the previous round's "stale shim dirs accumulate" comment. The two are in direct conflict: the wrappers hardcode Going back to the keyed directory (their option 2) is the right trade. The race isn't hypothetical — it's this repo's normal state, where 3. Tests write to user cache — the test now removes the directory it wrote. Not redirected via Re-verified after the changes — unit spec 3/3 (and leaves the cache as it found it), e2e passing on this branch and still failing with exit 127 against released 2.0.59. |
|
Code review by qodo was updated up to the latest commit 40f74a4 |
Read the wrapper and handle its absence, rather than testing for it first: another Bit process replacing it can remove the file between the two calls, and the resulting ENOENT would skip the whole setup. Delete PATH in the spec's after-hook when it was unset to begin with, so a later spec in the same process does not see the string "undefined". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both open threads addressed in aaf8797. 1. Path restore not symmetric — fixed. The after-hook now 2. Fragile shim file read — fixed, and it removes the check rather than guarding it: Re-verified: unit spec 3/3, e2e passing. |
| helper.command.install('@pnpm.e2e/has-binding-gyp', undefined, undefined, { | ||
| envVariables: { PATH: pathWithoutNodeGyp() }, | ||
| }); |
There was a problem hiding this comment.
1. Path override not portable 🐞 Bug ☼ Reliability
The new node-gyp e2e test only reads/overrides PATH, but CommandHelper.runCmd() builds the child-process env by spreading process.env into a plain object and does not normalize/remove alternate-cased path keys, so an existing Path entry (common on Windows) may remain unfiltered alongside the new PATH. This can make the test’s “no node-gyp on PATH” guarantee unreliable on Windows, potentially hiding regressions.
Agent Prompt
### Issue description
The new e2e test tries to remove all existing `node-gyp` locations from the PATH before running `bit install`, but it only reads `process.env.PATH` and only passes `envVariables: { PATH: ... }`. On Windows, the effective path variable may be represented as `Path` (different casing), and the helper builds an `env` plain object from `process.env` + overrides without normalizing/deleting alternate casings.
### Issue Context
This can cause the test to still run with an unfiltered PATH (e.g., `Path` preserved) even though `PATH` was overridden, making the test nondeterministic on Windows and potentially allowing a false pass when Bit fails to supply node-gyp.
### Fix Focus Areas
- e2e/harmony/dependencies/node-gyp.e2e.ts[14-18]
- e2e/harmony/dependencies/node-gyp.e2e.ts[46-48]
### Suggested fix
1. Update `pathWithoutNodeGyp()` to read from `process.env.PATH || process.env.Path || process.env.path || ''`.
2. When invoking `helper.command.install(...)`, set all common casings to the same filtered value, e.g.:
- `envVariables: { PATH: filtered, Path: filtered, path: filtered }`
This keeps the fix local to the new test and makes it robust across platforms.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit aaf8797 |
The command helper spreads process.env into a plain object, which drops Node's case-insensitive env lookup. A `PATH` override would then sit next to the `Path` Windows actually uses rather than replacing it, and the test would install with node-gyp still reachable — passing whether or not Bit supplies one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed in e0b54ee. Path override not portable — real: Fixed slightly differently than suggested: rather than setting Re-verified both directions after the change — passing on this branch, still failing with exit 127 against released 2.0.59. |
|
Code review by qodo was updated up to the latest commit e0b54ee |
2 participants
Summary
bit installfails on any dependency that shells out tonode-gyp:The pnpm engine spawns dependency lifecycle scripts with the PATH of the bit process plus the relevant
node_modules/.bindirs, and ships nonode-gypof its own (pacquet'snode_gyp_bin/node_gyp_pathlifecycle options exist, but every caller passesNone). Before #10508 Bit inherited anode-gypwrapper from@pnpm/npm-lifecycle, which depends on node-gyp and prepends its wrapper dir to PATH for every script it spawns — the old lockfile hadnode-gyp@11.2.0through it, the new one has none.So anything that falls back to
node-gyp rebuild—node-gyp-buildwith no matching prebuild,node-pre-gyp, or a plain"install": "node-gyp rebuild"— now dies withspawn node-gyp ENOENT. It shows up on platforms a package has no prebuild for:bufferutil@4.0.3ships no darwin-arm64 prebuild, so every Apple Silicon install of it fails.Changes
node-gyp-bin.ts(new) — writes an npm-stylenode-gypwrapper into<bit-cache>/node-gyp-bin/<key>and appends it toprocess.env.PATH. Appended rather than prepended, so a node-gyp the user installed themselves still wins. The directory is keyed by a hash of thenodeandnode-gyp.jspaths baked into the wrapper, so a Bit or Node upgrade gets a fresh directory instead of rewriting a script a concurrent install may be executing; wrappers are written temp-file-then-rename.process.envis the only lever available here —InstallOptionsexposes noextraBinPaths(onlyPackOptionsdoes).lynx.ts— call it once ininstall(), which covers bothnodeApi.installand the returnedrebuild.workspace.jsonc—node-gyp: 11.5.0in the root policy and in a new variant forscopes/dependencies/pnpm. The variant entry is required: nothing imports node-gyp, so dependency detection would never add it to@teambit/pnpm's package.json. 11.x rather than 13.x because node-gyp 13 requires Node^22.22.2and bvm ships 22.22.0.The complete fix belongs upstream —
pacquetalready has thenode_gyp_binplumbing, it just has nothing to point it at — but Bit needs to ship anode-gypeither way.Test plan
npm run lintbit test scopes/dependencies/pnpm/node-gyp-bin.spec.ts— 3/3 passingbufferutil@1.2.1("install": "node-gyp rebuild"): releasedbit→sh: line 1: node-gyp: command not found, exit 127. Local build → node-gyp runs (node-gyp -v v11.5.0, invoked as<node> <…>/node-gyp.js); the compile then fails on its own merits, since that 2016 nan addon does not build against Node 22.bufferutil@4.0.9andnpm_config_build_from_source=trueto force the gyp path:bit installcompletes green and produces a realbuild/Release/bufferutil.node.🤖 Generated with Claude Code