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

yarn overwrites symlinked dependencies #3288

Closed
oliversalzburg opened this issue Apr 30, 2017 · 29 comments · Fixed by #4757
Closed

yarn overwrites symlinked dependencies #3288

oliversalzburg opened this issue Apr 30, 2017 · 29 comments · Fixed by #4757

Comments

@oliversalzburg
Copy link

Do you want to request a feature or report a bug?

Bug

What is the current behavior?
I have a development checkout of the package @company/foo which I made globally available through yarn link. I then linked that packages into my project @company/project.

I also have package @company/bar, which is linked identically into @company/project.

When I now run yarn upgrade @company/bar in the directory of @company/project, yarn will overwrite my local development checkout with data it pulled from the npm registry.

If the current behavior is a bug, please provide the steps to reproduce.

It should be reproducible with any combination of linked packages as outlined above. It seems to me that when yarn says "linking dependencies", it will simply pull some fixed version from the local cache and overwrite my local development snapshot.

What is the expected behavior?
The linked package is not overwritten. npm deals with this: npm/npm@1619871

Please mention your node.js, yarn and operating system version.
6.10.2, 0.23.3, Windows 10

@jonschlinkert
Copy link

I'm also experiencing this. For the second time this week I lost data in 8 or 9 different projects that were symlinked (not to one another, just symlinked globally). Unfortunately, I didn't know it was yarn that caused the data to be lost the first time. I assumed it was the tool I use for git.

jonschlinkert added a commit to enquirer/prompt-base that referenced this issue May 11, 2017
there will be version mismatches and strange commits on a number of repos over the next few days. yarn completely destroyed data in 8 or 9 projects and reverted them to an earlier state.

see yarnpkg/yarn#3288
@bestander
Copy link
Member

Help investigating this is welcome.
I would just start with debugging (putting log statements if don't want to bother with Node.js debugging) package-resolver.js and package-linker.js (this is where it is not supposed to overwrite linked modules https://github.com/yarnpkg/yarn/blob/master/src/package-linker.js#L216)

@oliversalzburg
Copy link
Author

@bestander Awesome. Thanks a lot. I'll get to it :)

@oliversalzburg
Copy link
Author

  1. Situation is very hard to repro. Re-running yarn install never repros the issue. To repro, I have to delete every node_modules folder in my entire tree (rm -rf */node_modules), then run yarn install in a specific project in the tree.
  2. The location pointed out by @bestander does not appear to have any relevance with this issue. The project that is overwritten is contained in the possibleExtraneous set and is correctly detected as a symlink and correctly removed from possibleExtraneous and copyQueue.
  3. copyQueue seems to contain the problematic copy entry in position 0.

@oliversalzburg
Copy link
Author

oliversalzburg commented May 17, 2017

image

To clarify, @fairmanager/core-aws is the project that keeps being overwritten in my source project directory during yarn install operations.
@fairmanager/core-rpc2 is the "trigger" project in which I run yarn install.

@fairmanager/core-aws is a dependency of both @fairmanager/build-utils and @fairmanager/core-rpc2. @fairmanager/build-utils is also a dependency of @fairmanager/core-rpc2. Maybe this triangle of love is too much.

In case it is not clear, it seems to me like the entry in the screenshot above will result in @fairmanager/core-aws being copied to the node_modules of @fairmanager/build-utils. But because that folder is just a symlink to my source project directory, the contents get replaced with the contents of the tarball from the entry.

@jonschlinkert
Copy link

This is a response to #1214 (comment), as it seems like it belongs on this issue.

please open a new one with a scenario how to reproduce it.
Ideally if you could provide a bash script to make it automated.

Before we go too far down the debugging path... can we get some clarification on whether or not this is actually a bug? Because it sure smells like a design decision at the moment.

In particular:

  • Is yarn globally caching local symlinked projects (on purpose)?
  • if so, to what end? is the goal to make symlinked projects deterministic somehow? If this is the case, I can't wrap my brain around this, so any reasoning might help.
  • if yarn does indeed cache symlinked files, does yarn also then attempt to write those cached files somewhere - presumably back to the original symlinked path - upon some trigger event? like a version change, or when a certain command is run, or something else?

@oliversalzburg
Copy link
Author

I'm primarily interested in finding out why this happens for some projects and not others. I have 40 projects in my tree, all depending on each other in some way, but only some projects are affected by this behavior.

To me that indicates that it is a bug, as it should either always preserve the symlinked modules or it should never do it.

It is my understanding that preserving the projects is the intended behavior and it is also my opinion that this is the right choice.

btw, I'm currently assuming that circular dependencies are what's causing this for us, but I was running out of time for today to investigate this assumption.

@jonschlinkert
Copy link

Sorry, allow me to clarify my point and the reason for the questions. If you're saying that it's a bug when yarn overwrites user's project files, yes, that would be an understatement. I don't think there is any confusion about that part.

A couple of points of clarification, as it relates to my own experience with this bug:

  • The pattern I've observed is that projects are being overwritten by earlier versions of the same project. in one case, a v2.x.x project was overwritten by v0.5.2 of the same project. This seems to indicate that yarn is caching symlinked projects. (It seems like .git was even being overwritten in some cases, but I'm not sure about that. I commit often, and I lost everything in projects that I had been working on for 5 days or more)
  • there are no circular dependencies in my setup
  • I have never used yarn link, I'm not sure if that's material.

as it should either always preserve the symlinked modules or it should never do it.

IMO, there is no circumstance in which it would be okay for yarn to delete or create any file or project folder (outside of node_modules).

What I think is happening is this - which might be totally wrong, but I can't debug this at the moment, so I'll take a stab in case it helps:

  1. for some reason yarn is caching symlinked projects as if they were installed
  2. yarn is using the version of the symlinked module in the lockfile (or something like that? not sure)
  3. when some trigger event happens, like version changes in a dependency or yarn upgrade is run, yarn uses the realpath of the symlink to write the cached files (from the originally symlinked module) back to the point of origin of the realpath (e.g. your project files that shouldn't be overwritten).

Which brings me back to my questions and the reason for asking them. I might be wildly wrong in my assertions above, but it seems to fit exactly what's happening. And if it is, I'm having a hard time understanding why yarn would do anything at all with symlinked files. There would be no performance advantage, and there is zero chance of being able to guarantee a deterministic installation with symlinked files, etc.

@oliversalzburg
Copy link
Author

In case it wasn't clear, I was hoping for some support from someone with better knowledge of how yarn should work. But, apparently, nobody does know, which is why we have the issue in the first place.

Very disappointing community.

@jonschlinkert
Copy link

jonschlinkert commented May 30, 2017

why was this closed?!??! this problem is not solved, I'm also participating in this issue.

@bestander can you please reopen this issue?

@bestander bestander reopened this May 30, 2017
@bestander
Copy link
Member

@jonschlinkert, your contribution is welcome, thanks for speaking up.

@oliversalzburg
Copy link
Author

Yeah, your contribution is welcome, but don't expect any help.

@josebalius
Copy link

We are also affected by this. Would love to contribute or help on the issue but need guidance as to where this functionality is found. I'll be browsing the source code either way.

@bestander
Copy link
Member

Welcome, @josebalius!
The reason might be related to package-linker.js where it decides whether a folder is extraneous

@mgmeyers
Copy link

I can confirm this issue and can hopefully provide additional details. I'm running yarn v0.24.5 and node v6.9.5

I have two repos I'm working on: app-common and app-index

cd app-common && yarn link successfully creates a link to app-common
cd app-index && yarn link app-common successfully links app-common in app-index

Now, I want to upgrade app-index's package.json and yarn.lock to use the latest version of app-common

yarn upgrade app-common@https://github.com/url-to/app-common#v1.0.1

What this seems to do is copy https://github.com/url-to/app-common#v1.0.1 into the symlinked directory, overwriting the app-common directory which is outside of app-index/node_modules

This is basically what's described in the previous comments, but to make things even more interesting, if I:

yarn unlink app-common OR rm node_modules/app-common
mkdir node_modules/app-common
yarn upgrade app-common@https://github.com/url-to/app-common#v1.0.1

It still overwrites the app-common directory outside of app-index/node_modules

I think this is what you referenced, @jonschlinkert, with this:

for some reason yarn is caching symlinked projects as if they were installed

So I can confirm that, as well

For now, the work around we've settled on is:

yarn remove app-common
yarn add https://github.com/url-to/app-common#v1.0.1

@marijnh
Copy link

marijnh commented Sep 6, 2017

I just lost some work through this, and since I rely on symlinking to work on a multi-module system it's starting to make me fear running yarn at all. Any chance it can get some more attention?

@willviles
Copy link

Very frustrating issue! Would massively appreciate a fix for this - it's pretty debilitating when trying to work with linked modules.

@bestander
Copy link
Member

The issue is somewhere in package-linker.js, there are repro steps.
It is easy to check out the repo and put some console.log statements to see why things are not working.
Why no one has not sent a PR yet?

@gandazgul
Copy link
Contributor

gandazgul commented Oct 11, 2017

This is back in 1.2.0. Repro steps:

$ rm -rf node_modules
$ yarn
$ yarn link some-module (symlink created in node_modules)
Update a version in package.json to force yarn to run install again
$ yarn
the symlink is now gone.

dupe here: #4603

@pvdz
Copy link

pvdz commented Oct 13, 2017

I've tried to create a proof of concept repro script:

https://github.com/qfox/yarn/blob/symlink-clobbered-by-install/__tests__/fixtures/install/install-overriding-symlinks/unique.sh

It will create a temporary mock package, link it, then write a package.json to the current dir with a dep to left-pad@1.1.1 (can't seem to use file: for this). It calls yarn to install that and afterwards yarn link <temp pkg> to link the fake package we generated earlier. A ls -al node_modules to show that the package is linked.

Next it bumps the version of left-pad to 1.1.2 and calls yarn to proc an update. After this update the node_modules no longer contains the symlink.

My question to participants of this thread is whether this is in fact the proper repro causing the problem in this ticket. The problem can have different parameters that are subtle though important to the problem so that's why I'm asking.

On a side note; how does this lose data? I can understand certain cases in which this causes problems but for as far as yarn is concerned it only seems to destroy the symlink, not the actual files. Is it because the link was shadowing a package that's also in the package.json which then gets overridden? That'd be an example of a subtle difference that's important.

Here is a version where the linked package shadows a package in the package.json. I suppose that's one way of losing data depending on how your editor is setup.

https://github.com/qfox/yarn/blob/symlink-clobbered-by-install/__tests__/fixtures/install/install-overriding-symlinks/shadowed.sh

@pvdz
Copy link

pvdz commented Oct 13, 2017

Note that at 1.2.0 the behavior is that the symlinked dir is replaced in both cases of the test script above.

@gandazgul
Copy link
Contributor

gandazgul commented Oct 22, 2017

@qfox does that mean is replacing the symlinks by design?

@gandazgul
Copy link
Contributor

gandazgul commented Oct 22, 2017

I found where the issue is, and what the problem is but I'm not sure how to solve it.

@bestander was right is in package-linker.js. On line 380 await fs.unlink(loc); yarn unlinks the only left possibleExtraneous which is the linked package. Which means the logic to remove it from the list is not working.

Problem:

On line 302: it loops through linkedModules and fills linkTargets with the name and path of each one. fs.readlink(entryPath). Then on line 334 there's an if statement that compares linkTargets.get(pkgName) with fs.readLink(node_modules/pkg) but these paths are not the same in every case I set up.

The listing of the .config directory is:

$ ls -l /Users/gandazgul/.config/yarn/link/pkg
lrwxr-xr-x 1 gandazgul staff 26B Oct 22 15:32 /Users/gandazgul/.config/yarn/link/pkg -> ../../../Documents/web/pkg

While the listing of ./node_modules is:
lrwxr-xr-x 1 gandazgul staff 9B Oct 22 15:33 pkg -> ../../pkg

As you can see yarn link or OSX is making the symlinks differently which makes that if statement on line 334 return false every time.

@gandazgul
Copy link
Contributor

Ok. I resolved it by using fs.realpath instead of readlink which returns the entire path of the symlink. If both are the same then this is a yarn linked module and should not be removed. I tested locally with my repro scenario both installing and upgrading. I'll submit a PR.

@gandazgul
Copy link
Contributor

gandazgul commented Oct 23, 2017

I can't seem to write a test that will break with this issue. I have the issue fixed but would love some help with the unit test.

The test runs fine but doesn't trigger the error condition. Both paths are identical when run through the test but obviously not when run in a more real world scenario.

Here is the PR: #4757. Help is appreciated.

@bestander
Copy link
Member

@gandazgul, I love it how you debugged the code, thanks a lot for contributing!

@gandazgul
Copy link
Contributor

@bestander :) thanks.

@pvdz
Copy link

pvdz commented Oct 24, 2017

@gandazgul;

does that mean is replacing the symlinks by design?

I don't know :) That was before my time.

I found where the issue is, and what the problem is

Awesome!

@theandrewlane
Copy link
Contributor

@gandazgul I'm using your branch on my local for now, thank you :) 💯

@BYK BYK closed this as completed in #4757 Oct 26, 2017
BYK pushed a commit that referenced this issue Oct 26, 2017
**Summary**

 Actual fix: changed fs.readlink to fs.realpath when checking if a symlink is a linked dependency in package-linker.js This fixes yarn removing linked deps when installing or updating.

Fixes #3288, fixes #4770, fixes #4635, fixes #4603.

Potential fix for #3202.

**Test plan**

See #3288 (comment) for repro steps.
See #3288 (comment) for my explanation of the problem.

With a real world test scenario this works, but I'm unable to have it break from a unit test. I added a test in the integration suite but with the bug added back in it still passes because both generated paths are identical. I would like some help with the unit test.
joaolucasl pushed a commit to joaolucasl/yarn that referenced this issue Oct 27, 2017
…#4757)

**Summary**

 Actual fix: changed fs.readlink to fs.realpath when checking if a symlink is a linked dependency in package-linker.js This fixes yarn removing linked deps when installing or updating.

Fixes yarnpkg#3288, fixes yarnpkg#4770, fixes yarnpkg#4635, fixes yarnpkg#4603.

Potential fix for yarnpkg#3202.

**Test plan**

See yarnpkg#3288 (comment) for repro steps.
See yarnpkg#3288 (comment) for my explanation of the problem.

With a real world test scenario this works, but I'm unable to have it break from a unit test. I added a test in the integration suite but with the bug added back in it still passes because both generated paths are identical. I would like some help with the unit test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants