fix(arborist): honor gypfile:false on lockfile-driven installs - #9859
fix(arborist): honor gypfile:false on lockfile-driven installs#9859lazerg wants to merge 2 commits into
Conversation
| if ( | ||
| !hasExplicitGypGate && | ||
| pkg.gypfile !== false && | ||
| await isNodeGypPackage(node.path).catch(() => false) |
There was a problem hiding this comment.
getInstallScripts() also runs during the strict allow-scripts preflight, before reify replaces the existing node_modules entry. At that point, node.path can refer to a different version.
For example, suppose the installed version has binding.gyp and gypfile: false, while the locked version has binding.gyp without the opt-out. This reads false from the installed version, so strict preflight passes. Rebuild later sees the new package and synthesizes node-gyp rebuild.
Can we check that the package on disk matches the node's name and version before trusting this fallback?
There was a problem hiding this comment.
Good catch, that is real. The preflight walks the ideal tree, so node.path still points at whatever version is installed right now.
hasGypfileOptOut now takes the node's package object instead of only the gypfile value, and it trusts the disk read only when the package.json at that path carries the same name and version as the node. If either one differs, or the node has no name or version to compare against, the opt-out counts as absent and node-gyp rebuild is enumerated. That is what npm did before this fallback existed, so the preflight can no longer come out quieter than the later rebuild.
Added three cases in workspaces/arborist/test/install-scripts.js: a version mismatch on disk, a name mismatch, and a node with nothing to match on.
JamieMagee
left a comment
JamieMagee
left a comment
There was a problem hiding this comment.
The post-install warning and rebuild paths look correct. I found one issue in strict preflight, where the fallback can read metadata from a stale installed version.
The strict allow-scripts preflight walks the ideal tree before reify replaces node_modules, so the package.json at a node's path can still belong to a different version. Reading gypfile: false from it made the preflight pass while the later rebuild, which sees the real package, synthesised node-gyp rebuild anyway. Compare the name and the version on disk against the node before the opt-out counts. If either is missing or differs, treat the opt-out as absent.
2 participants
Summary
gypfile: falseopts a package out of thenode-gyp rebuildinstall script npm synthesises when it finds abinding.gyp. That opt-out only survives when the tree node was built from a packument or read off disk. Nodes built from a lockfile carry nogypfilefield at all, so onnpm ci, or on anynpm installwith an existingpackage-lock.json, npm sees thebinding.gypand adds the script anyway.With
better-sqlite3@13.0.2that shows up as a spuriousinstall-scriptswarning. It is not only cosmetic: the synthesised script is real, so once it is covered byallowScripts(or on npm 10, which has no such gate)node-gyp rebuildactually runs against a package that ships prebuilt binaries and asked not to be rebuilt.Both the script enumeration in
install-scripts.jsand the build set inrebuild.jsnow read the opt-out back from the installedpackage.jsonwhen the node itself does not know it.Fixes #9837
Testing
Against the repro in the issue,
npm cino longer warns, andnpm ci --dangerously-allow-all-scripts --foreground-scriptsno longer invokes node-gyp. Added regression tests for both paths.