diff --git a/.changeset/brave-seals-drive.md b/.changeset/brave-seals-drive.md new file mode 100644 index 0000000000..05f1f1321a --- /dev/null +++ b/.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. diff --git a/.changeset/silly-cows-camp.md b/.changeset/silly-cows-camp.md new file mode 100644 index 0000000000..7ae6ff83bb --- /dev/null +++ b/.changeset/silly-cows-camp.md @@ -0,0 +1,5 @@ +--- +"@pnpm/package-store": patch +--- + +Report package importing once it actually succeeds. diff --git a/packages/package-store/src/fs/importIndexedDir.ts b/packages/package-store/src/fs/importIndexedDir.ts index aad07f5956..8807e1dc00 100644 --- a/packages/package-store/src/fs/importIndexedDir.ts +++ b/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 +export type ImportFile = (src: string, dest: string) => Promise export default async function importIndexedDir ( importFile: ImportFile, diff --git a/packages/package-store/src/storeController/createImportPackage.ts b/packages/package-store/src/storeController/createImportPackage.ts index b829447558..169bf8fb63 100644 --- a/packages/package-store/src/storeController/createImportPackage.ts +++ b/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 + force: boolean + fromStore: boolean +} + +type ImportFunction = (to: string, opts: ImportOptions) => Promise + export default ( packageImportMethod?: 'auto' | 'hardlink' | 'copy' | 'clone' -): ( - to: string, - opts: { - filesMap: Record - fromStore: boolean - force: boolean - } - ) => ReturnType<(to: string, opts: { filesResponse: PackageFilesResponse, force: boolean }) => Promise> => { +): 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 - force: boolean - fromStore: boolean - } - ) { + opts: ImportOptions + ): Promise { 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 - 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 - 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 - 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 } diff --git a/packages/package-store/test/createImportPackage.spec.ts b/packages/package-store/test/createImportPackage.spec.ts index 9b89ff8b58..c125a8f784 100644 --- a/packages/package-store/test/createImportPackage.spec.ts +++ b/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() })