fix: don't retry cloning for each imported pkg

PR #2844

.changeset/brave-seals-drive.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/package-store": patch
+---
+
+When `package-import-method` is set to `auto`, cloning is only tried once. If it fails, it is not retried for other packages.

.changeset/silly-cows-camp.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/package-store": patch
+---
+
+Report package importing once it actually succeeds.

packages/package-store/src/fs/importIndexedDir.ts

@@ -8,7 +8,7 @@ import renameOverwrite = require('rename-overwrite')
 
 const filenameConflictsLogger = pnpmLogger('_filename-conflicts')
 
-type ImportFile = (src: string, dest: string) => Promise<void>
+export type ImportFile = (src: string, dest: string) => Promise<void>
 
 export default async function importIndexedDir (
   importFile: ImportFile,

packages/package-store/src/storeController/createImportPackage.ts

@@ -1,7 +1,6 @@
 import { importingLogger, packageImportMethodLogger } from '@pnpm/core-loggers'
 import { globalInfo, globalWarn } from '@pnpm/logger'
-import { PackageFilesResponse } from '@pnpm/store-controller-types'
-import importIndexedDir from '../fs/importIndexedDir'
+import importIndexedDir, { ImportFile } from '../fs/importIndexedDir'
 import path = require('path')
 import fs = require('mz/fs')
 import pLimit = require('p-limit')
@@ -9,18 +8,21 @@ import exists = require('path-exists')
 
 const limitLinking = pLimit(16)
 
+interface ImportOptions {
+  filesMap: Record<string, string>
+  force: boolean
+  fromStore: boolean
+}
+
+type ImportFunction = (to: string, opts: ImportOptions) => Promise<void>
+
 export default (
   packageImportMethod?: 'auto' | 'hardlink' | 'copy' | 'clone'
-): (
-    to: string,
-    opts: {
-      filesMap: Record<string, string>
-      fromStore: boolean
-      force: boolean
-    }
-  ) => ReturnType<(to: string, opts: { filesResponse: PackageFilesResponse, force: boolean }) => Promise<void>> => {
+): ImportFunction => {
   const importPackage = createImportPackage(packageImportMethod)
-  return (to, opts) => limitLinking(() => importPackage(to, opts))
+  return (to, opts) => limitLinking(async () => {
+    await importPackage(to, opts)
+  })
 }
 
 function createImportPackage (packageImportMethod?: 'auto' | 'hardlink' | 'copy' | 'clone') {
@@ -35,7 +37,7 @@ function createImportPackage (packageImportMethod?: 'auto' | 'hardlink' | 'copy'
     return clonePkg
   case 'hardlink':
     packageImportMethodLogger.debug({ method: 'hardlink' })
-    return hardlinkPkg
+    return hardlinkPkg.bind(null, linkOrCopy)
   case 'auto': {
     return createAutoImporter()
   }
@@ -47,57 +49,53 @@ function createImportPackage (packageImportMethod?: 'auto' | 'hardlink' | 'copy'
   }
 }
 
-function createAutoImporter () {
+function createAutoImporter (): ImportFunction {
   let auto = initialAuto
 
-  return auto
+  return async (to, opts) => {
+    await auto(to, opts)
+  }
 
   async function initialAuto (
     to: string,
-    opts: {
-      filesMap: Record<string, string>
-      force: boolean
-      fromStore: boolean
-    }
-  ) {
+    opts: ImportOptions
+  ): Promise<boolean> {
     try {
-      await clonePkg(to, opts)
+      if (!await clonePkg(to, opts)) return false
       packageImportMethodLogger.debug({ method: 'clone' })
       auto = clonePkg
-      return
+      return true
     } catch (err) {
       // ignore
     }
     try {
-      await hardlinkPkg(to, opts)
+      if (!await hardlinkPkg(fs.link, to, opts)) return false
       packageImportMethodLogger.debug({ method: 'hardlink' })
-      auto = hardlinkPkg
-      return
+      auto = hardlinkPkg.bind(null, linkOrCopy)
+      return true
     } catch (err) {
       if (!err.message.startsWith('EXDEV: cross-device link not permitted')) throw err
       globalWarn(err.message)
       globalInfo('Falling back to copying packages from store')
       packageImportMethodLogger.debug({ method: 'copy' })
       auto = copyPkg
-      await auto(to, opts)
+      return auto(to, opts)
     }
   }
 }
 
 async function clonePkg (
   to: string,
-  opts: {
-    filesMap: Record<string, string>
-    fromStore: boolean
-    force: boolean
-  }
+  opts: ImportOptions
 ) {
   const pkgJsonPath = path.join(to, 'package.json')
 
   if (!opts.fromStore || opts.force || !await exists(pkgJsonPath)) {
-    importingLogger.debug({ to, method: 'clone' })
     await importIndexedDir(cloneFile, to, opts.filesMap)
+    importingLogger.debug({ to, method: 'clone' })
+    return true
   }
+  return false
 }
 
 async function cloneFile (from: string, to: string) {
@@ -105,19 +103,18 @@ async function cloneFile (from: string, to: string) {
 }
 
 async function hardlinkPkg (
+  importFile: ImportFile,
   to: string,
-  opts: {
-    filesMap: Record<string, string>
-    force: boolean
-    fromStore: boolean
-  }
+  opts: ImportOptions
 ) {
   const pkgJsonPath = path.join(to, 'package.json')
 
   if (!opts.fromStore || opts.force || !await exists(pkgJsonPath) || !await pkgLinkedToStore(pkgJsonPath, opts.filesMap['package.json'], to)) {
+    await importIndexedDir(importFile, to, opts.filesMap)
     importingLogger.debug({ to, method: 'hardlink' })
-    await importIndexedDir(linkOrCopy, to, opts.filesMap)
+    return true
   }
+  return false
 }
 
 async function linkOrCopy (existingPath: string, newPath: string) {
@@ -148,16 +145,14 @@ async function isSameFile (file1: string, file2: string) {
 
 export async function copyPkg (
   to: string,
-  opts: {
-    filesMap: Record<string, string>
-    fromStore: boolean
-    force: boolean
-  }
+  opts: ImportOptions
 ) {
   const pkgJsonPath = path.join(to, 'package.json')
 
   if (!opts.fromStore || opts.force || !await exists(pkgJsonPath)) {
-    importingLogger.debug({ to, method: 'copy' })
     await importIndexedDir(fs.copyFile, to, opts.filesMap)
+    importingLogger.debug({ to, method: 'copy' })
+    return true
   }
+  return false
 }

packages/package-store/test/createImportPackage.spec.ts

@@ -36,9 +36,9 @@ test('packageImportMethod=auto: clone files by default', async (t) => {
 
 test('packageImportMethod=auto: link files if cloning fails', async (t) => {
   const importPackage = createImportPackage('auto')
-  fsMock.copyFile = () => {
+  fsMock.copyFile = sinon.spy(() => {
     throw new Error('This file system does not support cloning')
-  }
+  })
   fsMock.link = sinon.spy()
   fsMock.rename = sinon.spy()
   await importPackage('project/package', {
@@ -51,5 +51,41 @@ test('packageImportMethod=auto: link files if cloning fails', async (t) => {
   })
   t.ok(fsMock.link.calledWith(path.join('hash1'), path.join('project', '_tmp', 'package.json')))
   t.ok(fsMock.link.calledWith(path.join('hash2'), path.join('project', '_tmp', 'index.js')))
+  t.ok(fsMock.copyFile.called, 'the copy function is called the first time importing happens')
+  fsMock.copyFile.resetHistory()
+
+  // The copy function will not be called again
+  await importPackage('project2/package', {
+    filesMap: {
+      'index.js': 'hash2',
+      'package.json': 'hash1',
+    },
+    force: false,
+    fromStore: false,
+  })
+  t.notOk(fsMock.copyFile.called, 'the copy function is not called again')
+  t.ok(fsMock.link.calledWith(path.join('hash1'), path.join('project2', '_tmp', 'package.json')))
+  t.ok(fsMock.link.calledWith(path.join('hash2'), path.join('project2', '_tmp', 'index.js')))
+  t.end()
+})
+
+test('packageImportMethod=hardlink: fall back to copying if hardlinking fails', async (t) => {
+  const importPackage = createImportPackage('hardlink')
+  fsMock.link = sinon.spy(() => {
+    throw new Error('This file system does not support hard linking')
+  })
+  fsMock.copyFile = sinon.spy()
+  await importPackage('project/package', {
+    filesMap: {
+      'index.js': 'hash2',
+      'package.json': 'hash1',
+    },
+    force: false,
+    fromStore: false,
+  })
+  t.ok(fsMock.link.called)
+  t.ok(fsMock.copyFile.calledWith(path.join('hash1'), path.join('project', '_tmp', 'package.json')))
+  t.ok(fsMock.copyFile.calledWith(path.join('hash2'), path.join('project', '_tmp', 'index.js')))
+  fsMock.copyFile.resetHistory()
   t.end()
 })