Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Skip setting hash if hash was not set previously #1871

Merged
merged 7 commits into from
Nov 16, 2016
Merged

Skip setting hash if hash was not set previously #1871

merged 7 commits into from
Nov 16, 2016

Conversation

thomaschaaf
Copy link
Contributor

@thomaschaaf thomaschaaf commented Nov 15, 2016 •
edited
Loading

Summary

This should fix #1834 where yarn had resolved the hosted-git-packages without a hash and downloaded them to the cache folder <Packagename>-<Version>. The tarball fetcher would then add the hash after the download completed resulting in the linker believing that the Package could be found in the cache folder <Packagename>-<Version>-<Hash>.

This ba5a203 commit seems to have added the logic behind tarball hashes.

Test plan

I added end to end tests and some jest tests
yarn add mulesoft/api-console.git#master would not work. Now works.

@bestander
Copy link
Member

I would appreciate a unit test here.
It should be a matter of adding a test similar to this https://github.com/yarnpkg/yarn/blob/master/__tests__/commands/add.js#L554 that just checks that dependency gets installed correctly.

@bestander bestander mentioned this pull request Nov 16, 2016
Closed
16 tasks
@wyze wyze mentioned this pull request Nov 16, 2016
Closed
@bestander
Copy link
Member

Can this be done with the jest test?
It should be easier to maintain them within framework

@thomaschaaf
Copy link
Contributor Author

I will add unit tests now :) Then we can hopefully merge it very soon

@bestander
Copy link
Member

Thanks, @thomaschaaf, I'll be ready to merge it at once.

wyze
wyze suggested changes Nov 16, 2016
View reviewed changes
src/package-fetcher.js
@@ -88,7 +88,11 @@ export default class PackageFetcher {
newPkg = res.package;

// update with new remote
ref.remote.hash = res.hash;
// but only if there was a hash previously as the tarball fetcher does not provide a hash.
if(ref.remote.hash) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor nit to add a space here, if( -> if (.

@bestander
Copy link
Member

.tgz files got corrupted

@thomaschaaf
Copy link
Contributor Author

I don't know if this is the correct way to write jest tests as this makes a real http request to github and downloads the files but I did test the unit tests and when I reverted the change the unit tests failed.

bestander
bestander reviewed Nov 16, 2016
View reviewed changes
Copy link
Member

@bestander bestander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks a lot @thomaschaaf!
Let's just remove it from e2e scenario and I'll merge it

end_to_end_tests/README.md
@@ -3,3 +3,4 @@ This directory contains end-to-end tests for Yarn. The tests perform the followi
1. Spin up a fresh Docker container
2. Install Yarn through the package repository
3. Run `yarn add react` in a test directory
4. Run `yarn add 'substack/node-mkdirp#master'` in a test directory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's remove this now that it is covered with unit test.

end_to_end_tests/data/run-yarn-test.sh
@@ -17,3 +17,4 @@ mkdir yarntest
cd yarntest
echo {} > package.json
yarn add react || fail_with_log
yarn add 'substack/node-mkdirp#master' || fail_with_log
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this

@wyze
Copy link
Member

wyze commented Nov 16, 2016

Should there be some assertions in the unit test to ensure the module installed in the node_modules folder?

@bestander
Copy link
Member

Yes, ideally there should be assertations.
Although install/add commands run check afterwards automatically, it would fail without the fix in place.

@bestander
Copy link
Member

Does the test fail without the fix, @thomaschaaf ?

@thomaschaaf
Copy link
Contributor Author

@bestander yes the tests fail if it is not fixed

@bestander bestander merged commit 1180f0f into yarnpkg:master Nov 16, 2016
@thomaschaaf thomaschaaf deleted the fix-hosted-git-fetcher branch November 16, 2016 20:17
@bestander
Copy link
Member

I'll do 0.17.3 in 30 min

@wyze
Copy link
Member

wyze commented Nov 16, 2016

Thanks for the iterations @thomaschaaf!

@emveeoh
Copy link

emveeoh commented Nov 16, 2016

thanks for the quick fix everyone! :)

bestander pushed a commit that referenced this pull request Nov 16, 2016
@thomaschaaf @bestander
Skip setting hash if hash was not set previously (#1871)
ca6a3d2 
* Skip setting hash if hash was not set previously

* Add end to end test for installing a github package

* Update README.md

* Fix eslint

* Reverse accidental fixture changes

* Add unit tests

* No end to end tests for github installs

Conflicts:
	src/cli/commands/install.js
bestander added a commit that referenced this pull request Nov 16, 2016
@bestander
Revert "Skip setting hash if hash was not set previously (#1871)"
f3239d0 
This reverts commit ca6a3d2.
bestander pushed a commit that referenced this pull request Nov 16, 2016
@thomaschaaf @bestander
Skip setting hash if hash was not set previously (#1871)
202d595 
* Skip setting hash if hash was not set previously

* Add end to end test for installing a github package

* Update README.md

* Fix eslint

* Reverse accidental fixture changes

* Add unit tests

* No end to end tests for github installs

Conflicts:
	src/cli/commands/install.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bestander bestander bestander left review comments

@wyze wyze wyze approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"No such file file or directory, lstat" for Yarn cache during package install on Windows
4 participants
@thomaschaaf @bestander @wyze @emveeoh