Handling symlinks as project root #641

ibrasho · 2017-05-24T21:25:52Z

This started as an attempt to clarify the FAQ update done by @brianstarke in #247.
I went to the code to understand how handling symlinks as project root is implemented currently and came to the following conclusion:

If the project root (either a regular directory or symlink) is not within a GOPATH, an error will be returned since context.NewContext will return an error if the cwd is not within a GOPATH before context.resolveProjectRoot is reached where symlinks are handled currently. Tests pass currently since they don't go through main like a regular cmd will.

I've tried to rewrite the FAQ answer to provide some clarification:

I opted to use project root instead of cwd, since it's more consistent (running dep ensure in a project subdirectory will traverse up to the project root regardless of the cwd).
I've updated the implementation to reflect the answer below. Need @sdboyer & @brianstarke to confirm if my understanding and implementation is correct.

How does dep behave if the project root is a symbolic link?

because we're not crazy people who delight in inviting chaos into our lives, we need to work within one GOPATH at a time.
-@sdboyer in #247

Out of convenience, one might create a symlink to a directory within their GOPATH/src, e.g. ln -s ~/go/src/github.com/user/awesome-project ~/Code/awesome-project.
When dep is invoked with a project root that is a symlink, it will be resolved according to the following rules:

If the symlink is outside GOPATH and links to a directory within a GOPATH, or vice versa, then dep will choose whichever path is within GOPATH.

If the symlink is within a GOPATH and the resolved path is within a different GOPATH, then an error is returned.

If both the symlink and the resolved path are in the same GOPATH, then an error is returned.

If both the symlink and the resolved path are not in GOPATH, then an error is returned

This is the only symbolic link support that dep really intends to provide. In keeping with the general practices of the go tool, dep tends to either ignore symlinks (when walking) or copy the symlink itself, depending on the filesystem operation being performed.

Fixes #218.

sdboyer · 2017-05-25T01:39:42Z

Hmm - it seems we've regressed then, because this definitely did work when we brought in #247. Clearly our tests aren't properly covering this case.

Instead of updating the FAQ (which reflects how it's supposed to be), let's fix the code.

ibrasho · 2017-05-25T07:35:31Z

As of now, running dep where the project root is a symlink outside GOPATH will print project not in GOPATH and exit. This true for the last commit in #247 (753c8dd).
The error is returned when NewContext() is called (in cmd/dep/main.go) before reaching ResolveProjectRoot(). The tests were covering TestResolveProjectRoot() and that works as expected.

@sdboyer The only change I made to the FAQ (besides trying to clarify it) is the following change:

If neither path is within a GOPATH, dep produces an error.

to

If both the symlink and the resolved path are not in GOPATH, then an error is returned

This doesn't make sense, since determining the GOPATH for this case is not possible (unless we assume it's the cwd regardless). I've updated the FAQ and implementation to reflect this.

Let me know if this should be reverted.

Signed-off-by: Ibrahim AshShohail <[email protected]>

…ctRootAndGoPath() and add detectGoPath() Signed-off-by: Ibrahim AshShohail <[email protected]>

Signed-off-by: Ibrahim AshShohail <[email protected]>

sdboyer

just one little nit right now. unfortunately, i'm out of reviewing juice tonight 😢

sdboyer · 2017-06-06T03:06:34Z

context.go

+// If path is a symlink within a GOPATH, an error is returned.
+// If both path and the directory it resolves to are within the same GOPATH.
+// If path and the directory it resolves to are each within a different GOPATH.
+func (c *Ctx) ResolveProjectRootAndGoPath(path string) (string, string, error) {


s/GoPath/GOPATH/

…GOPATH() Edit letter case of: * detectGoPath() to detectGOPATH() * ResolveProjectRootAndGoPath() to ResolveProjectRootAndGOPATH() Signed-off-by: Ibrahim AshShohail <[email protected]>

Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho · 2017-06-10T04:07:25Z

This will need more changes since #673 is merged. 😁
I'll update it soon.

ofpiyush · 2017-06-11T11:49:11Z

@ibrasho I tried your branch with project root as a symlink to a directory outside GOPATH and got all dirs lacked any go code

A little digging revealed this line to be the culprit.

Maybe have a way to handle symlinked directories in pkgtree as well?
I haven't dug very deep yet, but I can lend a hand on weekends if that helps :)

sdboyer · 2017-06-12T01:58:43Z

Maybe have a way to handle symlinked directories in pkgtree as well?

@ofpiyush unfortunately, we don't plan on supporting this. It makes clear, consistent behavior vastly more difficult. There have been PRs in the past that attempted it - often going on for weeks, and ultimately resulting in unexpected behaviors that we didn't initially anticipate.

sdboyer

@ibrasho i'm sorry this keeps sliding - tbh, i have just put so much time into reviewing mind-numbing symlink PRs already that my mind seems to be resisting grokking yet one more. But, I've dug in a bit.

The direction here looks fundamentally correct. The most important point to make, really, is that #247 (comment) remains the closest thing to a spec for how this behavior should work that we have. If something deviates from that, then it's a bug. And this seems to bring us closer, which is good.

but yes, as you note, given #673, this'll need some more refactoring 😞

sdboyer · 2017-06-12T01:47:03Z

FAQ.md

+- If the symlink is outside `GOPATH` and links to a directory within a `GOPATH`, or vice versa, then `dep` will choose whichever path is within `GOPATH`.
+- If the symlink is within a `GOPATH` and the resolved path is within a *different* `GOPATH`, then an error is thrown.
+- If both the symlink and the resolved path are in the same `GOPATH`, then an error is thrown.
+- If both the symlink and the resolved path are not in a `GOPATH`, then an error is thrown.


Sentence construction thing:

"If neither the symlink nor the resolved path are in a GOPATH, then an error is thrown".

sdboyer · 2017-06-12T01:56:30Z

internal/fs/fs.go

+
+// ResolvePath resolves the path of the given symlink. If the given path is not a symlink,
+// then it returns the same path.
+func ResolvePath(path string) (string, error) {


How is this func different from just calling filepath.EvalSymlinks() directly?

It's totally useless. 😅
I just noticed I already call IsSymlink() before ResolvePath().

I'll remove it when I've time to refactor the PR.

ofpiyush · 2017-06-12T06:00:00Z

@sdboyer Go tool chain currently works fine with that setup. And google cloud builder necessitates that structure, check #743

sdboyer · 2017-06-12T15:56:04Z

@ofpiyush

Go tool chain currently works fine with that setup.

For this particular case, the toolchain has less complex requirements than dep does. The toolchain is entirely package-oriented right now; we have to be project-oriented (trees of packages).

And google cloud builder necessitates that structure, check #743

Yep, I've seen #743. Without having dug too far into what cloud builder is doing, it does not appear to rely on arbitrary symlinking within package trees - only symlinked roots, which is what we're addressing here.

I understand that broader symlinking support can be helpful for certain cases, but we can't have it unless we can contain the complexity (not to mention potential attack vectors) it introduces in the system. Feel free to look through some of the past wreckage: sdboyer/gps#157 sdboyer/gps#177

ofpiyush · 2017-06-12T17:23:28Z

@sdboyer

only symlinked roots, which is what we're addressing here.

I think there is some miscommunication here. I am talking about this case in particular.

If the symlink is outside GOPATH and links to a directory within a GOPATH, or vice versa, then dep will choose whichever path is within GOPATH.

Case:
GOPATH=/workspace/gopath
code_directory= /workspace
project_root = /workspace/gopath/github.com/private/repository -> /workspace

In this case, I get the error all dirs lacked any go code. Isn't this is the vice-versa case that is supposed to work?

For this particular case, the toolchain has less complex requirements than dep does. The toolchain is entirely package-oriented right now; we have to be project-oriented (trees of packages).

As an end user, I generally expect all tools to behave in a consistent way with each other, even if that way might not be something I agree with. It's pretty sad that I hit that one case where one tool has to diverge from the norm :-/

Feel free to look through some of the past wreckage: sdboyer/gps#157 sdboyer/gps#177

I will definitely check it out through this weekend. Though, I suspect I am not going to find the golden fix if better people have tried and given up. Worth a shot anyway. ☮️

Edit: ordering based on importance

sdboyer · 2017-06-12T18:01:37Z

In this case, I get the error all dirs lacked any go code. Isn't this is the vice-versa case that is supposed to work?

Yes, and it's something that either this PR, or a follow-up, should fix.

As an end user, I generally expect all tools to behave in a consistent way with each other, even if that way might not be something I agree with. It's pretty sad that I hit that one case where one tool has to diverge from the norm :-/

Throughout the process, we've sought to minimize the number of significant changes that dep introduces to the ecosystem. The committee came to the conclusion that the knock-on effects of not adopting a project-orientated view were much worse than doing so. Possible changes in symlink behavior are, unfortunately, one of the results.

ofpiyush · 2017-06-12T18:59:06Z

Yes, and it's something that either this PR, or a follow-up, should fix.

That's how I reached pkgtree. This line fails because it's expecting a directory and the project root is a symlink.

Would a special case fix for project roots work?

…oots-changes Signed-off-by: Ibrahim AshShohail <[email protected]>

Signed-off-by: Ibrahim AshShohail <[email protected]>

Project.ResolvedAbsRoot() returns the resolved project.AbsRoot if it's a symlink. Signed-off-by: Ibrahim AshShohail <[email protected]>

Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho · 2017-06-15T03:19:59Z

Tests should be passing now.

sdboyer

very close now

sdboyer · 2017-06-16T16:46:28Z

context.go

-	}
-
-*/
+// Ctx defines the supporting context of dep.


I'd like to see equivalent docs to what was previously there - examples, and such.

sdboyer · 2017-06-16T16:52:04Z

context.go

-	if err != nil {
-		return "", errors.Wrap(err, "resolveProjectRoot")
+// DetectProjectGOPATH will return an error in the following cases:
+// If p.AbsRoot is not a symlink and is not within any known GOPATH.


Put in a newline and further indent each of these by one space. That'll make godoc format them as a fixed block. As-written, this will all get collapsed without any line breaks.

sdboyer · 2017-06-16T17:33:00Z

project.go

+	p.ResolvedAbsRoot = root
+
+	// if p.AbsRoot is a symlink, set p.ResolvedAbsRoot to the resolved path.
+	if sym, err := fs.IsSymlink(p.AbsRoot); err != nil {


Probably don't need to check this; filepath.EvalSymlinks() will just return the input if it's not a symlink, which is the desired behavior anyway.

filepath.EvalSymlinks() will recursively walk over the symlinks and errors might happen along the way (lstat or readlink could fail). 😞

Anyhow, I changed the flow a bit.

sdboyer · 2017-06-16T17:35:00Z

project.go

+// SetRoot set the project AbsRoot. If the root is a symlink, it attempts to resolve it.
+func (p *Project) SetRoot(root string) error {
+	p.AbsRoot = root
+	p.ResolvedAbsRoot = root


It's probably preferable that we only change the *Project's internal state if filepath.EvalSymlinks() exits without an error. This makes it generally consistent with the practice that Go functions should not return values from funcs when an error is also returned - just, in this case, those values are internal properties, rather than being return values.

Does that make sense?

Signed-off-by: Ibrahim AshShohail <[email protected]>

sdboyer · 2017-06-16T23:11:40Z

context.go

-	l, err := os.Lstat(path)
-	if err != nil {
-		return "", errors.Wrap(err, "resolveProjectRoot")
+/*


nit: i know this works, but the standard is really just to use the // leader for godoc comments, even when they become quite large.

i'm just now realizing that some of the existing comments did this. that's my bad - i shouldn't have merged those in the first place. still, let's not repeat the pattern.

Done. 👍 I've fixed the existing occurrences.

sdboyer · 2017-06-17T00:40:16Z

project.go

 	// ImportRoot is the import path of the project's root directory.
 	ImportRoot gps.ProjectRoot
 	Manifest   *Manifest
 	Lock       *Lock // Optional
 }

+// SetRoot set the project AbsRoot. If the root is a symlink, it attempts to resolve it.
+func (p *Project) SetRoot(root string) error {
+	sym, err := fs.IsSymlink(root)


What I was suggesting before would be this:

func (p *Project) SetRoot(root string) error { rroot, err := filepath.EvalSymlinks(root) if err != nil { return err } p.ResolvedAbsRoot, p.AbsRoot = rroot, root return nil }

I think this would be equivalent. If root is not a symlink, then it's basically a no-op. If root is a symlink, then any error that would occur from fs.IsSymlink() are a subset of the errors from filepath.EvalSymlinks().

Apparently, I misunderstood the original comment. Updated now. 😁

Signed-off-by: Ibrahim AshShohail <[email protected]>

sdboyer · 2017-06-17T01:27:19Z

ok, i think we're looking good - waiting for tests now :D

Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho · 2017-06-17T01:46:01Z

Tests are passing now. 🎉
CodeClimate has actually finished running, but Github is still waiting for results for some reason. 😕

sdboyer · 2017-06-17T02:58:23Z

and in we finally go 😄

googlebot added the cla: yes label May 24, 2017

ibrasho mentioned this pull request May 26, 2017

internal/fs: handle symlinks in copyFile() #657

Merged

sdboyer mentioned this pull request May 31, 2017

Do not error out when the current directory is a symlink #684

Closed

ibrasho added 3 commits June 3, 2017 22:12

update FAQ about symlinks

3b21f6e

Signed-off-by: Ibrahim AshShohail <[email protected]>

internal/fs: add ResolvePath function

9730410

Signed-off-by: Ibrahim AshShohail <[email protected]>

dep: update NewContext(), rename ResolveProjectRoot() to ResolveProje…

91ac4d2

…ctRootAndGoPath() and add detectGoPath() Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho force-pushed the symlink-project-roots-changes branch from f3df625 to 91ac4d2 Compare June 3, 2017 19:53

dep: minor fixes & tweaks in context.go

3be59e3

Signed-off-by: Ibrahim AshShohail <[email protected]>

sdboyer suggested changes Jun 6, 2017

View reviewed changes

ibrasho added 2 commits June 6, 2017 06:25

dep: update letter casing to detectGOPATH() and ResolveProjectRootAnd…

6a464ac

…GOPATH() Edit letter case of: * detectGoPath() to detectGOPATH() * ResolveProjectRootAndGoPath() to ResolveProjectRootAndGOPATH() Signed-off-by: Ibrahim AshShohail <[email protected]>

Minor text edits

9675932

Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho self-assigned this Jun 10, 2017

This was referenced Jun 11, 2017

resolve project root is linked to another path within a GOPATH #743

Closed

Allow to run dep ensure in working directories which are symlinks #741

Closed

sdboyer suggested changes Jun 12, 2017

View reviewed changes

ibrasho added 4 commits June 12, 2017 22:21

Merge remote-tracking branch 'upstream/master' into symlink-project-r…

b7048cc

…oots-changes Signed-off-by: Ibrahim AshShohail <[email protected]>

internal/fs: Remove fs.ResolvePath()

28b6f86

Signed-off-by: Ibrahim AshShohail <[email protected]>

fix tests

22cd768

Signed-off-by: Ibrahim AshShohail <[email protected]>

dep: Add project.ResolvedAbsRoot()

5bf9205

Project.ResolvedAbsRoot() returns the resolved project.AbsRoot if it's a symlink. Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho force-pushed the symlink-project-roots-changes branch from e0f2a5f to 5bf9205 Compare June 13, 2017 16:55

ibrasho force-pushed the symlink-project-roots-changes branch 2 times, most recently from 96ede09 to aa6fd8e Compare June 15, 2017 02:23

Ensure dep reads GOPATH from Config.Env

0b8edb1

Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho force-pushed the symlink-project-roots-changes branch from aa6fd8e to 0b8edb1 Compare June 15, 2017 03:13

sdboyer suggested changes Jun 16, 2017

View reviewed changes

Update docs for dep.Ctx. Ctx.SetPaths and Ctx.DetectProjectGOPATH

026038a

Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho force-pushed the symlink-project-roots-changes branch 3 times, most recently from 0306540 to f40426e Compare June 16, 2017 19:08

Tweak dep.Project.SetRoot to ensure consistent internal state

b98d713

Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho force-pushed the symlink-project-roots-changes branch 3 times, most recently from 53e69de to b98d713 Compare June 16, 2017 20:45

sdboyer suggested changes Jun 17, 2017

View reviewed changes

ibrasho added 4 commits June 17, 2017 03:47

dep: Update godoc to use // instead of /* ... */

138cebb

Signed-off-by: Ibrahim AshShohail <[email protected]>

dep: Simplify dep.*Project.SetRoot

3826bb0

Signed-off-by: Ibrahim AshShohail <[email protected]>

dep: Fix godoc for dep.*Ctx.DetectProjectGOPATH

f4b9f49

Signed-off-by: Ibrahim AshShohail <[email protected]>

dep: update tests for dep.*Ctx.DetectProjectGOPATH

a69d6ae

Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho force-pushed the symlink-project-roots-changes branch from 0d05390 to 4945522 Compare June 17, 2017 01:24

dep: cleanup TestLoadProject and TestDetectProjectGOPATH

13f512f

Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho force-pushed the symlink-project-roots-changes branch from 4945522 to 13f512f Compare June 17, 2017 01:32

sdboyer approved these changes Jun 17, 2017

View reviewed changes

sdboyer merged commit a42708e into golang:master Jun 17, 2017

ibrasho deleted the symlink-project-roots-changes branch June 17, 2017 03:04

ibrasho mentioned this pull request Jun 17, 2017

dep ensure failed to resolve project root #372

Closed

tkrajina mentioned this pull request Jul 29, 2017

Use evaluated project root path #812

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handling symlinks as project root #641

Handling symlinks as project root #641

ibrasho commented May 24, 2017 •

edited by sdboyer

Loading

sdboyer commented May 25, 2017

ibrasho commented May 25, 2017 •

edited

Loading

sdboyer left a comment

sdboyer Jun 6, 2017

ibrasho commented Jun 10, 2017

ofpiyush commented Jun 11, 2017

sdboyer commented Jun 12, 2017

sdboyer left a comment

sdboyer Jun 12, 2017

sdboyer Jun 12, 2017

ibrasho Jun 12, 2017

ofpiyush commented Jun 12, 2017

sdboyer commented Jun 12, 2017

ofpiyush commented Jun 12, 2017 •

edited

Loading

sdboyer commented Jun 12, 2017

ofpiyush commented Jun 12, 2017 •

edited

Loading

ibrasho commented Jun 15, 2017

sdboyer left a comment

sdboyer Jun 16, 2017

sdboyer Jun 16, 2017

ibrasho Jun 16, 2017

sdboyer Jun 16, 2017

ibrasho Jun 16, 2017

sdboyer Jun 16, 2017

ibrasho Jun 16, 2017

sdboyer Jun 16, 2017

sdboyer Jun 17, 2017

ibrasho Jun 17, 2017 •

edited

Loading

sdboyer Jun 17, 2017

ibrasho Jun 17, 2017

sdboyer commented Jun 17, 2017

ibrasho commented Jun 17, 2017

sdboyer commented Jun 17, 2017

Handling symlinks as project root #641

Handling symlinks as project root #641

Conversation

ibrasho commented May 24, 2017 • edited by sdboyer Loading

How does dep behave if the project root is a symbolic link?

sdboyer commented May 25, 2017

ibrasho commented May 25, 2017 • edited Loading

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibrasho commented Jun 10, 2017

ofpiyush commented Jun 11, 2017

sdboyer commented Jun 12, 2017

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofpiyush commented Jun 12, 2017

sdboyer commented Jun 12, 2017

ofpiyush commented Jun 12, 2017 • edited Loading

sdboyer commented Jun 12, 2017

ofpiyush commented Jun 12, 2017 • edited Loading

ibrasho commented Jun 15, 2017

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibrasho Jun 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer commented Jun 17, 2017

ibrasho commented Jun 17, 2017

sdboyer commented Jun 17, 2017

ibrasho commented May 24, 2017 •

edited by sdboyer

Loading

How does `dep` behave if the project root is a symbolic link?

ibrasho commented May 25, 2017 •

edited

Loading

ofpiyush commented Jun 12, 2017 •

edited

Loading

ofpiyush commented Jun 12, 2017 •

edited

Loading

ibrasho Jun 17, 2017 •

edited

Loading