Adds versioning to the cache path #2768

arcanis · 2017-02-23T14:50:16Z

Summary

Cf #2035 - sometimes the cache structure changes, and things break.

This commit adds an additional part in the cache path that allows to invalidate previous caches. These old caches will not get removed until after running yarn cache clean, which allows for some easy rollback, just in case.

It should not conflict with the current structure (old cache files will stay in the root cache directory until the next cache clean, but that shouldn't cause any other issue).

Test plan

The main effect is that the cache directory will change a bit:

mael-mbp:yarn mael$ ~/yarn/bin/yarn cache dir
/Users/mael/Library/Caches/Yarn/v1

Note that this variable will also be appended to custom paths:

mael-mbp:yarn mael$ ~/yarn/bin/yarn cache dir --cache-folder=/tmp/cache
/tmp/cache/v1

bestander · 2017-02-23T15:36:41Z

What kind of tests could we implement here?

arcanis · 2017-02-23T15:40:12Z

Not sure; testing it would require changing the constant value, which means that yarn would have to be rebuilt while running the tests :/

bestander · 2017-02-23T15:45:49Z

I think we could mock it with jest easily.

install
check contents of cache
mock version to be 2
install
check the contents of cache

bestander · 2017-02-23T15:58:25Z

Some more ideas:

probably best to make it an integration test in tests/commands/install/integration.js
any test case there has config with cache folder which you can access via config.cacheFolder, it would return something like /var/folders/q1/mbtgtys10s18hb48c78_59wrw6jg9z/T/yarn-skip-integrity-check-1487865032287-0.49457392285559965/.yarn-cache/v1
it is possible to mock a module with Jest but then you need to make sure that config gets reloaded

arcanis · 2017-02-27T20:01:18Z

I digged a bit

any test case there has config with cache folder which you can access via config.cacheFolder, it would return something like /var/folders/q1/mbtgtys10s18hb48c78_59wrw6jg9z/T/yarn-skip-integrity-check-1487865032287-0.49457392285559965/.yarn-cache/v1

I can check that the path ends with the expected version number without issue.

it is possible to mock a module with Jest but then you need to make sure that config gets reloaded

Now things start to get a bit hairy. Deleting the cache with delete require.cache[require.resolve] doesn't really work because jest hijacks the require process, and the actual cache is stored in <jest runtime>._moduleRegistry, which is apparently not accessible from the tests themselves. I tried to use jest.resetModules(), but unfortunately 1/ it isn't recognized by Flow and 2/ it breaks two other tests (because they seem to depend on the request module internal state).

bestander · 2017-02-27T20:11:09Z

It is ok to run the test in sequence (not .concurrent) so that it does not run with other tests in parallel.
However, would it work with ES6 proxy https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy?

arcanis · 2017-02-27T20:21:30Z

It is ok to run the test in sequence (not .concurrent) so that it does not run with other tests in parallel.

Yep, I'm already doing this - the issue is that the two affected tests somehow need the request module state to correctly work, otherwise the request._getAuthedRequests calls return the wrong results. I haven't yet looked why, but I suspect it's because the request used in the tests and the one used by the yarn code aren't the same (since we've reset them previously).

Thinking of it, I found a way to make it work. Running request = require('request') inside my mockConstants function fixes the reference we have, and the remaining tests correctly pass. However, I'm not completely satisfied, since it might introduce other bugs in the tests later that could be hard to debug for anyone not aware of this hack :/

arcanis · 2017-02-27T23:01:52Z

I've just push my test - note that there's still a failure because Flow fails to find the jest.resetModuleRegistry function for some reason.

bestander · 2017-02-27T23:27:48Z

1 test got broken.
Can you replace resetModuleRegistry with resetModules like in facebook/react-native@4aabf4b#diff-c7958b7b0499b2e8e26ceea7b11c19d1?

BTW, jest got just updated to 19.0.0 in master, you may want to rebase

arcanis · 2017-02-27T23:39:57Z

Just rebased. Unfortunately, neither resetModuleRegistry nor resetModules are recognized.

bestander · 2017-02-27T23:44:31Z

You'll probably need to grab https://github.com/flowtype/flow-typed/blob/c17c09b83fd7e71ee4044beb73d7740575ec4975/definitions/npm/jest_v18.x.x/test_jest-v18.x.x.js and replace https://github.com/yarnpkg/yarn/blob/master/flow-typed/npm/jest_v14.0.x.js with it

arcanis · 2017-02-28T00:02:13Z

Should be ok! 👍 I also had to change the file to manually add jasmine.addMatchers and expect.toContainPackage, but now Flow seems happy.

arcanis · 2017-02-28T00:58:14Z

Just noticed that I'm still using opts.cacheFolder and --cache-folder to setup the newly-renamed variable config.cacheRootFolder - maybe they should be renamed as well, but then it wouldn't be backward compatible :/

bestander · 2017-02-28T12:15:53Z

__tests__/commands/install/integration.js

+
+    await mockConstants({CACHE_VERSION: 42}, async (config): Promise<void> => {
+      await cache(config, reporter, {}, ['dir']);
+      assert.ok(!!(JSON.parse(String(inOut.read())) : any).data.match(/\/v42\/?$/));


I think you should add a more high level check that verifies connection between current CACHE_VERSION and what gets installed in node_modules.

E.g.

Install package-a

add an empty file MARKER to the cache folder of package-a

install package-a (with --skip-integrity-check flag) and verify that MARKER was copied

set CACHE_VERSION=42

install package-a (with --skip-integrity-check flag) and verify that MARKER was removed in node_modules

set CACHE_VERSION=1

install package-a (with --skip-integrity-check flag) and verify that MARKER was copied

bestander · 2017-02-28T12:18:12Z

Awesome job!
A test needs to be expanded.
Also note Windows CI, the tests fails on Appveyor

…the cache version

bestander

I think there should be only one cacheFolder in config. The fact that it is versioned is implementation detail that should not leak to other classes

bestander · 2017-02-28T22:14:59Z

src/config.js

@@ -232,10 +236,16 @@ export default class Config {
      networkConcurrency: this.networkConcurrency,
    });

+    this.cacheFolder = String(


After thinking about this I think this.cacheFolder should contain the folder with version.
Otherwise you will have to modify every command that reads cacheFolder from config

bestander · 2017-02-28T22:15:31Z

src/resolvers/registries/npm-resolver.js

-    invariant(this.config.cacheFolder, 'expected packages root');
-    const cacheFolder = path.join(this.config.cacheFolder, scope ? 'npm-' + scope : '');
+    invariant(this.config.versionedCacheFolder, 'expected packages root');
+    const cacheFolder = path.join(this.config.versionedCacheFolder, scope ? 'npm-' + scope : '');


This will work for npm dependencies but not for other resolvers: git etc

bestander · 2017-02-28T22:16:00Z

__tests__/commands/install/integration.js

+    assert.ok(await fs.exists(path.join(config.cwd, 'node_modules', 'is-array', 'yarn.test')));
+
+    await mockConstants({CACHE_VERSION: 42}, async (config): Promise<void> => {
+      const firstReinstall = new Install({}, config, reporter, lockfile);


nit: it is secondReinstall then?

arcanis · 2017-03-02T00:00:56Z

I've just pushed a commit that changes cacheFolder to _cacheRootFolder and versionedCacheFolder to cacheFolder. However, there's still a disparity in the configuration parameters (opts.cacheFolder and --cache-folder are configuring config.cacheRootFolder instead of config.cacheFolder). Isn't it possible to delay merging this PR until the next major release, so that the options can be renamed as well? Or at least merge this PR, but keep an open issue to rename these options when the opportunity arises.

On a similar note, I feel like cacheRootFolder should be public rather than private, since it is required to be able to fetch its value in order to derive a configuration from another (thing we do in the tests) or to just know the root cache path (thing we do in cache clean, but could be used in other contexts - for example if we were to list the cache directories for some reason).

bestander

Great!
Let's see the CI to be green and merge it then

bestander · 2017-03-02T00:09:31Z

I am not sure when we have the next major release :)

The goal is to not have breaking changes unless absolutely necessary, so I'd keep the argument --cache-folder for people to use.
I am worried that if we keep --cache-root-folder public people will get confused which one they are supposed to use. I think --cache-root-folder should not be used by any class except config when it bootstraps

arcanis · 2017-03-02T17:12:59Z

@bestander Should be fine now :) There's an error with Node 4, but I doubt it comes from this PR:

rm : Cannot find path 'C:\projects\yarn\dist\node_modules\core-js\client' because it does not exist.

arcanis force-pushed the cache-key branch from 33bb7fa to d40264d Compare February 23, 2017 15:09

Adds versioning to the cache path

c4b64a4

arcanis force-pushed the cache-key branch from f43e929 to 186513f Compare February 27, 2017 23:39

arcanis force-pushed the cache-key branch from 186513f to 76040a1 Compare February 28, 2017 00:00

Adds a test to check that the cache path is correctly updated

3dec8d4

arcanis force-pushed the cache-key branch from 76040a1 to 3dec8d4 Compare February 28, 2017 00:56

bestander reviewed Feb 28, 2017

View reviewed changes

Maël Nison added 3 commits February 28, 2017 10:57

Renames "cacheFolder" into "versionedCacheFolder"

0e849c2

Adds a test to check that the cache is correctly busted when bumping …

eab68c6

…the cache version

Fixes tests on Windows

c2b2653

bestander reviewed Feb 28, 2017

View reviewed changes

arcanis force-pushed the cache-key branch from cab0c1d to 9c8a8e3 Compare March 2, 2017 00:02

bestander approved these changes Mar 2, 2017

View reviewed changes

arcanis force-pushed the cache-key branch from 9c8a8e3 to b28c475 Compare March 2, 2017 00:44

Changes variable names

275ff7c

arcanis force-pushed the cache-key branch from b28c475 to 275ff7c Compare March 2, 2017 02:02

bestander merged commit ed6b9b9 into yarnpkg:master Mar 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds versioning to the cache path #2768

Adds versioning to the cache path #2768

arcanis commented Feb 23, 2017 •

edited

Loading

bestander commented Feb 23, 2017

arcanis commented Feb 23, 2017 •

edited

Loading

bestander commented Feb 23, 2017

bestander commented Feb 23, 2017

arcanis commented Feb 27, 2017 •

edited

Loading

bestander commented Feb 27, 2017

arcanis commented Feb 27, 2017 •

edited

Loading

arcanis commented Feb 27, 2017

bestander commented Feb 27, 2017

arcanis commented Feb 27, 2017

bestander commented Feb 27, 2017

arcanis commented Feb 28, 2017 •

edited

Loading

arcanis commented Feb 28, 2017 •

edited

Loading

bestander Feb 28, 2017 •

edited

Loading

bestander commented Feb 28, 2017

bestander left a comment

bestander Feb 28, 2017

bestander Feb 28, 2017

bestander Feb 28, 2017

arcanis commented Mar 2, 2017 •

edited

Loading

bestander left a comment

bestander commented Mar 2, 2017

arcanis commented Mar 2, 2017 •

edited

Loading

Adds versioning to the cache path #2768

Adds versioning to the cache path #2768

Conversation

arcanis commented Feb 23, 2017 • edited Loading

bestander commented Feb 23, 2017

arcanis commented Feb 23, 2017 • edited Loading

bestander commented Feb 23, 2017

bestander commented Feb 23, 2017

arcanis commented Feb 27, 2017 • edited Loading

bestander commented Feb 27, 2017

arcanis commented Feb 27, 2017 • edited Loading

arcanis commented Feb 27, 2017

bestander commented Feb 27, 2017

arcanis commented Feb 27, 2017

bestander commented Feb 27, 2017

arcanis commented Feb 28, 2017 • edited Loading

arcanis commented Feb 28, 2017 • edited Loading

bestander Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

bestander commented Feb 28, 2017

bestander left a comment

Choose a reason for hiding this comment

bestander Feb 28, 2017

Choose a reason for hiding this comment

bestander Feb 28, 2017

Choose a reason for hiding this comment

bestander Feb 28, 2017

Choose a reason for hiding this comment

arcanis commented Mar 2, 2017 • edited Loading

bestander left a comment

Choose a reason for hiding this comment

bestander commented Mar 2, 2017

arcanis commented Mar 2, 2017 • edited Loading

arcanis commented Feb 23, 2017 •

edited

Loading

arcanis commented Feb 23, 2017 •

edited

Loading

arcanis commented Feb 27, 2017 •

edited

Loading

arcanis commented Feb 27, 2017 •

edited

Loading

arcanis commented Feb 28, 2017 •

edited

Loading

arcanis commented Feb 28, 2017 •

edited

Loading

bestander Feb 28, 2017 •

edited

Loading

arcanis commented Mar 2, 2017 •

edited

Loading

arcanis commented Mar 2, 2017 •

edited

Loading