Skip to content

Commit a8ea928

Browse files
authored
Fixes global cache (#5739)
**What's the problem this PR addresses?** The cache fix merged in #5730 had a bug that caused the global cache to be skipped when a package had no checksum - which happens when there are no lockfile in the project. This was detected by the perf regression visible on the dashboard: <img width="1381" alt="image" src="https://github.com/yarnpkg/berry/assets/1037931/11aed9d5-7946-418d-806a-6df78aee4734"> **How did you fix it?** I fixed the condition (it was "use the cache if there's a checksum and it's compatible", it should have been "use the cache if there's no checksum, or it's a compatible one") and added tests to avoid further regressions. **Checklist** <!--- Don't worry if you miss something, chores are automatically tested. --> <!--- This checklist exists to help you remember doing the chores when you submit a PR. --> <!--- Put an `x` in all the boxes that apply. --> - [x] I have read the [Contributing Guide](https://yarnpkg.com/advanced/contributing). <!-- See https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released for more details. --> <!-- Check with `yarn version check` and fix with `yarn version check -i` --> - [x] I have set the packages that need to be released for my changes to be effective. <!-- The "Testing chores" workflow validates that your PR follows our guidelines. --> <!-- If it doesn't pass, click on it to see details as to what your PR might be missing. --> - [x] I will check that all automated PR checks pass before the PR gets reviewed.
1 parent 9c3dc22 commit a8ea928

File tree

6 files changed

+227
-162
lines changed

6 files changed

+227
-162
lines changed

.yarn/versions/8b4f2cd6.yml

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
releases:
2+
"@yarnpkg/cli": patch
3+
"@yarnpkg/core": patch
4+
"@yarnpkg/plugin-essentials": patch
5+
6+
declined:
7+
- "@yarnpkg/plugin-compat"
8+
- "@yarnpkg/plugin-constraints"
9+
- "@yarnpkg/plugin-dlx"
10+
- "@yarnpkg/plugin-exec"
11+
- "@yarnpkg/plugin-file"
12+
- "@yarnpkg/plugin-git"
13+
- "@yarnpkg/plugin-github"
14+
- "@yarnpkg/plugin-http"
15+
- "@yarnpkg/plugin-init"
16+
- "@yarnpkg/plugin-interactive-tools"
17+
- "@yarnpkg/plugin-link"
18+
- "@yarnpkg/plugin-nm"
19+
- "@yarnpkg/plugin-npm"
20+
- "@yarnpkg/plugin-npm-cli"
21+
- "@yarnpkg/plugin-pack"
22+
- "@yarnpkg/plugin-patch"
23+
- "@yarnpkg/plugin-pnp"
24+
- "@yarnpkg/plugin-pnpm"
25+
- "@yarnpkg/plugin-stage"
26+
- "@yarnpkg/plugin-typescript"
27+
- "@yarnpkg/plugin-version"
28+
- "@yarnpkg/plugin-workspace-tools"
29+
- "@yarnpkg/builder"
30+
- "@yarnpkg/doctor"
31+
- "@yarnpkg/extensions"
32+
- "@yarnpkg/nm"
33+
- "@yarnpkg/pnpify"
34+
- "@yarnpkg/sdks"

packages/acceptance-tests/pkg-tests-specs/sources/features/cache.test.ts

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import {Filename, ppath, xfs} from '@yarnpkg/fslib';
2+
import {tests} from 'pkg-tests-core';
23

34
describe(`Cache`, () => {
45
test(
@@ -12,17 +13,35 @@ describe(`Cache`, () => {
1213
}),
1314
);
1415

15-
test(
16-
`it should make packages installable even without network`,
17-
makeTemporaryEnv({
18-
dependencies: {
19-
[`no-deps`]: `1.0.0`,
20-
},
21-
}, async ({path, run, source}) => {
22-
await run(`install`);
23-
await run(`install`, {enableNetwork: false});
24-
}),
25-
);
16+
for (const enableGlobalCache of [false, true]) {
17+
for (const withLockfile of [false, true]) {
18+
test(
19+
`it should make packages installable even without network (${enableGlobalCache ? `global` : `local`} cache, ${withLockfile ? `with` : `without`} lockfile)`,
20+
makeTemporaryEnv({
21+
dependencies: {
22+
[`no-deps`]: `1.0.0`,
23+
},
24+
}, {
25+
enableGlobalCache,
26+
}, async ({path, run, source}) => {
27+
await run(`install`);
28+
29+
if (!withLockfile)
30+
await xfs.removePromise(ppath.join(path, Filename.lockfile));
31+
32+
const requests = await tests.startRegistryRecording(async () => {
33+
await run(`install`);
34+
});
35+
36+
if (withLockfile) {
37+
expect(requests).toHaveLength(0);
38+
} else {
39+
expect(requests.filter(req => req.type !== tests.RequestType.PackageInfo)).toHaveLength(0);
40+
}
41+
}),
42+
);
43+
}
44+
}
2645

2746
test(
2847
`it should detect when the files checksum is incorrect`,

packages/plugin-essentials/sources/commands/info.ts

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -255,18 +255,13 @@ export default class InfoCommand extends BaseCommand {
255255
if (!extra.has(`cache`))
256256
return;
257257

258-
const cacheOptions = {
259-
mockedPackages: project.disabledLocators,
260-
unstablePackages: project.conditionalLocators,
261-
};
262-
263258
const checksum = project.storedChecksums.get(pkg.locatorHash) ?? null;
264-
const cachePath = cache.getLocatorPath(pkg, checksum, cacheOptions);
259+
const cachePath = cache.getLocatorPath(pkg, checksum);
265260

266261
let stat;
267262
if (cachePath !== null) {
268263
try {
269-
stat = xfs.statSync(cachePath);
264+
stat = await xfs.statPromise(cachePath);
270265
} catch {}
271266
}
272267

packages/yarnpkg-core/sources/Cache.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ export class Cache {
176176
return true;
177177
}
178178

179-
getLocatorPath(locator: Locator, expectedChecksum: string | null, opts: CacheOptions = {}) {
179+
getLocatorPath(locator: Locator, expectedChecksum: string | null) {
180180
// When using the global cache we want the archives to be named as per
181181
// the cache key rather than the hash, as otherwise we wouldn't be able
182182
// to find them if we didn't have the hash (which is the case when adding
@@ -365,7 +365,7 @@ export class Cache {
365365
isColdHit: true,
366366
});
367367

368-
const cachePath = this.getLocatorPath(locator, checksum, opts);
368+
const cachePath = this.getLocatorPath(locator, checksum);
369369
const copyProcess: Array<() => Promise<void>> = [];
370370

371371
// Copy the package into the mirror
@@ -403,8 +403,8 @@ export class Cache {
403403
const mutexedLoad = async () => {
404404
const isUnstablePackage = opts.unstablePackages?.has(locator.locatorHash);
405405

406-
const tentativeCachePath = isUnstablePackage || expectedChecksum && this.isChecksumCompatible(expectedChecksum)
407-
? this.getLocatorPath(locator, expectedChecksum, opts)
406+
const tentativeCachePath = isUnstablePackage || !expectedChecksum || this.isChecksumCompatible(expectedChecksum)
407+
? this.getLocatorPath(locator, expectedChecksum)
408408
: null;
409409

410410
const cacheFileExists = tentativeCachePath !== null

packages/yarnpkg-core/sources/Project.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1136,10 +1136,10 @@ export class Project {
11361136
const locator = this.storedPackages.get(locatorHash);
11371137
const checksum = this.storedChecksums.get(locatorHash) ?? null;
11381138

1139-
const p = cache.getLocatorPath(locator!, checksum, cacheOptions);
1140-
const stat = p ? await xfs.statPromise(p) : null;
1139+
const p = cache.getLocatorPath(locator!, checksum);
1140+
const stat = await xfs.statPromise(p);
11411141

1142-
return stat?.size ?? 0;
1142+
return stat.size;
11431143
}));
11441144

11451145
const finalSizeChange = addedSizes.reduce((sum, size) => sum + size, 0) - (cleanInfo?.size ?? 0);

0 commit comments

Comments
 (0)