From fd016a5ad2312c2a70174fe45bf6a99903c2a1fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Tue, 12 Sep 2023 12:34:24 +0200 Subject: [PATCH 1/3] Two fixes --- .github/workflows/e2e-cra-workflow.yml | 1 + .../sources/features/cache.test.ts | 37 ++++++++++++++++++- packages/yarnpkg-cli/sources/lib.ts | 6 +++ packages/yarnpkg-core/sources/Cache.ts | 14 +++---- packages/yarnpkg-core/sources/Project.ts | 4 +- packages/yarnpkg-core/sources/formatUtils.ts | 2 +- 6 files changed, 53 insertions(+), 11 deletions(-) diff --git a/.github/workflows/e2e-cra-workflow.yml b/.github/workflows/e2e-cra-workflow.yml index 0b6d35baaa37..dddb3f9f6a81 100644 --- a/.github/workflows/e2e-cra-workflow.yml +++ b/.github/workflows/e2e-cra-workflow.yml @@ -1,3 +1,4 @@ +# on: schedule: - cron: '0 */4 * * *' diff --git a/packages/acceptance-tests/pkg-tests-specs/sources/features/cache.test.ts b/packages/acceptance-tests/pkg-tests-specs/sources/features/cache.test.ts index b5f5d44b000c..6aff09104387 100644 --- a/packages/acceptance-tests/pkg-tests-specs/sources/features/cache.test.ts +++ b/packages/acceptance-tests/pkg-tests-specs/sources/features/cache.test.ts @@ -1,4 +1,4 @@ -import {ppath, xfs} from '@yarnpkg/fslib'; +import {Filename, ppath, xfs} from '@yarnpkg/fslib'; describe(`Cache`, () => { test( @@ -114,6 +114,41 @@ describe(`Cache`, () => { }), ); + test( + `it should ignore checksum mismatches and regenerate archives when their cache key is different from Yarn's own cache key, if cacheMigrationMode=always (global cache, hot)`, + makeTemporaryEnv({ + dependencies: { + [`no-deps`]: `1.0.0`, + }, + }, { + cacheMigrationMode: `always`, + enableGlobalCache: true, + }, async ({path, run, source}) => { + await run(`install`, { + cacheVersionOverride: `2`, + }); + + const cacheFiles = await xfs.readdirPromise(ppath.join(path, `.yarn/global/cache`)); + const cacheFile = ppath.join(path, `.yarn/global/cache`, cacheFiles.find(name => name.startsWith(`no-deps-`))!); + + // Adding some data to give it a different checksum than what we'll have for + // "cache key v1"; zip archives allow pseudo-arbitrary content at their end + await xfs.appendFilePromise(cacheFile, `corrupted archive`); + + // Removing the lockfile to make sure it'll be populated with "cache key v1" data + await xfs.removePromise(ppath.join(path, Filename.lockfile)); + + await run(`install`, { + cacheVersionOverride: `1`, + }); + + await run(`install`, { + cacheVersionOverride: `2`, + cacheCheckpointOverride: `1`, + }); + }), + ); + test( `it should update the cache files when changing the compression level, if cacheMigrationMode=match-spec`, makeTemporaryEnv({ diff --git a/packages/yarnpkg-cli/sources/lib.ts b/packages/yarnpkg-cli/sources/lib.ts index f28ff20080db..2d938174a03e 100644 --- a/packages/yarnpkg-cli/sources/lib.ts +++ b/packages/yarnpkg-cli/sources/lib.ts @@ -99,6 +99,12 @@ function checkCwd(cli: YarnCli, argv: Array) { } else if (argv.length >= 1 && argv[0].startsWith(`--cwd=`)) { cwd = npath.toPortablePath(argv[0].slice(6)); postCwdArgv = argv.slice(1); + } else if (argv[0] === `add` && argv[argv.length - 2] === `--cwd`) { + // CRA adds `--cwd` at the end of the command; it's not ideal, but since + // it's unlikely to receive more releases we can just special-case it + // TODO v5: remove this special case + cwd = npath.toPortablePath(argv[argv.length - 1]); + postCwdArgv = argv.slice(0, argv.length - 2); } cli.defaultContext.cwd = cwd !== null diff --git a/packages/yarnpkg-core/sources/Cache.ts b/packages/yarnpkg-core/sources/Cache.ts index 222b02f946b1..4891252ca943 100644 --- a/packages/yarnpkg-core/sources/Cache.ts +++ b/packages/yarnpkg-core/sources/Cache.ts @@ -145,13 +145,6 @@ export class Cache { } getLocatorPath(locator: Locator, expectedChecksum: string | null, opts: CacheOptions = {}) { - // If there is no mirror, then the local cache *is* the mirror, in which - // case we use the versioned filename pattern. Same if the package is - // unstable, meaning it may be there or not depending on the environment, - // so we can't rely on its checksum to get a stable location. - if (this.mirrorCwd === null || opts.unstablePackages?.has(locator.locatorHash)) - return ppath.resolve(this.cwd, this.getVersionFilename(locator)); - // If we don't yet know the checksum, discard the path resolution for now // until the checksum can be obtained from somewhere (mirror or network). if (expectedChecksum === null) @@ -180,6 +173,13 @@ export class Cache { if (cacheSpec !== this.cacheSpec && migrationMode !== `required-only`) return null; + // If there is no mirror, then the local cache *is* the mirror, in which + // case we use the versioned filename pattern. Same if the package is + // unstable, meaning it may be there or not depending on the environment, + // so we can't rely on its checksum to get a stable location. + if (this.mirrorCwd === null || opts.unstablePackages?.has(locator.locatorHash)) + return ppath.resolve(this.cwd, this.getVersionFilename(locator)); + return ppath.resolve(this.cwd, this.getChecksumFilename(locator, expectedChecksum)); } diff --git a/packages/yarnpkg-core/sources/Project.ts b/packages/yarnpkg-core/sources/Project.ts index 956d778e3a26..92e2d6302cb8 100644 --- a/packages/yarnpkg-core/sources/Project.ts +++ b/packages/yarnpkg-core/sources/Project.ts @@ -1151,7 +1151,7 @@ export class Project { zero: `No new packages`, one: `A package was`, more: `${formatUtils.pretty(this.configuration, addedCount, formatUtils.Type.NUMBER)} packages were`, - })} added to the cache`; + })} added to the project`; const removedLine = `${miscUtils.plural(removedCount, { zero: `none were`, @@ -2690,7 +2690,7 @@ function emitPeerDependencyWarnings(project: Project, report: Report) { formatUtils.pretty(project.configuration, warning.hash, formatUtils.Type.CODE) }), requested by ${ structUtils.prettyIdent(project.configuration, warning.requester) - }`; + }.`; }) ?? []; report.startSectionSync({ diff --git a/packages/yarnpkg-core/sources/formatUtils.ts b/packages/yarnpkg-core/sources/formatUtils.ts index 7681a569bdc9..3482b145ea36 100644 --- a/packages/yarnpkg-core/sources/formatUtils.ts +++ b/packages/yarnpkg-core/sources/formatUtils.ts @@ -476,7 +476,7 @@ export function prettyTruncatedLocatorList(configuration: Configuration, locator .slice(0, -2); const mark = `X`.repeat(locatorsCopy.length.toString().length); - const suffix = `and ${mark} more`; + const suffix = `and ${mark} more.`; let otherCount = locatorsCopy.length; while (named.length > 1 && remainingLength < suffix.length) { From 944fabca94b0b459340952345df57a464df6cf4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Tue, 12 Sep 2023 13:11:09 +0200 Subject: [PATCH 2/3] Splits some logic outside of getLocatorPath --- .yarn/versions/eec3be1d.yml | 34 ++++++++++++++++ packages/yarnpkg-core/sources/Cache.ts | 54 +++++++++++++++----------- 2 files changed, 65 insertions(+), 23 deletions(-) create mode 100644 .yarn/versions/eec3be1d.yml diff --git a/.yarn/versions/eec3be1d.yml b/.yarn/versions/eec3be1d.yml new file mode 100644 index 000000000000..2d6a48f178d8 --- /dev/null +++ b/.yarn/versions/eec3be1d.yml @@ -0,0 +1,34 @@ +releases: + "@yarnpkg/cli": patch + "@yarnpkg/core": patch + +declined: + - "@yarnpkg/plugin-compat" + - "@yarnpkg/plugin-constraints" + - "@yarnpkg/plugin-dlx" + - "@yarnpkg/plugin-essentials" + - "@yarnpkg/plugin-exec" + - "@yarnpkg/plugin-file" + - "@yarnpkg/plugin-git" + - "@yarnpkg/plugin-github" + - "@yarnpkg/plugin-http" + - "@yarnpkg/plugin-init" + - "@yarnpkg/plugin-interactive-tools" + - "@yarnpkg/plugin-link" + - "@yarnpkg/plugin-nm" + - "@yarnpkg/plugin-npm" + - "@yarnpkg/plugin-npm-cli" + - "@yarnpkg/plugin-pack" + - "@yarnpkg/plugin-patch" + - "@yarnpkg/plugin-pnp" + - "@yarnpkg/plugin-pnpm" + - "@yarnpkg/plugin-stage" + - "@yarnpkg/plugin-typescript" + - "@yarnpkg/plugin-version" + - "@yarnpkg/plugin-workspace-tools" + - "@yarnpkg/builder" + - "@yarnpkg/doctor" + - "@yarnpkg/extensions" + - "@yarnpkg/nm" + - "@yarnpkg/pnpify" + - "@yarnpkg/sdks" diff --git a/packages/yarnpkg-core/sources/Cache.ts b/packages/yarnpkg-core/sources/Cache.ts index 4891252ca943..1bab2b9e5a48 100644 --- a/packages/yarnpkg-core/sources/Cache.ts +++ b/packages/yarnpkg-core/sources/Cache.ts @@ -144,40 +144,50 @@ export class Cache { return `${structUtils.slugifyLocator(locator)}-${significantChecksum}.zip` as Filename; } - getLocatorPath(locator: Locator, expectedChecksum: string | null, opts: CacheOptions = {}) { + isChecksumCompatible(checksum: string) { // If we don't yet know the checksum, discard the path resolution for now // until the checksum can be obtained from somewhere (mirror or network). - if (expectedChecksum === null) - return null; + if (checksum === null) + return false; const { cacheVersion, cacheSpec, - } = splitChecksumComponents(expectedChecksum); + } = splitChecksumComponents(checksum); if (cacheVersion === null) - return null; + return false; // The cache keys must always be at least as old as the last checkpoint. if (cacheVersion < CACHE_CHECKPOINT) - return null; + return false; const migrationMode = this.configuration.get(`cacheMigrationMode`); // If the global cache is used, then the lockfile must always be up-to-date, // so the archives must be regenerated each time the version changes. if (cacheVersion < CACHE_VERSION && migrationMode === `always`) - return null; + return false; // If the cache spec changed, we may need to regenerate the archive if (cacheSpec !== this.cacheSpec && migrationMode !== `required-only`) - return null; + return false; + + return true; + } + + getLocatorPath(locator: Locator, expectedChecksum: string | null, opts: CacheOptions = {}) { + // When using the global cache we want the archives to be named as per + // the cache key rather than the hash, as otherwise we wouldn't be able + // to find them if we didn't have the hash (which is the case when adding + // new dependencies to a project). + if (this.mirrorCwd === null) + return ppath.resolve(this.cwd, this.getVersionFilename(locator)); - // If there is no mirror, then the local cache *is* the mirror, in which - // case we use the versioned filename pattern. Same if the package is - // unstable, meaning it may be there or not depending on the environment, - // so we can't rely on its checksum to get a stable location. - if (this.mirrorCwd === null || opts.unstablePackages?.has(locator.locatorHash)) + // Same thing if we don't know the checksum; it means that the package + // doesn't support being checksum'd (unstablePackage), so we fallback + // on the versioned filename. + if (expectedChecksum === null) return ppath.resolve(this.cwd, this.getVersionFilename(locator)); return ppath.resolve(this.cwd, this.getChecksumFilename(locator, expectedChecksum)); @@ -351,14 +361,11 @@ export class Cache { const {path: packagePath, source: packageSource} = await loadPackageThroughMirror(); // Do this before moving the file so that we don't pollute the cache with corrupted archives - const checksum = (await validateFile(packagePath, { + const {hash: checksum} = await validateFile(packagePath, { isColdHit: true, - })).hash; + }); const cachePath = this.getLocatorPath(locator, checksum, opts); - if (!cachePath) - throw new Error(`Assertion failed: Expected the cache path to be available`); - const copyProcess: Array<() => Promise> = []; // Copy the package into the mirror @@ -394,10 +401,11 @@ export class Cache { const loadPackageThroughMutex = async () => { const mutexedLoad = async () => { - // We don't yet know whether the cache path can be computed yet, since that - // depends on whether the cache is actually the mirror or not, and whether - // the checksum is known or not. - const tentativeCachePath = this.getLocatorPath(locator, expectedChecksum, opts); + const isUnstablePackage = opts.unstablePackages?.has(locator.locatorHash); + + const tentativeCachePath = isUnstablePackage || expectedChecksum && this.isChecksumCompatible(expectedChecksum) + ? this.getLocatorPath(locator, expectedChecksum, opts) + : null; const cacheFileExists = tentativeCachePath !== null ? this.markedFiles.has(tentativeCachePath) || await baseFs.existsPromise(tentativeCachePath) @@ -414,7 +422,7 @@ export class Cache { action(); if (!isCacheHit) { - if (this.immutable && opts.unstablePackages?.has(locator.locatorHash)) + if (this.immutable && isUnstablePackage) throw new ReportError(MessageName.IMMUTABLE_CACHE, `Cache entry required but missing for ${structUtils.prettyLocator(this.configuration, locator)}; consider defining ${formatUtils.pretty(this.configuration, `supportedArchitectures`, formatUtils.Type.CODE)} to cache packages for multiple systems`); return loadPackage(); From 0a7e1b59776b4a4d485c89dce148f4f2559131d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Tue, 12 Sep 2023 14:31:08 +0200 Subject: [PATCH 3/3] Fixes tests --- .../__snapshots__/mergeConflictResolution.test.ts.snap | 8 ++++---- packages/yarnpkg-core/tests/formatUtils.test.ts | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/acceptance-tests/pkg-tests-specs/sources/features/__snapshots__/mergeConflictResolution.test.ts.snap b/packages/acceptance-tests/pkg-tests-specs/sources/features/__snapshots__/mergeConflictResolution.test.ts.snap index 204fbc991a9b..7ac20c264ba1 100644 --- a/packages/acceptance-tests/pkg-tests-specs/sources/features/__snapshots__/mergeConflictResolution.test.ts.snap +++ b/packages/acceptance-tests/pkg-tests-specs/sources/features/__snapshots__/mergeConflictResolution.test.ts.snap @@ -41,7 +41,7 @@ exports[`Features Merge Conflict Resolution it should properly fix merge conflic ➤ YN0000: ┌ Resolution step ➤ YN0000: └ Completed ➤ YN0000: ┌ Fetch step -➤ YN0013: │ No new packages added to the cache, but one was removed (- 0.78 KiB). +➤ YN0013: │ No new packages added to the project, but one was removed (- 0.78 KiB). ➤ YN0000: └ Completed ➤ YN0000: ┌ Link step ➤ YN0000: └ Completed @@ -141,7 +141,7 @@ exports[`Features Merge Conflict Resolution it should support fixing cherry-pick ➤ YN0000: ┌ Resolution step ➤ YN0000: └ Completed ➤ YN0000: ┌ Fetch step -➤ YN0013: │ No new packages added to the cache, but one was removed (- 0.78 KiB). +➤ YN0013: │ No new packages added to the project, but one was removed (- 0.78 KiB). ➤ YN0000: └ Completed ➤ YN0000: ┌ Link step ➤ YN0000: └ Completed @@ -191,7 +191,7 @@ exports[`Features Merge Conflict Resolution it should support fixing rebase conf ➤ YN0000: ┌ Resolution step ➤ YN0000: └ Completed ➤ YN0000: ┌ Fetch step -➤ YN0013: │ No new packages added to the cache, but one was removed (- 0.78 KiB). +➤ YN0013: │ No new packages added to the project, but one was removed (- 0.78 KiB). ➤ YN0000: └ Completed ➤ YN0000: ┌ Link step ➤ YN0000: └ Completed @@ -241,7 +241,7 @@ exports[`Features Merge Conflict Resolution it shouldn't re-fetch the lockfile m ➤ YN0000: ┌ Resolution step ➤ YN0000: └ Completed ➤ YN0000: ┌ Fetch step -➤ YN0013: │ No new packages added to the cache, but one was removed (- 0.78 KiB). +➤ YN0013: │ No new packages added to the project, but one was removed (- 0.78 KiB). ➤ YN0000: └ Completed ➤ YN0000: ┌ Link step ➤ YN0000: └ Completed diff --git a/packages/yarnpkg-core/tests/formatUtils.test.ts b/packages/yarnpkg-core/tests/formatUtils.test.ts index 555c72cdf6a8..3e3645a678e2 100644 --- a/packages/yarnpkg-core/tests/formatUtils.test.ts +++ b/packages/yarnpkg-core/tests/formatUtils.test.ts @@ -27,11 +27,11 @@ describe(`formatUtils`, () => { }); it(`should return cut the list of locators if needed`, () => { - expect(formatUtils.prettyTruncatedLocatorList(configuration, LOCATORS, 99)).toEqual(`foo@npm:1.0.0, foo@npm:2.0.0, foo@npm:3.0.0, foo@npm:4.0.0, foo@npm:5.0.0, and 2 more`); + expect(formatUtils.prettyTruncatedLocatorList(configuration, LOCATORS, 100)).toEqual(`foo@npm:1.0.0, foo@npm:2.0.0, foo@npm:3.0.0, foo@npm:4.0.0, foo@npm:5.0.0, and 2 more.`); }); it(`should return cut the list of locators if needed (right on edge)`, () => { - expect(formatUtils.prettyTruncatedLocatorList(configuration, LOCATORS, 100)).toEqual(`foo@npm:1.0.0, foo@npm:2.0.0, foo@npm:3.0.0, foo@npm:4.0.0, foo@npm:5.0.0, foo@npm:6.0.0, and 1 more`); + expect(formatUtils.prettyTruncatedLocatorList(configuration, LOCATORS, 101)).toEqual(`foo@npm:1.0.0, foo@npm:2.0.0, foo@npm:3.0.0, foo@npm:4.0.0, foo@npm:5.0.0, foo@npm:6.0.0, and 1 more.`); }); }); });