From dfd31e15d09ab6ef6c8fd51c3ccfe1a96ab5be7e Mon Sep 17 00:00:00 2001 From: lazerg Date: Sun, 9 Aug 2026 00:29:35 +0500 Subject: [PATCH] fix(arborist): honor gypfile:false on lockfile-driven installs --- workspaces/arborist/lib/arborist/rebuild.js | 7 +-- workspaces/arborist/lib/gypfile.js | 19 ++++++++ workspaces/arborist/lib/install-scripts.js | 5 ++- workspaces/arborist/test/arborist/rebuild.js | 46 ++++++++++++++++++++ workspaces/arborist/test/install-scripts.js | 23 ++++++++++ 5 files changed, 95 insertions(+), 5 deletions(-) create mode 100644 workspaces/arborist/lib/gypfile.js diff --git a/workspaces/arborist/lib/arborist/rebuild.js b/workspaces/arborist/lib/arborist/rebuild.js index 53c6f63e77d94..db750524e2f56 100644 --- a/workspaces/arborist/lib/arborist/rebuild.js +++ b/workspaces/arborist/lib/arborist/rebuild.js @@ -13,6 +13,7 @@ const { promiseRetry } = require('@gar/promise-retry') const { log, time } = require('proc-log') const { resolve, delimiter } = require('node:path') const { isScriptAllowed } = require('../script-allowed.js') +const { hasGypfileOptOut } = require('../gypfile.js') const boolEnv = b => b ? '1' : '' const sortNodes = (a, b) => (a.depth - b.depth) || localeCompare(a.path, b.path) @@ -277,10 +278,10 @@ module.exports = cls => class Builder extends cls { // Rebuild node-gyp dependencies lacking an install or preinstall script // note that 'scripts' might be missing entirely, and the package may // set gypfile:false to avoid this automatic detection. - const isGyp = gypfile !== false && - !install && + const isGyp = !install && !preinstall && - await isNodeGypPackage(node.path) + await isNodeGypPackage(node.path) && + !await hasGypfileOptOut(node.path, gypfile) if (bin || preinstall || install || postinstall || prepare || isGyp) { if (bin) { diff --git a/workspaces/arborist/lib/gypfile.js b/workspaces/arborist/lib/gypfile.js new file mode 100644 index 0000000000000..09f00c4005132 --- /dev/null +++ b/workspaces/arborist/lib/gypfile.js @@ -0,0 +1,19 @@ +const PackageJson = require('@npmcli/package-json') + +// `gypfile: false` opts a package out of the synthetic `node-gyp rebuild` +// install script npm adds when it finds a `binding.gyp`. +// +// The flag is only on the tree node when the node came from a packument or +// from disk. Lockfile-derived nodes (`npm ci`, a repeat `npm install`) carry +// no `gypfile` field at all, so the opt-out is invisible there and has to be +// read back off the installed package.json. +const hasGypfileOptOut = async (path, gypfile) => { + if (gypfile !== undefined) { + return gypfile === false + } + + const { content } = await PackageJson.load(path).catch(() => ({ content: {} })) + return content.gypfile === false +} + +module.exports = { hasGypfileOptOut } diff --git a/workspaces/arborist/lib/install-scripts.js b/workspaces/arborist/lib/install-scripts.js index 29e9eace8eb96..fa33a6c444d44 100644 --- a/workspaces/arborist/lib/install-scripts.js +++ b/workspaces/arborist/lib/install-scripts.js @@ -1,5 +1,6 @@ const { isNodeGypPackage } = require('@npmcli/node-gyp') const PackageJson = require('@npmcli/package-json') +const { hasGypfileOptOut } = require('./gypfile.js') // Returns the install-relevant lifecycle scripts that would run for a // given arborist Node, or `{}` if there are none. @@ -65,8 +66,8 @@ const getInstallScripts = async (node) => { const hasExplicitGypGate = !!(collected.preinstall || collected.install) if ( !hasExplicitGypGate && - pkg.gypfile !== false && - await isNodeGypPackage(node.path).catch(() => false) + await isNodeGypPackage(node.path).catch(() => false) && + !await hasGypfileOptOut(node.path, pkg.gypfile) ) { collected.install = 'node-gyp rebuild' } diff --git a/workspaces/arborist/test/arborist/rebuild.js b/workspaces/arborist/test/arborist/rebuild.js index 6c3062be89a00..791f8564a0f8c 100644 --- a/workspaces/arborist/test/arborist/rebuild.js +++ b/workspaces/arborist/test/arborist/rebuild.js @@ -603,6 +603,52 @@ t.test('do not rebuild node-gyp dependencies with gypfile:false', async t => { await arb.rebuild() }) +// ref: https://github.com/npm/cli/issues/9837 +t.test('do not rebuild node-gyp dependencies with gypfile:false from a lockfile', async t => { + const Arborist = t.mock('../../lib/arborist/index.js', { + '@npmcli/run-script': async () => { + throw new Error('should not run any scripts') + }, + }) + const path = t.testdir({ + node_modules: { + dep: { + 'package.json': JSON.stringify({ + name: 'dep', + version: '1.0.0', + gypfile: false, + }), + 'binding.gyp': '', + }, + }, + 'package-lock.json': JSON.stringify({ + name: 'project', + lockfileVersion: 3, + requires: true, + packages: { + '': { + name: 'project', + dependencies: { + dep: '1', + }, + }, + 'node_modules/dep': { + version: '1.0.0', + }, + }, + }), + 'package.json': JSON.stringify({ + name: 'project', + dependencies: { + dep: '1', + }, + }), + }) + const arb = new Arborist({ path, dangerouslyAllowAllScripts: true }) + const tree = await arb.loadVirtual() + await arb.rebuild({ nodes: [...tree.inventory.values()] }) +}) + // ref: https://github.com/npm/cli/issues/2905 t.test('do not run lifecycle scripts of linked deps twice', async t => { const testdir = t.testdir({ diff --git a/workspaces/arborist/test/install-scripts.js b/workspaces/arborist/test/install-scripts.js index 342e8e7d891d4..32db9de6ab5cd 100644 --- a/workspaces/arborist/test/install-scripts.js +++ b/workspaces/arborist/test/install-scripts.js @@ -95,6 +95,29 @@ t.test('synthetic node-gyp suppressed when gypfile: false', async t => { ) }) +t.test('synthetic node-gyp suppressed by gypfile: false on disk', async t => { + const getInstallScripts = mockGetInstallScripts(t, () => true) + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'dep', + version: '1.0.0', + gypfile: false, + }), + }) + t.strictSame(await getInstallScripts(node({ path })), {}) +}) + +t.test('synthetic node-gyp still detected when disk has no gypfile', async t => { + const getInstallScripts = mockGetInstallScripts(t, () => true) + const path = t.testdir({ + 'package.json': JSON.stringify({ name: 'dep', version: '1.0.0' }), + }) + t.strictSame( + await getInstallScripts(node({ path })), + { install: 'node-gyp rebuild' } + ) +}) + t.test('synthetic node-gyp suppressed when explicit install is present', async t => { const getInstallScripts = mockGetInstallScripts(t, () => true) t.strictSame(