-
Notifications
You must be signed in to change notification settings - Fork 1k
Conversation
Allows dep to be executed in a directory symlinked to a directory in $GOPATH.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
CLAs look good, thanks! |
The tests are fixed now. It looks like on OSX even the temporary test GOPATH was a symlink. That's why in |
context.go
Outdated
@@ -11,6 +11,7 @@ import ( | |||
"runtime" | |||
"strings" | |||
|
|||
"fmt" |
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.
nit: could move this import up with the other std lib imports
LGTM besides, unless there should be some new test cases as well.
context.go
Outdated
|
||
path, err = filepath.EvalSymlinks(path) | ||
if err != nil { | ||
return "", fmt.Errorf("Error evaluating symlinks in %s: %s", path, err.Error()) |
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.
Is the intention to show the original path in the error? This looks like path
would be set to the result of EvalSymlinks
.
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.
Yes, you're right. Pushed now a fix (includes moving "fmt"
import up).
context.go
Outdated
} | ||
|
||
return "", errors.Errorf("%s not in GOPATH", path) | ||
return "", errors.Errorf("%s not in GOPATH", pathEvaluated) |
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.
Should this one stay path
?
Looks good. 👍 |
@darkowlzz pushed the test now. One thing I didn't test is the situation when even the GOPATH is a symlink, but that situation is implicitly tested in the OSX build on travis. And I don't think that's really a common situation. But if you think it's needed, I can add that one, too. |
context.go
Outdated
srcprefix := filepath.Join(c.GOPATH, "src") + string(filepath.Separator) | ||
if fs.HasFilepathPrefix(path, srcprefix) { | ||
if len(path) <= len(srcprefix) { | ||
|
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.
Remove the extra new line.
context.go
Outdated
@@ -5,6 +5,7 @@ | |||
package dep | |||
|
|||
import ( | |||
"fmt" |
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.
Make sure you run gofmt
.
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.
Hm, not really sure what's the problem here. I did run gofmt (in fact one of the pust-push checks fails when the code is not gofmt'ed).
context.go
Outdated
|
||
gopathEvaluated, err := filepath.EvalSymlinks(c.GOPATH) | ||
if err != nil { | ||
return "", fmt.Errorf("Error evaluating symlinks in GOPATH %s: %s", c.GOPATH, err.Error()) |
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.
Errors should start with a lowercase letter.
Also, use errors.Wrap
to achieve better results:
errors.Wrapf(err, "failed to evaluate symlink %s", c.GOPATH)
context.go
Outdated
|
||
pathEvaluated, err := filepath.EvalSymlinks(path) | ||
if err != nil { | ||
return "", fmt.Errorf("Error evaluating symlinks in %s: %s", path, err.Error()) |
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.
Same nit as the error above.
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.
Great to see the test! 😃
Just a few more small changes.
context.go
Outdated
|
||
gopathEvaluated, err := filepath.EvalSymlinks(c.GOPATH) | ||
if err != nil { | ||
return "", fmt.Errorf("Error evaluating symlinks in GOPATH %s: %s", c.GOPATH, err.Error()) |
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.
Let's use errors.Errorf
instead of fmt.Errorf
to be consistent with the rest of the file.
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.
oops! there's error value. Follow @ibrasho 's comment 👍
context.go
Outdated
|
||
pathEvaluated, err := filepath.EvalSymlinks(path) | ||
if err != nil { | ||
return "", fmt.Errorf("Error evaluating symlinks in %s: %s", path, err.Error()) |
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.
fmt.Errorf
-> errors.Errorf
context_test.go
Outdated
err := os.Symlink(symlinkSrc, symlinkTarget) | ||
if err != nil { | ||
t.Errorf("Error creating symlink from %s to %s: %s", symlinkSrc, symlinkTarget, err.Error()) | ||
} |
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.
Since os.Symlink()
just returns a single value, let's make it short:
if err := os.Symlink(symlinkSrc, symlinkTarget); err != nil {
t.Errorf("Error creating symlink from %s to %s: %s", symlinkSrc, symlinkTarget, err.Error())
}
TBH, I don't feel comfortable with this. 😩 I liked the first attempt where you called |
@ibrasho My first idea was the first commit, but that method is called in two places. One is the one from my change, the other one is https://github.com/golang/dep/blob/master/cmd/dep/init.go#L123 . It's simple to use the resolved path in both cases, but I was thinking that:
Anyway, If you guys agree that symlink evaluation needs to happen outside of |
I was actually thinking of renaming the method to |
Something along these lines... 🤔 (click to open)diff --git a/cmd/dep/init.go b/cmd/dep/init.go
index d45f7e40e52e..931062ea9bd2 100644
--- a/cmd/dep/init.go
+++ b/cmd/dep/init.go
@@ -116,9 +116,9 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error {
return errors.Errorf("invalid state: manifest %q does not exist, but lock %q does", mf, lf)
}
- ip, err := ctx.ImportForAbs(root)
+ ip, err := ctx.ImportPathForProject(p)
if err != nil {
- return errors.Wrap(err, "root project import")
+ return errors.Wrap(err, "failed to determine import path for project")
}
p.ImportRoot = gps.ProjectRoot(ip)
diff --git a/context.go b/context.go
index cec883133bf9..3f917972bdba 100644
--- a/context.go
+++ b/context.go
@@ -113,9 +113,9 @@ func (c *Ctx) LoadProject() (*Project, error) {
return nil, err
}
- ip, err := c.ImportForAbs(p.AbsRoot)
+ ip, err := c.ImportPathForProject(p)
if err != nil {
- return nil, errors.Wrap(err, "root project import")
+ return nil, errors.Wrap(err, "failed to determine import path for project")
}
p.ImportRoot = gps.ProjectRoot(ip)
@@ -219,9 +220,14 @@ func (c *Ctx) detectGOPATH(path string) (string, error) {
return "", errors.Errorf("%s is not within a known GOPATH", path)
}
-// ImportForAbs returns the import path for an absolute project path by trimming the
-// `$GOPATH/src/` prefix. Returns an error for paths equal to, or without this prefix.
-func (c *Ctx) ImportForAbs(path string) (string, error) {
+// ImportPathForProject returns the import path for an absolute project path by trimming the
+// `GOPATH/src/` prefix. Returns an error for paths equal to, or without this prefix.
+func (c *Ctx) ImportPathForProject(p *Project) (string, error) {
+ path := p.AbsRoot
+ if p.AbsRoot != p.ResolvedAbsRoot {
+ path = p.ResolvedAbsRoot
+ }
+
srcprefix := filepath.Join(c.GOPATH, "src") + string(filepath.Separator)
if fs.HasFilepathPrefix(path, srcprefix) {
if len(path) <= len(srcprefix) {
diff --git a/context_test.go b/context_test.go
index 4ce9c0521d81..3f07e30608ec 100644
--- a/context_test.go
+++ b/context_test.go
@@ -39,7 +39,12 @@ func TestCtx_ProjectImport(t *testing.T) {
for _, want := range importPaths {
fullpath := filepath.Join(depCtx.GOPATH, "src", want)
h.TempDir(filepath.Join("src", want))
- got, err := depCtx.ImportForAbs(fullpath)
+
+ p := &Project{}
+ if err := p.SetRoot(fullpath); err != nil {
+ t.Fatal(err)
+ }
+ got, err := depCtx.ImportPathForProject(p)
if err != nil {
t.Fatal(err)
}
@@ -49,16 +54,14 @@ func TestCtx_ProjectImport(t *testing.T) {
}
// test where it should return an error when directly within $GOPATH/src
- got, err := depCtx.ImportForAbs(filepath.Join(depCtx.GOPATH, "src"))
+ p := &Project{}
+ if err := p.SetRoot(filepath.Join(depCtx.GOPATH, "src")); err != nil {
+ t.Fatal(err)
+ }
+ got, err := depCtx.ImportPathForProject(p)
if err == nil || !strings.Contains(err.Error(), "GOPATH/src") {
t.Fatalf("should have gotten an error for use directly in GOPATH/src, but got %s", got)
}
-
- // test where it should return an error
- got, err = depCtx.ImportForAbs("tra/la/la/la")
- if err == nil {
- t.Fatalf("should have gotten an error but did not for tra/la/la/la: %s", got)
- }
}
func TestAbsoluteProjectRoot(t *testing.T) {
@@ -347,7 +350,11 @@ func TestCaseInsentitiveGOPATH(t *testing.T) {
ip := "github.com/pkg/errors"
fullpath := filepath.Join(depCtx.GOPATH, "src", ip)
h.TempDir(filepath.Join("src", ip))
- pr, err := depCtx.ImportForAbs(fullpath)
+ p := &Project{}
+ if err := p.SetRoot(fullpath); err != nil {
+ t.Fatal(err)
+ }
+ pr, err := depCtx.ImportPathForProject(p)
if err != nil {
t.Fatal(err)
}
diff --git a/internal/test/integration_testproj.go b/internal/test/integration_testproj.go
index c6c68db7a4de..b0ed7caff1a7 100644
--- a/internal/test/integration_testproj.go
+++ b/internal/test/integration_testproj.go
@@ -55,7 +55,7 @@ func NewTestProject(t *testing.T, initPath, wd string, externalProc bool, run Ru
// below the wd, and therefore the GOPATH, is recorded as "/var/..." but the
// actual process runs in "/private/var/..." and dies due to not being in the
// GOPATH because the roots don't line up.
- if externalProc && runtime.GOOS == "darwin" && needsPrivateLeader(new.tempdir) {
+ if runtime.GOOS == "darwin" && needsPrivateLeader(new.tempdir) {
new.Setenv("GOPATH", filepath.Join("/private", new.tempdir))
} else {
new.Setenv("GOPATH", new.tempdir) |
@ibrasho That will work. But the build will fail because the situation where GOPATH itself is a symlink isn't handled. I know this is not a common situation, but it happens in the fourth build on travis (see https://travis-ci.org/golang/dep/builds/254211644?utm_source=github_status&utm_medium=notification). These are the $GOPATH directories created on my laptop (the first one is the $GOPATH, the second one is what it evaluates to):
That's why I added @darkowlzz mentioned that that this is the problem in #834 My current solution solves both the problems. I like your solution, but if we want to handle the symlinked GOPATH problem, and want to be consistent -- we should probably introduce PS. I can totally understand your feeling when you sad "I'm drowning in symlinks nightmares" :) |
no worries, it's no rush :)
err, i didn't ask a yes/no question - rather, it was "source" or "target." could you rephrase? |
I'm not sure which symlink you are referring to. At the moment, we don't evaluate GOPATH even if it's a symlink. We only do that for the project path. And I'm would prefer it stays that way. 😁 Playing with 2 symlinks at the same time is not going to be pretty. 😩 |
I think that the code in If that change is acceptable for you guys, then @ibrasho's proposed change will work without problems. In that case the only thing missing (for me) is to refactor my What do you think? |
Build is now OK (after #882 merged). |
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.
ok so, i need to take some time to digest this. symlinks are my ongoing waking nightmare, and i'd swear we just had a PR in recently that was supposed to fix this exact problem. i need to review this in detail, compare it with both our open and closed PRs in order to be really sure of the outcomes here, and that it is only bringing us closer to the intended model, rather than creating a some-steps-forward some-steps-back situation.
and that's probably not going to happen until next week, just because i won't have time 😞
@sdboyer FYI pull request which originally fixed this problem was #247 (#330) . But the bug was later reintroduced and for some reason the test didn't fail when it did. If that can help, I can bisect to see when exactly was the bug reintroduced (but, I'm in a similar situation, I can spend some time on this only on weekends). |
if you can find time to do this, that actually would be helpful. i really, really want to never have these problems again, and anything that helps me build the big picture of the chronology in my mind is a useful step towards that. |
@sdboyer I tried to bisect, but no luck because I couldn't find the "good" point in the #247 merge. That PR introduced the
...and this method had no symlink resolution, and that's why any The second PR I could find was #641 (a42708e) This PR introduced
Instead of using Note that the
The current PR is basically just a small improvement over #641. It contain's @ibrasho's suggestion to change One of the earlier commits in this PR had a similar solution, but just evaluated every path (for projects and GOPATH) inside |
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.
Minor nits.
func (h *Helper) Path(name string) string { | ||
path := h.OriginalPath(name) |
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 we instead keep Path
as-is, and make the new function EvalPath
or ResolvedPath
the one that evaluates symlinks?
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.
@ibrasho yes, it makes sense. But I'd propose to delay that change after @sdboyer decides if this PR is mergeable or not. Because, this change will just increase the diff by a few hundred lines and increase the possibility of merge conflicts with the current origin/master
.
If everything is OK, I'll make sure that the PR with this additional change merges in origin/master
without conflicts.
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.
@sdboyer What's your opinion?
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.
Minor nits. Otherwise, LGTM.
path := p.AbsRoot | ||
if p.AbsRoot != p.ResolvedAbsRoot { | ||
path = p.ResolvedAbsRoot | ||
} |
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't we just do path = p.ResolvedAbsRoot
instead?
If they are the same, we did what line 223 is doing. If they are different, we did the if block logic.
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.
Or just use p.ResolvedAbsRoot
instead of path
...
Hey, are you considering the cases when I'm troubled with the long path required by Golang setup, and symlinked
All go tools worked, only dep don't. I don't know if considering the case when |
@ddspog Nope, this PR is mostly about the situation when you have
...and then you you use all the go tools inside |
@tkrajina Any update on this PR? In the need of a solution for this. |
@Globegitter I don't know. For me, I just use my own fork of I see that |
@Globegitter Here you have, the lattest commit makes this PR mergeable again. For some strange reason the HOMEBREW build fails with:
But running test locally works on my OSX. I don't have the time to look into this at the moment. Hopefully next weekend. |
+1 to fixing this, I am now in need of this functionality as well. @tkrajina, If I can do anything to help let me know. |
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! |
Allows dep to be executed in a directory symlinked to a
directory in $GOPATH.
What does this do / why do we need it?
All go command line tools work with directories which are symlinked from a directory in $GOPATH.
For example:
...and in this symlinked directory all go tools (go, godep, gofmt, goimports, ...) work without problems.
But, dep fails with:
What should your reviewer look out for in this PR?
This PR is rather simple. It just runs makes sure that the
c.ImportForAbs()
is called with the evaluated project path. Calling this method with a nonevalated path fails in case the project root is a symlink to a directory inside $GOPATH/src.Do you need help or clarification on anything?
No.
Which issue(s) does this PR fix?
This is a followup fix for #330 which was resolved/closed, but the bug was later reintroduced.