fix: installing packages that have bin dirs with subdirs (#3268)

close #3263
close #2332

.changeset/four-wombats-clap.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/package-bins": minor
+---
+
+Find all files inside the bin directory.

.changeset/khaki-humans-jump.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/lifecycle": patch
+---
+
+Run `node-gyp` when `binding.gyp` is present, even if an install lifecycle script is not present in the scripts field.

.changeset/kind-cats-love.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/link-bins": patch
+---
+
+Don't fail when linking bins of a package that uses the `directories.bin` and points to a directory that has subdirectories.

.changeset/tricky-carrots-sip.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/read-package-json": major
+---
+
+Don't normalize the `bin` field of `package.json`.

packages/lifecycle/src/index.ts

@@ -23,27 +23,31 @@ export async function runPostinstallHooks (
   }
 ): Promise<boolean> {
   const pkg = await readPackageJsonFromDir(opts.pkgRoot)
-  const scripts = pkg?.scripts ?? {}
-
-  if (!scripts.install) {
-    await checkBindingGyp(opts.pkgRoot, scripts)
+  if (pkg.scripts == null) {
+    pkg.scripts = {}
   }
 
-  if (scripts.preinstall) {
+  if (!pkg.scripts.install) {
+    await checkBindingGyp(opts.pkgRoot, pkg.scripts)
+  }
+
+  if (pkg.scripts.preinstall) {
     await runLifecycleHook('preinstall', pkg, opts)
   }
-  if (scripts.install) {
+  if (pkg.scripts.install) {
     await runLifecycleHook('install', pkg, opts)
   }
-  if (scripts.postinstall) {
+  if (pkg.scripts.postinstall) {
     await runLifecycleHook('postinstall', pkg, opts)
   }
 
-  if (opts.prepare && scripts.prepare) {
+  if (opts.prepare && pkg.scripts.prepare) {
     await runLifecycleHook('prepare', pkg, opts)
   }
 
-  return !!scripts.preinstall || !!scripts.install || !!scripts.postinstall
+  return pkg.scripts.preinstall != null ||
+    pkg.scripts.install != null ||
+    pkg.scripts.postinstall != null
 }
 
 /**

packages/link-bins/test/.gitignore

@@ -1,2 +1,3 @@
 !node_modules
 node_modules/.bin
+fixtures_for_testing

packages/link-bins/test/fixtures/bin-dir/pkg/bin/subdir/index.js

@@ -0,0 +1 @@
+#!/usr/bin/env node

packages/link-bins/test/fixtures/bin-dir/pkg/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "bin-dir",
+  "version": "0.0.0",
+  "directories": {
+    "bin": "bin"
+  }
+}

packages/link-bins/test/index.ts

@@ -260,3 +260,11 @@ test('linkBins() would not give warning if package has no bin field but inside n
 
   expect(warn).not.toHaveBeenCalled()
 })
+
+test('linkBins() links commands from bin directory with a subdirectory', async () => {
+  const binTarget = tempy.directory()
+
+  await linkBins(path.join(fixtures, 'bin-dir'), binTarget, { warn: () => {} })
+
+  expect(await fs.readdir(binTarget)).toEqual(getExpectedBins(['index.js']))
+})

packages/package-bins/jest.config.js

@@ -1 +1,5 @@
-module.exports = require('../../jest.config.js')
+const config = require('../../jest.config.js')
+module.exports = Object.assign({}, config, {
+  // Shallow so fixtures aren't matched
+  testMatch: ["**/test/*.[jt]s?(x)"]
+})

packages/package-bins/package.json

@@ -32,9 +32,8 @@
   "homepage": "https://github.com/pnpm/pnpm/blob/master/packages/package-bins#readme",
   "dependencies": {
     "@pnpm/types": "workspace:6.4.0",
-    "graceful-fs": "4.2.4",
-    "is-subdir": "^1.1.1",
-    "p-filter": "^2.1.0"
+    "fast-glob": "^3.2.4",
+    "is-subdir": "^1.1.1"
   },
   "devDependencies": {
     "@types/graceful-fs": "^4.1.4",

packages/package-bins/src/index.ts

@@ -1,12 +1,7 @@
-import { promisify } from 'util'
 import { DependencyManifest, PackageBin } from '@pnpm/types'
-import { readdir, stat } from 'graceful-fs'
+import fastGlob = require('fast-glob')
 import path = require('path')
 import isSubdir = require('is-subdir')
-import pFilter = require('p-filter')
-
-const readdirP = promisify(readdir)
-const statP = promisify(stat)
 
 export interface Command {
   name: string
@@ -20,20 +15,21 @@ export default async function binify (manifest: DependencyManifest, pkgPath: str
   if (manifest.directories?.bin) {
     const binDir = path.join(pkgPath, manifest.directories.bin)
     const files = await findFiles(binDir)
-    return pFilter(
-      files.map((file) => ({
-        name: file,
-        path: path.join(binDir, file),
-      })),
-      async (cmd: Command) => (await statP(cmd.path)).isFile()
-    )
+    return files.map((file) => ({
+      name: path.basename(file),
+      path: path.join(binDir, file),
+    }))
   }
   return []
 }
 
 async function findFiles (dir: string): Promise<string[]> {
   try {
-    return await readdirP(dir)
+    return await fastGlob('**', {
+      cwd: dir,
+      onlyFiles: true,
+      followSymbolicLinks: false,
+    })
   } catch (err) {
     if ((err as NodeJS.ErrnoException).code !== 'ENOENT') {
       throw err

packages/package-bins/test/fixtures/bin-dir/rootBin.js

@@ -0,0 +1 @@
+#!/usr/bin/env node

packages/package-bins/test/fixtures/bin-dir/subdir/subBin.js

@@ -0,0 +1 @@
+#!/usr/bin/env node

packages/package-bins/test/index.ts

@@ -15,6 +15,28 @@ test('getBinsFromPkg()', async () => {
   )
 })
 
+test('find all the bin files from a bin directory', async () => {
+  const fixtures = path.join(__dirname, 'fixtures')
+  expect(
+    await getBinsFromPkg({
+      name: 'bin-dir',
+      version: '1.0.0',
+
+      directories: { bin: 'bin-dir' },
+    }, fixtures)).toStrictEqual(
+    [
+      {
+        name: 'rootBin.js',
+        path: path.join(fixtures, 'bin-dir/rootBin.js'),
+      },
+      {
+        name: 'subBin.js',
+        path: path.join(fixtures, 'bin-dir/subdir/subBin.js'),
+      },
+    ]
+  )
+})
+
 test('get bin of scoped package', async () => {
   expect(
     await getBinsFromPkg({

packages/read-package-json/package.json

@@ -32,7 +32,8 @@
   "dependencies": {
     "@pnpm/error": "workspace:1.4.0",
     "@pnpm/types": "workspace:6.4.0",
-    "read-package-json": "^3.0.0"
+    "load-json-file": "^6.2.0",
+    "normalize-package-data": "^3.0.2"
   },
   "funding": "https://opencollective.com/pnpm"
 }

packages/read-package-json/src/index.ts

@@ -1,14 +1,14 @@
 import PnpmError from '@pnpm/error'
-import { promisify } from 'util'
 import { PackageManifest } from '@pnpm/types'
+import loadJsonFile = require('load-json-file')
+import normalizePackageData = require('normalize-package-data')
 import path = require('path')
-import readPackageManifestCB = require('read-package-json')
-
-const readPackageManifest = promisify<string, PackageManifest>(readPackageManifestCB)
 
 export default async function readPkg (pkgPath: string): Promise<PackageManifest> {
   try {
-    return await readPackageManifest(pkgPath)
+    const manifest = await loadJsonFile<PackageManifest>(pkgPath)
+    normalizePackageData(manifest)
+    return manifest
   } catch (err: any) { // eslint-disable-line
     if (err.code) throw err
     throw new PnpmError('BAD_PACKAGE_JSON', `${pkgPath}: ${err.message as string}`)

pnpm-lock.yaml

@@ -1466,14 +1466,12 @@ importers:
       '@pnpm/types': workspace:6.4.0
       '@types/graceful-fs': ^4.1.4
       '@types/node': ^14.14.22
-      graceful-fs: 4.2.4
+      fast-glob: ^3.2.4
       is-subdir: ^1.1.1
-      p-filter: ^2.1.0
     dependencies:
       '@pnpm/types': link:../types
-      graceful-fs: 4.2.4
+      fast-glob: 3.2.5
       is-subdir: 1.2.0
-      p-filter: 2.1.0
     devDependencies:
       '@pnpm/package-bins': 'link:'
       '@types/graceful-fs': 4.1.5
@@ -2583,11 +2581,13 @@ importers:
       '@pnpm/error': workspace:1.4.0
       '@pnpm/read-package-json': 'link:'
       '@pnpm/types': workspace:6.4.0
-      read-package-json: ^3.0.0
+      load-json-file: ^6.2.0
+      normalize-package-data: ^3.0.2
     dependencies:
       '@pnpm/error': link:../error
       '@pnpm/types': link:../types
-      read-package-json: 3.0.1
+      load-json-file: 6.2.0
+      normalize-package-data: 3.0.2
     devDependencies:
       '@pnpm/read-package-json': 'link:'
 
@@ -8591,6 +8591,14 @@ packages:
     engines: {node: '>=10'}
     dependencies:
       lru-cache: 6.0.0
+    dev: true
+
+  /hosted-git-info/4.0.1:
+    resolution: {integrity: sha512-eT7NrxAsppPRQEBSwKSosReE+v8OzABwEScQYk5d4uxaEPlzxTIku7LINXtBGalthkLhJnq5lBI89PfK43zAKg==}
+    engines: {node: '>=10'}
+    dependencies:
+      lru-cache: 6.0.0
+    dev: false
 
   /html-encoding-sniffer/1.0.2:
     resolution: {integrity: sha512-71lZziiDnsuabfdYiUeWdCVyKuqwWi23L8YeIgV9jSSZHCtb6wB1BKWooH7L3tn4/FuZJMVWyNaIDr4RGmaSYw==}
@@ -10914,6 +10922,17 @@ packages:
       resolve: 1.20.0
       semver: 7.3.4
       validate-npm-package-license: 3.0.4
+    dev: true
+
+  /normalize-package-data/3.0.2:
+    resolution: {integrity: sha512-6CdZocmfGaKnIHPVFhJJZ3GuR8SsLKvDANFp47Jmy51aKIr8akjAWTSxtpI+MBgBFdSMRyo4hMpDlT6dTffgZg==}
+    engines: {node: '>=10'}
+    dependencies:
+      hosted-git-info: 4.0.1
+      resolve: 1.20.0
+      semver: 7.3.4
+      validate-npm-package-license: 3.0.4
+    dev: false
 
   /normalize-path/2.1.1:
     resolution: {integrity: sha1-GrKLVW4Zg2Oowab35vogE3/mrtk=}
@@ -10943,10 +10962,6 @@ packages:
       once: 1.4.0
     dev: true
 
-  /npm-normalize-package-bin/1.0.1:
-    resolution: {integrity: sha512-EPfafl6JL5/rU+ot6P3gRSCpPDW5VmIzX959Ob1+ySFUuuYHWHekXpwdUZcKP5C+DS4GEtdJluwBjnsNDl+fSA==}
-    dev: false
-
   /npm-package-arg/4.2.1:
     resolution: {integrity: sha1-WTMD/eqF98Qid18X+et2cPaA4+w=}
     dependencies:
@@ -11910,16 +11925,6 @@ packages:
       strip-bom: 4.0.0
     dev: false
 
-  /read-package-json/3.0.1:
-    resolution: {integrity: sha512-aLcPqxovhJTVJcsnROuuzQvv6oziQx4zd3JvG0vGCL5MjTONUc4uJ90zCBC6R7W7oUKBNoR/F8pkyfVwlbxqng==}
-    engines: {node: '>=10'}
-    dependencies:
-      glob: 7.1.6
-      json-parse-even-better-errors: 2.3.1
-      normalize-package-data: 3.0.1
-      npm-normalize-package-bin: 1.0.1
-    dev: false
-
   /read-pkg-up/1.0.1:
     resolution: {integrity: sha1-nWPBMnbAZZGNV/ACpX9AobZD+wI=}
     engines: {node: '>=0.10.0'}