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.

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

fix errors thrown if symlink source does not exist #2744

Merged
merged 1 commit into from
Feb 27, 2017

Conversation

canufeel
Copy link
Contributor

@canufeel canufeel commented Feb 21, 2017

Summary
#2454 introduced a regression as during install symlink sources wouldn't always exist at the time of symlink creation so as fs.realpathSync checks if the path actually exist the ENOENT error might be thrown here.

This PR makes sure we have more defensive approach by checking that symlink source exists before trying to derive absolute symlink path which might throw. So if source doesn't exist we would stick to more conservative relative symlinks otherwise where symlink source itself is not a requirement at least until the symlink is used.

Fixes #2714

@canufeel canufeel closed this Feb 21, 2017
@canufeel canufeel reopened this Feb 21, 2017
@canufeel canufeel closed this Feb 23, 2017
@canufeel canufeel reopened this Feb 23, 2017
@SomeoneWeird
Copy link

Someone able to look at this?

@stefanpenner
Copy link
Contributor

So if source doesn't exist we would stick to more conservative relative symlinks otherwise where symlink source itself is not a requirement at least until the symlink is used.

Ya, I honestly I think yarn's custom symlink stuff shouldn't try to do anything funky here at all. #2454 restores some functionality, but the eager reification causes the issue this PR is aiming to mitigate.

This PR does mitigate some additional scenarios, I do worry that it makes the behavior even less predictable. That trade-off may be fine though.

@canufeel
Copy link
Contributor Author

This PR does mitigate some additional scenarios, I do worry that it makes the behavior even less predictable.

@stefanpenner This is indeed true. The proper solution seems to be using only one symlink type i.e. absolute path symlinks. Unfortunately i didn't have time to investigate in detail why the symlink problem happens now in the first place. It's either because of race-condition of some kind or because of the way yarn flattens the dependencies. In my experience this problem shows up with symlinks which are multiple level deep in app/node_modules/some_dep/node_modules/....

If former is the case then probably the proper way to go would be to delay symlink creation till the point when all deps are already installed project-wise. If latter is true then some kind of more sophisticated symlink "correction" and resolve mechanism is required here.

Any ideas on that are welcome.

@stefanpenner
Copy link
Contributor

stefanpenner commented Feb 25, 2017

@canufeel the current behavior is largely the result yarn trying to make symlinks relative: 36d73cc and then working around what that causes when applied to all symlinks yarn creates.

I suspect either yarn should go back to regular symlinks, or yarn should consider using several distinct methods with the behavior that scenario would prefer.

@stefanpenner
Copy link
Contributor

@canufeel if the yarn folks would prefer to keep the existing behavior (rather then restoring symlinks to work as they are expected to do), I do believe your PR improves the current state and should be considered.

@bestander
Copy link
Member

Thanks for @canufeel and @stefanpenner for the contribution.
The relative symlink behavior is needed to make caches work correctly, reverting to absolute symlinks could open other issues in this area.
I'll merge this and add a test.

@bestander
Copy link
Member

cc me if anyone finds a related issue in the future

@bestander bestander merged commit b409e18 into yarnpkg:master Feb 27, 2017
@bestander
Copy link
Member

Also will patch 0.20 and 0.21

bestander added a commit to bestander/yarn that referenced this pull request Feb 27, 2017
bestander added a commit that referenced this pull request Feb 27, 2017
* added test for #2744

* fix lint

* updated network caches

* more tests optimization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants