From e338f44156a3d56e2a66aad6599fef6f55270977 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Sat, 4 Jun 2022 20:53:08 +0300 Subject: [PATCH] fix: resolve manifest promise when manifest is not found (#4826) close #4822 --- .changeset/seven-shirts-vanish.md | 12 +++++++++ packages/build-modules/src/buildSequence.ts | 2 +- packages/build-modules/src/index.ts | 4 +-- packages/cafs/package.json | 2 ++ packages/cafs/src/addFilesFromDir.ts | 3 +++ packages/cafs/src/addFilesFromTarball.ts | 3 +++ packages/cafs/test/index.ts | 6 ++++- packages/cafs/tsconfig.json | 3 +++ packages/core/test/install/fromRepo.ts | 12 +++++++++ packages/fetcher-base/src/index.ts | 2 +- .../package-requester/src/packageRequester.ts | 16 ++++++----- packages/resolve-dependencies/src/index.ts | 2 +- .../src/resolveDependencies.ts | 27 +++++++++++++++---- packages/store-controller-types/src/index.ts | 6 +++-- pnpm-lock.yaml | 6 ++++- 15 files changed, 85 insertions(+), 21 deletions(-) create mode 100644 .changeset/seven-shirts-vanish.md diff --git a/.changeset/seven-shirts-vanish.md b/.changeset/seven-shirts-vanish.md new file mode 100644 index 00000000000..7395e5ad9bd --- /dev/null +++ b/.changeset/seven-shirts-vanish.md @@ -0,0 +1,12 @@ +--- +"@pnpm/build-modules": patch +"@pnpm/cafs": patch +"@pnpm/core": patch +"@pnpm/fetcher-base": patch +"@pnpm/package-requester": patch +"@pnpm/resolve-dependencies": patch +"@pnpm/store-controller-types": patch +"pnpm": patch +--- + +It should be possible to install a git-hosted package that has no `package.json` file [#4822](https://github.com/pnpm/pnpm/issues/4822). diff --git a/packages/build-modules/src/buildSequence.ts b/packages/build-modules/src/buildSequence.ts index cb3827c9539..53ec49481c8 100644 --- a/packages/build-modules/src/buildSequence.ts +++ b/packages/build-modules/src/buildSequence.ts @@ -6,7 +6,7 @@ export interface DependenciesGraphNode { children: {[alias: string]: string} depPath: string dir: string - fetchingBundledManifest?: () => Promise + fetchingBundledManifest?: () => Promise filesIndexFile: string hasBin: boolean hasBundledDependencies: boolean diff --git a/packages/build-modules/src/index.ts b/packages/build-modules/src/index.ts index b560c99a533..5bb94b09acd 100644 --- a/packages/build-modules/src/index.ts +++ b/packages/build-modules/src/index.ts @@ -4,7 +4,7 @@ import { skippedOptionalDependencyLogger } from '@pnpm/core-loggers' import { runPostinstallHooks } from '@pnpm/lifecycle' import linkBins, { linkBinsOfPackages } from '@pnpm/link-bins' import logger from '@pnpm/logger' -import { fromDir as readPackageFromDir } from '@pnpm/read-package-json' +import { fromDir as readPackageFromDir, safeReadPackageFromDir } from '@pnpm/read-package-json' import { StoreController } from '@pnpm/store-controller-types' import { DependencyManifest } from '@pnpm/types' import runGroups from 'run-groups' @@ -170,7 +170,7 @@ export async function linkBinsOfDependencies ( const pkgs = await Promise.all(pkgNodes .map(async (dep) => ({ location: dep.dir, - manifest: await dep.fetchingBundledManifest?.() ?? (await readPackageFromDir(dep.dir) as DependencyManifest), + manifest: await dep.fetchingBundledManifest?.() ?? (await safeReadPackageFromDir(dep.dir) as DependencyManifest) ?? {}, })) ) diff --git a/packages/cafs/package.json b/packages/cafs/package.json index 08a6cb04805..6cd9a93bd20 100644 --- a/packages/cafs/package.json +++ b/packages/cafs/package.json @@ -32,10 +32,12 @@ }, "devDependencies": { "@pnpm/cafs": "workspace:3.0.16", + "@pnpm/types": "workspace:7.10.0", "@types/concat-stream": "^1.6.1", "@types/node": "^14.18.20", "@types/ssri": "^7.1.1", "@types/tar-stream": "^2.2.2", + "p-defer": "^3.0.0", "tempy": "^1.0.1" }, "bugs": { diff --git a/packages/cafs/src/addFilesFromDir.ts b/packages/cafs/src/addFilesFromDir.ts index 3aa79374f1d..b2f41f9bed3 100644 --- a/packages/cafs/src/addFilesFromDir.ts +++ b/packages/cafs/src/addFilesFromDir.ts @@ -23,6 +23,9 @@ export default async function ( ): Promise { const index: FilesIndex = {} await _retrieveFileIntegrities(cafs, dirname, dirname, index, manifest) + if (manifest && !index['package.json']) { + manifest.resolve(undefined) + } return index } diff --git a/packages/cafs/src/addFilesFromTarball.ts b/packages/cafs/src/addFilesFromTarball.ts index 0e8cd97b696..2d5aefddde7 100644 --- a/packages/cafs/src/addFilesFromTarball.ts +++ b/packages/cafs/src/addFilesFromTarball.ts @@ -47,5 +47,8 @@ export default async function ( .pipe(decompress() as Duplex) .on('error', reject).pipe(extract) }) + if (!filesIndex['package.json'] && manifest != null) { + manifest.resolve(undefined) + } return filesIndex } diff --git a/packages/cafs/test/index.ts b/packages/cafs/test/index.ts index 06caffa4b79..295aba84f81 100644 --- a/packages/cafs/test/index.ts +++ b/packages/cafs/test/index.ts @@ -1,4 +1,6 @@ import { createReadStream, promises as fs } from 'fs' +import { DependencyManifest } from '@pnpm/types' +import pDefer from 'p-defer' import path from 'path' import tempy from 'tempy' import createCafs, { @@ -25,7 +27,8 @@ describe('cafs', () => { it('replaces an already existing file, if the integrity of it was broken', async () => { const storeDir = tempy.directory() const srcDir = path.join(__dirname, 'fixtures/one-file') - const addFiles = async () => createCafs(storeDir).addFilesFromDir(srcDir) + const manifest = pDefer() + const addFiles = async () => createCafs(storeDir).addFilesFromDir(srcDir, manifest) let filesIndex = await addFiles() const { integrity } = await filesIndex['foo.txt'].writeResult @@ -37,6 +40,7 @@ describe('cafs', () => { filesIndex = await addFiles() await filesIndex['foo.txt'].writeResult expect(await fs.readFile(filePath, 'utf8')).toBe('foo\n') + expect(await manifest.promise).toEqual(undefined) }) }) diff --git a/packages/cafs/tsconfig.json b/packages/cafs/tsconfig.json index 093eae20002..e744cbfadff 100644 --- a/packages/cafs/tsconfig.json +++ b/packages/cafs/tsconfig.json @@ -17,6 +17,9 @@ }, { "path": "../store-controller-types" + }, + { + "path": "../types" } ] } diff --git a/packages/core/test/install/fromRepo.ts b/packages/core/test/install/fromRepo.ts index d2f070873a7..4ee4100d2b3 100644 --- a/packages/core/test/install/fromRepo.ts +++ b/packages/core/test/install/fromRepo.ts @@ -162,3 +162,15 @@ test.skip('from a non-github git repo', async () => { type: 'git', }) }) + +test('from a github repo the has no package.json file', async () => { + const project = prepareEmpty() + + const manifest = await addDependenciesToPackage({}, ['pnpm/for-testing.no-package-json'], await testDefaults()) + + await project.has('for-testing.no-package-json') + + expect(manifest.dependencies).toStrictEqual({ + 'for-testing.no-package-json': 'github:pnpm/for-testing.no-package-json', + }) +}) diff --git a/packages/fetcher-base/src/index.ts b/packages/fetcher-base/src/index.ts index 131f48b6ab9..c800ffaef4a 100644 --- a/packages/fetcher-base/src/index.ts +++ b/packages/fetcher-base/src/index.ts @@ -45,7 +45,7 @@ export interface FetchOptions { } export interface DeferredManifestPromise { - resolve: (manifest: DependencyManifest) => void + resolve: (manifest: DependencyManifest | undefined) => void reject: (err: Error) => void } diff --git a/packages/package-requester/src/packageRequester.ts b/packages/package-requester/src/packageRequester.ts index 03c44c13617..59ce98d1d8a 100644 --- a/packages/package-requester/src/packageRequester.ts +++ b/packages/package-requester/src/packageRequester.ts @@ -30,6 +30,7 @@ import { } from '@pnpm/resolver-base' import { BundledManifest, + BundledManifestFunction, FetchPackageToStoreFunction, FetchPackageToStoreOptions, PackageResponse, @@ -279,7 +280,7 @@ async function resolveAndFetch ( } interface FetchLock { - bundledManifest?: Promise + bundledManifest?: Promise files: Promise filesIndexFile: string finishing: Promise @@ -305,7 +306,7 @@ function fetchToStore ( }, opts: FetchPackageToStoreOptions ): { - bundledManifest?: () => Promise + bundledManifest?: BundledManifestFunction filesIndexFile: string files: () => Promise finishing: () => Promise @@ -385,6 +386,7 @@ function fetchToStore ( if (opts.fetchRawManifest && (result.bundledManifest == null)) { result.bundledManifest = removeKeyOnFail( result.files.then(async (filesResult) => { + if (!filesResult.filesIndex['package.json']) return undefined if (!filesResult.local) { const { integrity, mode } = filesResult.filesIndex['package.json'] const manifestPath = ctx.getFilePathByModeInCafs(integrity, mode) @@ -439,7 +441,7 @@ function fetchToStore ( if ((pkgFilesIndex?.files) != null) { const manifest = opts.fetchRawManifest - ? safeDeferredPromise() + ? safeDeferredPromise() : undefined if ( ( @@ -473,7 +475,7 @@ Actual package in the store by the given integrity: ${pkgFilesIndex.name}@${pkgF }) if (manifest != null) { manifest() - .then((manifest) => bundledManifest.resolve(normalizeBundledManifest(manifest))) + .then((manifest) => bundledManifest.resolve(manifest == null ? manifest : normalizeBundledManifest(manifest))) .catch(bundledManifest.reject) } finishing.resolve(undefined) @@ -496,11 +498,11 @@ Actual package in the store by the given integrity: ${pkgFilesIndex.name}@${pkgF const priority = (++ctx.requestsQueue['counter'] % ctx.requestsQueue['concurrency'] === 0 ? -1 : 1) * 1000 // eslint-disable-line const fetchManifest = opts.fetchRawManifest - ? safeDeferredPromise() + ? safeDeferredPromise() : undefined if (fetchManifest != null) { fetchManifest() - .then((manifest) => bundledManifest.resolve(normalizeBundledManifest(manifest))) + .then((manifest) => bundledManifest.resolve(manifest == null ? manifest : normalizeBundledManifest(manifest))) .catch(bundledManifest.reject) } const fetchedPackage = await ctx.requestsQueue.add(async () => ctx.fetch( @@ -561,7 +563,7 @@ Actual package in the store by the given integrity: ${pkgFilesIndex.name}@${pkgF /* eslint-disable @typescript-eslint/no-floating-promises */ bundledManifest.promise .then((manifest) => writeFilesIndexFile(filesIndexFile, { - pkg: manifest, + pkg: manifest ?? {}, files: integrity, })) .catch() diff --git a/packages/resolve-dependencies/src/index.ts b/packages/resolve-dependencies/src/index.ts index a649d86f584..f2bf5de9763 100644 --- a/packages/resolve-dependencies/src/index.ts +++ b/packages/resolve-dependencies/src/index.ts @@ -244,7 +244,7 @@ async function finishLockfileUpdates ( // The npm team suggests to always read the package.json for deciding whether the package has lifecycle scripts const pkgJson = await depNode.fetchingBundledManifest() depNode.requiresBuild = Boolean( - pkgJson.scripts != null && ( + pkgJson?.scripts != null && ( Boolean(pkgJson.scripts.preinstall) || Boolean(pkgJson.scripts.install) || Boolean(pkgJson.scripts.postinstall) diff --git a/packages/resolve-dependencies/src/resolveDependencies.ts b/packages/resolve-dependencies/src/resolveDependencies.ts index dc9bd006b70..d4b533e73ea 100644 --- a/packages/resolve-dependencies/src/resolveDependencies.ts +++ b/packages/resolve-dependencies/src/resolveDependencies.ts @@ -177,7 +177,7 @@ export interface ResolvedPackage { dev: boolean optional: boolean fetchingFiles: () => Promise - fetchingBundledManifest?: () => Promise + fetchingBundledManifest?: () => Promise filesIndexFile: string finishing: () => Promise name: string @@ -717,6 +717,11 @@ async function resolveDependency ( if (pkgResponse.body.isLocal) { const manifest = pkgResponse.body.manifest ?? await pkgResponse.bundledManifest!() // eslint-disable-line @typescript-eslint/dot-notation + if (!manifest) { + // This should actually never happen because the local-resolver returns a manifest + // even if no real manifest exists in the filesystem. + throw new PnpmError('MISSING_PACKAGE_JSON', `Can't install ${wantedDependency.pref}: Missing package.json file`) + } return { alias: wantedDependency.alias || manifest.name, depPath: pkgResponse.body.id, @@ -731,12 +736,12 @@ async function resolveDependency ( } } - let pkg: PackageManifest let prepare!: boolean let hasBin!: boolean - pkg = (ctx.readPackageHook != null) - ? await ctx.readPackageHook(pkgResponse.body.manifest ?? await pkgResponse.bundledManifest!()) - : pkgResponse.body.manifest ?? await pkgResponse.bundledManifest!() + let pkg: PackageManifest = await getManifestFromResponse(pkgResponse, wantedDependency) + if (ctx.readPackageHook != null) { + pkg = await ctx.readPackageHook(pkg) + } if (!pkg.name) { // TODO: don't fail on optional dependencies throw new PnpmError('MISSING_PACKAGE_NAME', `Can't install ${wantedDependency.pref}: Missing package name`) } @@ -889,6 +894,18 @@ async function resolveDependency ( } } +async function getManifestFromResponse ( + pkgResponse: PackageResponse, + wantedDependency: WantedDependency +): Promise { + const pkg = pkgResponse.body.manifest ?? await pkgResponse.bundledManifest!() + if (pkg) return pkg + return { + name: wantedDependency.pref.split('/').pop()!, + version: '0.0.0', + } +} + function getMissingPeers (pkg: PackageManifest, parentPkgAliases: ParentPkgAliases): Record { const missingPeers = {} as Record for (const [peerName, peerVersion] of Object.entries(pkg.peerDependencies ?? {})) { diff --git a/packages/store-controller-types/src/index.ts b/packages/store-controller-types/src/index.ts index 5fb45a42667..99321678b7b 100644 --- a/packages/store-controller-types/src/index.ts +++ b/packages/store-controller-types/src/index.ts @@ -47,7 +47,7 @@ export interface StoreController { export type FetchPackageToStoreFunction = ( opts: FetchPackageToStoreOptions ) => { - bundledManifest?: () => Promise + bundledManifest?: BundledManifestFunction filesIndexFile: string files: () => Promise finishing: () => Promise @@ -100,8 +100,10 @@ export interface RequestPackageOptions { workspacePackages?: WorkspacePackages } +export type BundledManifestFunction = () => Promise + export interface PackageResponse { - bundledManifest?: () => Promise + bundledManifest?: BundledManifestFunction files?: () => Promise filesIndexFile?: string finishing?: () => Promise // a package request is finished once its integrity is generated and saved diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 53df7a1bc48..68daf6309e4 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -208,6 +208,7 @@ importers: '@pnpm/fetcher-base': workspace:11.1.6 '@pnpm/graceful-fs': workspace:1.0.0 '@pnpm/store-controller-types': workspace:12.0.0 + '@pnpm/types': workspace:7.10.0 '@types/concat-stream': ^1.6.1 '@types/node': ^14.18.20 '@types/ssri': ^7.1.1 @@ -216,6 +217,7 @@ importers: concat-stream: ^2.0.0 decompress-maybe: ^1.0.0 get-stream: ^6.0.1 + p-defer: ^3.0.0 p-limit: ^3.1.0 path-temp: ^2.0.0 rename-overwrite: ^4.0.2 @@ -239,10 +241,12 @@ importers: tar-stream: 2.2.0 devDependencies: '@pnpm/cafs': 'link:' + '@pnpm/types': link:../types '@types/concat-stream': 1.6.1 '@types/node': 14.18.20 '@types/ssri': 7.1.1 '@types/tar-stream': 2.2.2 + p-defer: 3.0.0 tempy: 1.0.1 packages/calc-dep-state: @@ -5353,7 +5357,7 @@ packages: /@types/concat-stream/1.6.1: resolution: {integrity: sha512-eHE4cQPoj6ngxBZMvVf6Hw7Mh4jMW4U9lpGmS5GBPB9RYxlFg+CHaVN7ErNY4W9XfLIEn20b4VDYaIrbq0q4uA==} dependencies: - '@types/node': 14.18.20 + '@types/node': 17.0.39 dev: true /@types/cross-spawn/6.0.2: