fix: run node-gyp rebuild when a project has a binding.gyp file (#…

…8325) close #8293 --------- Co-authored-by: Zoltan Kochan <z@kochan.io>
pnpm · Jul 29, 2024 · 9899576 · 9899576
1 parent bf3cafa
commit 9899576
Show file tree

Hide file tree

Showing 9 changed files with 87 additions and 35 deletions.
diff --git a/.changeset/five-brooms-wait.md b/.changeset/five-brooms-wait.md
@@ -0,0 +1,6 @@
+---
+"@pnpm/lifecycle": patch
+"pnpm": patch
+---
+
+`pnpm install` should run `node-gyp rebuild` if the project has a `binding.gyp` file even if the project doesn't have an install script [#8293](https://github.com/pnpm/pnpm/issues/8293).
diff --git a/exec/lifecycle/package.json b/exec/lifecycle/package.json
@@ -50,6 +50,7 @@
   "devDependencies": {
     "@pnpm/lifecycle": "workspace:*",
     "@pnpm/test-fixtures": "workspace:*",
+    "@pnpm/prepare": "workspace:*",
     "@pnpm/test-ipc-server": "workspace:*",
     "@types/is-windows": "catalog:",
     "@types/rimraf": "catalog:",

diff --git a/exec/lifecycle/src/index.ts b/exec/lifecycle/src/index.ts
@@ -1,9 +1,6 @@
-import path from 'path'
 import { safeReadPackageJsonFromDir } from '@pnpm/read-package-json'
-import exists from 'path-exists'
 import { runLifecycleHook, type RunLifecycleHookOptions } from './runLifecycleHook'
 import { runLifecycleHooksConcurrently, type RunLifecycleHooksConcurrentlyOptions } from './runLifecycleHooksConcurrently'
-import { type PackageScripts } from '@pnpm/types'
 
 export function makeNodeRequireOption (modulePath: string): { NODE_OPTIONS: string } {
   let { NODE_OPTIONS } = process.env
@@ -27,34 +24,15 @@ export async function runPostinstallHooks (
     pkg.scripts = {}
   }
 
-  if (!pkg.scripts.install && !pkg.scripts.preinstall) {
-    await checkBindingGyp(opts.pkgRoot, pkg.scripts)
-  }
-
   if (pkg.scripts.preinstall) {
     await runLifecycleHook('preinstall', pkg, opts)
   }
-  if (pkg.scripts.install) {
-    await runLifecycleHook('install', pkg, opts)
-  }
+  const executedAnInstallScript = await runLifecycleHook('install', pkg, opts)
   if (pkg.scripts.postinstall) {
     await runLifecycleHook('postinstall', pkg, opts)
   }
 
   return pkg.scripts.preinstall != null ||
-    pkg.scripts.install != null ||
+    executedAnInstallScript ||
     pkg.scripts.postinstall != null
 }
-
-/**
- * Run node-gyp when binding.gyp is available. Only do this when there are no
- * `install` and `preinstall` scripts (see `npm help scripts`).
- */
-async function checkBindingGyp (
-  root: string,
-  scripts: PackageScripts
-): Promise<void> {
-  if (await exists(path.join(root, 'binding.gyp'))) {
-    scripts.install = 'node-gyp rebuild'
-  }
-}
diff --git a/exec/lifecycle/src/runLifecycleHook.ts b/exec/lifecycle/src/runLifecycleHook.ts
@@ -1,7 +1,8 @@
+import path from 'path'
 import { lifecycleLogger } from '@pnpm/core-loggers'
 import { globalWarn } from '@pnpm/logger'
 import lifecycle from '@pnpm/npm-lifecycle'
-import { type DependencyManifest, type ProjectManifest, type PrepareExecutionEnv } from '@pnpm/types'
+import { type DependencyManifest, type ProjectManifest, type PrepareExecutionEnv, type PackageScripts } from '@pnpm/types'
 import { PnpmError } from '@pnpm/error'
 import { existsSync } from 'fs'
 import isWindows from 'is-windows'
@@ -31,7 +32,7 @@ export async function runLifecycleHook (
   stage: string,
   manifest: ProjectManifest | DependencyManifest,
   opts: RunLifecycleHookOptions
-): Promise<void> {
+): Promise<boolean> {
   const optional = opts.optional === true
 
   // To remediate CVE_2024_27980, Node.js does not allow .bat or .cmd files to
@@ -70,19 +71,28 @@ Please unset the script-shell option, or configure it to a .exe instead.
   const m = { _id: getId(manifest), ...manifest }
   m.scripts = { ...m.scripts }
 
-  if (stage === 'start' && !m.scripts.start) {
-    if (!existsSync('server.js')) {
-      throw new PnpmError('NO_SCRIPT_OR_SERVER', 'Missing script start or file server.js')
+  switch (stage) {
+  case 'start':
+    if (!m.scripts.start) {
+      if (!existsSync('server.js')) {
+        throw new PnpmError('NO_SCRIPT_OR_SERVER', 'Missing script start or file server.js')
+      }
+      m.scripts.start = 'node server.js'
+    }
+    break
+  case 'install':
+    if (!m.scripts.install && !m.scripts.preinstall) {
+      checkBindingGyp(opts.pkgRoot, m.scripts)
     }
-    m.scripts.start = 'node server.js'
+    break
   }
   if (opts.args?.length && m.scripts?.[stage]) {
     const escapedArgs = opts.args.map((arg) => JSON.stringify(arg))
     m.scripts[stage] = `${m.scripts[stage]} ${escapedArgs.join(' ')}`
   }
   // This script is used to prevent the usage of npm or Yarn.
   // It does nothing, when pnpm is used, so we may skip its execution.
-  if (m.scripts[stage] === 'npx only-allow pnpm') return
+  if (m.scripts[stage] === 'npx only-allow pnpm' || !m.scripts[stage]) return false
   if (opts.stdio !== 'inherit') {
     lifecycleLogger.debug({
       depPath: opts.depPath,
@@ -131,6 +141,7 @@ Please unset the script-shell option, or configure it to a .exe instead.
     stdio: opts.stdio ?? 'pipe',
     unsafePerm: opts.unsafePerm,
   })
+  return true
 
   function npmLog (prefix: string, logId: string, stdtype: string, line: string): void {
     switch (stdtype) {
@@ -162,6 +173,19 @@ Please unset the script-shell option, or configure it to a .exe instead.
   }
 }
 
+/**
+ * Run node-gyp when binding.gyp is available. Only do this when there are no
+ * `install` and `preinstall` scripts (see `npm help scripts`).
+ */
+function checkBindingGyp (
+  root: string,
+  scripts: PackageScripts
+) {
+  if (existsSync(path.join(root, 'binding.gyp'))) {
+    scripts.install = 'node-gyp rebuild'
+  }
+}
+
 function getId (manifest: ProjectManifest | DependencyManifest): string {
   return `${manifest.name ?? ''}@${manifest.version ?? ''}`
 }

diff --git a/exec/lifecycle/src/runLifecycleHooksConcurrently.ts b/exec/lifecycle/src/runLifecycleHooksConcurrently.ts
@@ -65,9 +65,9 @@ export async function runLifecycleHooksConcurrently (
         }
         let isBuilt = false
         for (const stage of (importerStages ?? stages)) {
-          if (!manifest.scripts?.[stage]) continue
-          await runLifecycleHook(stage, manifest, runLifecycleHookOpts) // eslint-disable-line no-await-in-loop
-          isBuilt = true
+          if (await runLifecycleHook(stage, manifest, runLifecycleHookOpts)) { // eslint-disable-line no-await-in-loop
+            isBuilt = true
+          }
         }
         if (targetDirs == null || targetDirs.length === 0 || !isBuilt) return
         const filesResponse = await fetchFromDir(rootDir, { resolveSymlinks: opts.resolveSymlinksInInjectedDirs })

diff --git a/exec/lifecycle/test/fixtures/.gitignore b/exec/lifecycle/test/fixtures/.gitignore
@@ -1,2 +1,3 @@
 output.json
 test.sock
+
diff --git a/exec/lifecycle/test/index.ts b/exec/lifecycle/test/index.ts
@@ -1,9 +1,16 @@
 /// <reference path="../../../__typings__/index.d.ts"/>
+import fs from 'fs'
 import path from 'path'
-import { runLifecycleHook, runPostinstallHooks } from '@pnpm/lifecycle'
+import { runLifecycleHook, runLifecycleHooksConcurrently, runPostinstallHooks } from '@pnpm/lifecycle'
 import { PnpmError } from '@pnpm/error'
 import { createTestIpcServer } from '@pnpm/test-ipc-server'
 import { fixtures } from '@pnpm/test-fixtures'
+import { tempDir } from '@pnpm/prepare'
+import { type ProjectRootDir } from '@pnpm/types'
+import { type StoreController } from '@pnpm/store-controller-types'
+import isWindows from 'is-windows'
+
+const skipOnWindows = isWindows() ? test.skip : test
 
 const f = fixtures(path.join(__dirname, 'fixtures'))
 const rootModulesDir = path.join(__dirname, '..', 'node_modules')
@@ -100,3 +107,32 @@ test('preinstall script does not trigger node-gyp rebuild', async () => {
 
   expect(server.getLines()).toStrictEqual(['preinstall'])
 })
+
+skipOnWindows('runLifecycleHooksConcurrently() should check binding.gyp', async () => {
+  const projectDir = tempDir(false)
+
+  fs.writeFileSync(path.join(projectDir, 'binding.gyp'), JSON.stringify({
+    targets: [
+      {
+        target_name: 'run_js_script',
+        actions: [
+          {
+            action_name: 'execute_postinstall',
+            inputs: [],
+            outputs: ['foo'],
+            action: ['node', '-e', 'require(\'fs\').writeFileSync(\'foo\', \'\', \'utf8\')'],
+          },
+        ],
+      },
+    ],
+  }), 'utf8')
+
+  await runLifecycleHooksConcurrently(['install'], [{ buildIndex: 0, rootDir: projectDir as ProjectRootDir, modulesDir: '', manifest: {} }], 5, {
+    storeController: {} as StoreController,
+    optional: false,
+    rawConfig: {},
+    unsafePerm: true,
+  })
+
+  expect(fs.existsSync(path.join(projectDir, 'foo'))).toBeTruthy()
+})
diff --git a/exec/lifecycle/tsconfig.json b/exec/lifecycle/tsconfig.json
@@ -9,6 +9,9 @@
     "../../__typings__/**/*.d.ts"
   ],
   "references": [
+    {
+      "path": "../../__utils__/prepare"
+    },
     {
       "path": "../../__utils__/test-fixtures"
     },

diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml