Add hash to cache path for non-NPM packages

[mourner]

Summary

Currently, only packages resolved to NPM registry have their hash appended to cache directory name. Other packages, such as those with GitHub URL, get cached by their name + version rather than actual content. This leads to a bug where even if you update the GitHub url in package.json (e.g. different GitHub commit hash), it is never updated on yarn install unless you clean the cached folders for the package along with node_modules. This is a huge pain for teams that depend on GitHub urls in dependencies.

Should address #1171, #1893, #1087, #1280, #1278, #1017 #1573 #2280. I'm not sure it fully fixes all the mentioned issues, so it's a work in progress in need of a discussion and thorough testing, but it should be a step in the right direction.

Test plan

This is my first dive into Yarn codebase and I haven't yet figured out a way to test this — currently npm test fails for me locally on master with "process exited unexpectedly error" in commands/remove.js without much detail.

cc @kittens @jfirebaugh @jordanburke @ronag @wmertens @tamird @mgutz @panrafal @omnidan

[sebmck]

Where is info.hash coming from? This is the package manifest.

[mourner]

@kittens it's saved to the metadata file here. The only occasion I found where info.hash is not present is when the file resolver is used, but that should be OK.

[bestander]

@mourner, could you add a test for the new behavior and the issues it addresses, please?

[bestander]

• need tests to ensure it does not regress later

[mourner]

Sure, I just wanted to get some early feedback before I start working on tests. Will do.

[bestander]

It does make some sense, we have some confusion in the system where the hashes come from.
Let's see how it works with a few tests, looks like it does not break any existing ones

[benmosher]

@mourner want any help with tests?

[mourner]

@benmosher sure! I was going to take a look next week, but if you're willing to help out with the PR, you're very welcome!

[mourner]

@bestander @kittens updated the PR with a simpler fix. It leaves npm-resolver cache paths the same while adding hashes to tarball and hosted-git (and derived) resolvers, fixing numerious issues with different commits of the same package version colliding in the cache folder.

Note that the test failures are the same as the ones in the current master, for which I'm not responsible.

I'm still not able to add tests for the change though because package resolver tests were disabled by @kittens in b59fd78 and I don't have enough knowledge to reenable them and fix the failures (unrelated to the changes in this PR).

[bestander]

@mourner, I've enabled most of the tests in #2373.
Also master branch should be more stable now, could you please rebase?

[mourner]

@bestander thanks! Rebased on top of master. I'll wait for #2373 to land before attempting to add any tests.

@benmosher are you still up for help on the tests?

[mourner]

@bestander finally added some tests based on the reenabled resolver tests! Hopefully we can land this in a Yarn release soon.

Also, I used this Yarn fork locally and it worked perfectly when doing a difficult git bisect in a project with tons of git-hashed dependencies (mapbox-gl-js), whereas it would be impossible in both stable Yarn (due to the bug) and NPM (due to unbearable install times).

[mourner]

@jordanburke @ronag @wmertens @tamird @mgutz @panrafal @omnidan hey, could you please confirm that this PR fixes your corresponding opened issues (listed above) so that we could close them when this PR lands?

[mourner]

Found another duplicate for the problem: #2280, cc @redgenie ^^^

[bestander]

@mourner, great job!
Thanks for making an extra effort with the test!

[mourner]

Yay, thanks for merging!

[tamird]

I believe this has regressed; currently a github dependency is not updated on disk after a yarn upgrade without a cache clean.

EDIT: yarn 0.21.3, macOS 10.12.3.

[mourner]

@tamird I think yarn upgrade behavior would be a different issue. This one is about linking wrong versions from cache (other than the ones specified in yarn.lock).

[tamird]

Yes, yarn upgrade does the right thing, but the subsequent yarn install continues to use the old version (not the one which is now specified in yarn.lock).

[mourner]

@tamird can you check if the relevant resolver tests still pass?

[tamird]

What do you mean? Does yarn not have CI?

[mourner]

@tamird it does but it's quite useless at the moment because all recent master commits have failed builds, so I don't know whether the resolver tests actually caught any regression or not. 😕

[tamird]

Yes, it seems the resolver tests do pass.