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: turbo doesn't work under a symbolic link #988

Closed
wants to merge 4 commits into from
Closed

fix: turbo doesn't work under a symbolic link #988

wants to merge 4 commits into from

Conversation

BlackGlory
Copy link

@BlackGlory BlackGlory commented Apr 3, 2022

The current version of turbo doesn't work under a symbolic link, which is causing a lot of pain.
This patch fixes it, but there may be a better way.

The related issue: #920

@vercel
Copy link

vercel bot commented Apr 3, 2022

@BlackGlory is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

@gsoltis
Copy link
Contributor

gsoltis commented Apr 4, 2022

I think that the right way to do this would be to resolve the root path that turbo uses to its actual path at config time, and then make sure that everything operates off of that.

I don't think I can approve this particular fix without doing a deeper dive into the implications of following symlinks in all cases where this is used. Which, maybe we need, but my sense is that correctly resolving the root path and building off of that is a more correct way to address this particular issue. If you're interested in tackling this, I would start by looking here

@BlackGlory
Copy link
Author

I refactored this patch with filepath.EvalSymlinks, works fine on Linux, not sure if it will cause cross-platform issues (see golang/go#40180).

@jaredpalmer jaredpalmer changed the title fix: https://github.com/vercel/turborepo/issues/920 fix: turbo doesn't work under a symbolic link Apr 7, 2022
@BlackGlory
Copy link
Author

Any progress? I'm still forced to use my own compiled turbo.

@gsoltis
Copy link
Contributor

gsoltis commented Apr 25, 2022

@BlackGlory I think I have the right place to put this code. However, I'd like to understand a bit more about your use-case. In particular, for any references outside of the repository, would you expect them to be relative to the link, or the linked-to location?

For example, if you have a global dependency on a file that is a sibling to the repository, would you expect turbo to be checking the sibling to the actual location? Or checking the sibling to the link? With this change, turbo would check relative to the actual location, not relative to the link.

@BlackGlory
Copy link
Author

BlackGlory commented Apr 26, 2022

@gsoltis Considering this is a simple fix, I think you've expanded the problem, the problem is simply that turbo doesn't work properly when the current directory is a symbolic link.

@BlackGlory
Copy link
Author

Since the turbo team does not seem interested in taking the issue seriously, I have decided to close this PR.

@BlackGlory BlackGlory closed this Apr 28, 2022
@nathanhammond nathanhammond reopened this May 5, 2022
@nathanhammond
Copy link
Contributor

Heya @BlackGlory! We do want to address this but we simply don't have enough hours in the day to get to everything. I've been at Vercel now for a month and we're continuing to bring on more people to increase our bandwidth. Please be patient with us as our time is a limited resource and we are attempting to triage which pieces of the codebase most need our attention.

@gsoltis
Copy link
Contributor

gsoltis commented May 10, 2022

Tentative decision on this is to do our best to emulate how the various package managers do it. Just needs a little research and then we can move forward.

@cedric-marcone
Copy link

Hello guys !
One more use case that breaks without the symlinks support :
=> pm2 deployments are ran inside a symlinked source directory.
This PR would fix pm2 deployments.
Thanks for your help.

@gsoltis
Copy link
Contributor

gsoltis commented May 16, 2022

Apologies for the slowness on this one. I've finished a survey of the various package managers we support, and they are unanimous in resolving symlinks, so that looks like what we'll do as well.

@gsoltis
Copy link
Contributor

gsoltis commented May 16, 2022

I'm going to close this in favor of #1246, which includes EvalSymlinks in GetCwd as well as a test.

@gsoltis gsoltis closed this May 16, 2022
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