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

[FIXES #2096] fix relative symlinks of symlinks #2454

Merged
merged 1 commit into from
Jan 20, 2017

Conversation

stefanpenner
Copy link
Contributor

@stefanpenner stefanpenner commented Jan 15, 2017

Summary

This makes yarn link work when $HOME/.config/yarn/link/ is itself a symlink.


The following commit forces symlinks to be relative: 36d73cc

This isn't ideal, although it enables some portability it breaks others. For example, it is not uncommon for $HOME/.config itself to be be a symlink: $HOME/.config -> $HOME/src/stefanpenner/dotfiles.

Now when running yarn link inside $HOME/src/ember-cli/ember-cli we end up with:

path.relative('/Users/spenner/.config/yarn/link/ember-cli', '/Users/spenner/src/ember-cli/ember-cli');
=> '../../../src/ember-cli/ember-cli'

Which results in the link located at: /Users/spenner/.config/yarn/link/ember-cli pointing to
/Users/spenner/.config/src/ember-cli/ember-cli rather then /Users/spenner/src/ember-cli/ember-cli

This is because /Users/spenner/.config is actually a symlink pointing to /Users/spenner/src/stefanpenner/dotfiles/.config/


This PR mitigates he issue raised in #2096, but reifying the paths via realpathSync before deriving the relative path. This ensures that at the time of linking, the paths are correct. This doesn't fix all issues, e.g. symlinks cannot always be changed after this reification.

Test plan

src/lib/fs.js appears currently untested... if this is an approach you all would like to take I can add some tests.

This makes yarn link work when $HOME/.config/yarn/link/ is itself a symlink.

----

The following commit forces symlinks to be relative:
yarnpkg@36d73cc

This isn't ideal, although it enables some portability it breaks others.
For example, it is not uncommon for `$HOME/.config` itself to be be a
symlink: `$HOME/.config` -> `$HOME/src/stefanpenner/dotfiles`.

Now when running `yarn link` inside `$HOME/src/ember-cli/ember-cli` we
end up with:

```js
path.relative('/Users/spenner/.config/yarn/link/ember-cli',
'/Users/spenner/src/ember-cli/ember-cli');
=> '../../../src/ember-cli/ember-cli'
```

Which results in the link located at:
`/Users/spenner/.config/yarn/link/ember-cli` pointing to
`/Users/spenner/.config/src/ember-cli/ember-cli` rather then
`/Users/spenner/src/ember-cli/ember-cli`

This is because `/Users/spenner/.config` is actually a symlink pointing
to `/Users/spenner/src/stefanpenner/dotfiles/.config/`

---

This PR provides mitigate the issue raised in yarnpkg#2096, but reifying the
paths via `realpathSync` before deriving the relative path. This doesn't
fix all issues, e.g. symlinks cannot be changed after this reification.
@arcanis
Copy link
Member

arcanis commented Aug 28, 2018

I've just stumbled upon this PR, but I don't understand it.

path.relative('/Users/spenner/.config/yarn/link/ember-cli', '/Users/spenner/src/ember-cli/ember-cli');
=> '../../../src/ember-cli/ember-cli'

Which results in the link located at: /Users/spenner/.config/yarn/link/ember-cli pointing to
/Users/spenner/.config/src/ember-cli/ember-cli rather then /Users/spenner/src/ember-cli/ember-cli

I don't understand this last sentence. If /Users/spenner/.config/yarn/link/ember-cli is a symlink to ../../../src/ember-cli/ember-cli, then realpath("/Users/spenner/.config/yarn/link/ember-cli") will yield /Users/spenner/src/ember-cli/ember-cli as it should be, not /Users/spenner/.config/src/ember-cli/ember-cli as you report. The realpath doesn't change anything here.

@stefanpenner Do you recall more details? This realpath doesn't make sense to me and cause issues when one of the path component is invalid for whatever reason.

arcanis added a commit that referenced this pull request Aug 28, 2018
@arcanis arcanis mentioned this pull request Aug 28, 2018
@stefanpenner
Copy link
Contributor Author

I don’t really recall. I believe my pr was simply the best of two evils. IMHO the entire endeavor of changing how fs symlink works is fraught with peril, and should be truely reconsidered

arcanis added a commit that referenced this pull request Sep 5, 2018
* Reverts #2454

* Update fs.js
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.

3 participants