-
Notifications
You must be signed in to change notification settings - Fork 1k
GOPATH detection now works when GOPATH is a symlink #834
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving just a light review. Have to go through it again. 😊
joined = filepath.Join(h.tempdir, name) | ||
evaled, err := filepath.EvalSymlinks(h.tempdir) | ||
if err != nil { | ||
h.t.Fatalf("%+v", errors.Wrapf(err, "internal testsuite error: could not evaluate symlinks for dir(%q)", h.tempdir)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. I can wait and fix it after #812 is completed.
if err := os.Symlink(fullOldPath, fullNewPath); err != nil { | ||
h.t.Fatalf("%+v", errors.Errorf("Unable to create symlink from %s -> %s", fullOldPath, fullNewPath)) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Can you add a comment for this method? As it's an exported method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this.
@nkatsaros Is there a specific reason for using a symlink as @sdboyer Do we plan to support this case? I'm drowning in symlinks nightmares thinking about this. 😫 |
39cbdd4
to
d688a81
Compare
@ibrasho When I setup Go on my laptop a long time ago I set my GOPATH to a symlink for some reason. I don't set it up like that on new machines anymore nor have a special use case that requires it. Like I mentioned at the top, however, I've never had problems with this setup and the |
just dropping an update to say that #812 is likely to be blocked until at least next week, when i'll have time and energy to steel my nerves for another deep dive into symlinks 😱 |
Ok, we had a few cases in the office where people used symlinks so I understand the use-case better now. A common case was |
Just encountered this. On FreeBSD, |
Hi, is there any chance to get this merged? There was an article on gopheracademy on dep, but it doesn't work with default FreeBSD installations, where /home is a symlink to /usr/home. |
Dep was officially deprecated earlier this year, and the proposal to archive this repository was accepted. As such, I'm closing outstanding issues before archiving the repository. For any further comments, please use the proposal thread on the Go issue tracker. Thanks! |
What does this do / why do we need it?
Currently,
dep init
fails withctx.DetectProjectGOPATH: /Users/nkatsaros/some/symlinked/src/project is not within a known GOPATH
if your GOPATH is a symlink. Note that the GOPATH is the symlink, not the project path.Given that the standard Go toolchain works with this situation, dep should likely work too.
What should your reviewer look out for in this PR?
Nope.
Do you need help or clarification on anything?
The tests in
TestDetectProjectGOPATH
are a little strange, the function likely needs some clean up. They work, but I'm not 100% sure they are correct.Which issue(s) does this PR fix?
None.