Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Use evaluated project root path #812

Closed
wants to merge 13 commits into from
Closed

Conversation

tkrajina
Copy link
Contributor

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:

cd ~/project
ln -s $GOPATH/src/github.com/project/aaa/project .
cd project

...and in this symlinked directory all go tools (go, godep, gofmt, goimports, ...) work without problems.

But, dep fails with:

root project import: /.../projects/project not in GOPATH

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.

Allows dep to be executed in a directory symlinked  to a
directory in $GOPATH.
@googlebot
Copy link
Collaborator

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!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@tkrajina
Copy link
Contributor Author

I signed it!

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@tkrajina
Copy link
Contributor Author

The tests are fixed now. It looks like on OSX even the temporary test GOPATH was a symlink. That's why in ImportForAbs() both the import path and GOPATH need to be Eval'ed.

context.go Outdated
@@ -11,6 +11,7 @@ import (
"runtime"
"strings"

"fmt"
Copy link
Collaborator

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())
Copy link
Collaborator

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.

Copy link
Contributor Author

@tkrajina tkrajina Jul 15, 2017

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)
Copy link
Collaborator

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?

@darkowlzz
Copy link
Collaborator

Looks good. 👍
Can you also add a test for the same?
Refer: https://github.com/golang/dep/blob/master/context_test.go#L25
Let us know if you need any help with that 😊

@tkrajina
Copy link
Contributor Author

tkrajina commented Jul 16, 2017

@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) {

Copy link
Collaborator

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"
Copy link
Collaborator

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.

Copy link
Contributor Author

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())
Copy link
Collaborator

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())
Copy link
Collaborator

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.

Copy link
Collaborator

@darkowlzz darkowlzz left a 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())
Copy link
Collaborator

@darkowlzz darkowlzz Jul 16, 2017

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.

Copy link
Collaborator

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())
Copy link
Collaborator

@darkowlzz darkowlzz Jul 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fmt.Errorf -> errors.Errorf

@darkowlzz
Copy link
Collaborator

One thing I didn't test is the situation when even the GOPATH is a symlink

@tkrajina there seems to be work going on for that already #834

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())
}
Copy link
Collaborator

@darkowlzz darkowlzz Jul 16, 2017

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())
}

@ibrasho
Copy link
Collaborator

ibrasho commented Jul 16, 2017

TBH, I don't feel comfortable with this. 😩

I liked the first attempt where you called ctx.ImportForAbs with p.ResolvedAbsRoot. If we can make the tests work with that, it'll be a better solution IMO.

@tkrajina
Copy link
Contributor Author

tkrajina commented Jul 16, 2017

@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:

  • other developers will need to make sure to always call ctx.ImportForAbs(p.ResolvedAbsRoot) instead of ctx.ImportForAbs(p.AbsRoot). It's easy to forget it.
  • it's simpler to unit test only ctx (like I did in my test), than test it in all commands calling ctx.ImportForAbs()

Anyway, If you guys agree that symlink evaluation needs to happen outside of ctx.ImportForAbs() I'll change it, and rewrite the test. Just let me know.

@ibrasho
Copy link
Collaborator

ibrasho commented Jul 16, 2017

I was actually thinking of renaming the method to ImportPathForProject and passing it a Project instead of the path. 🤔

@ibrasho
Copy link
Collaborator

ibrasho commented Jul 16, 2017

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)

@tkrajina
Copy link
Contributor Author

tkrajina commented Jul 17, 2017

@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):

/var/folders/np/f6xzpfd94217nvgdpmwt_qbr0000gn/T/gotest032483835
/private/var/folders/np/f6xzpfd94217nvgdpmwt_qbr0000gn/T/gotest032483835

That's why I added filepath.EvalSymlinks(c.GOPATH) in ImportForAbs().

@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 ctx.ResolvedGOPAH along with the existing ctx.GOPATH. Similarly to the existing p.ResolvedAbsRoot and p.AbsRoot. And then use the resolved GOPATH in ImportPathForProject().

PS. I can totally understand your feeling when you sad "I'm drowning in symlinks nightmares" :)

@sdboyer
Copy link
Member

sdboyer commented Jul 18, 2017

no worries, it's no rush :)

but from what I can see, no

err, i didn't ask a yes/no question - rather, it was "source" or "target." could you rephrase?

@ibrasho
Copy link
Collaborator

ibrasho commented Jul 20, 2017

My big question to start would be - is the source or the target of the symlink considered when deciding what the root import path of the project is?

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. 😩

@tkrajina
Copy link
Contributor Author

I think that the code in  integration_testproj.go can be simplified by completely removing the runtime.GOOS == "darwin" check. Since it's not directly related to this issue, I created a new pull request #882 .

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 TestDetectProjectSymlinkedOutsideGOPATH() test to work with ctx.ImportPathForProject() instead of ctx.ImportForAbs().

What do you think?

@tkrajina
Copy link
Contributor Author

tkrajina commented Jul 23, 2017

The last commit pushed includes the changes proposed by @ibrasho and the project-symlink test.

The build will fail. because of #882 .

@tkrajina
Copy link
Contributor Author

Build is now OK (after #882 merged).

Copy link
Member

@sdboyer sdboyer left a 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 😞

@tkrajina
Copy link
Contributor Author

tkrajina commented Jul 26, 2017

@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).

@sdboyer
Copy link
Member

sdboyer commented Jul 26, 2017

I can bisect to see when exactly was the bug reintroduced

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.

@tkrajina
Copy link
Contributor Author

@sdboyer I tried to bisect, but no luck because I couldn't find the "good" point in the #247 merge. That PR introduced the ctx.resolveProjectRoot() which should fix the problem. And the unit test tested if the context works correctly using that method. But the problem was that ctx.resolveProjectRoot() was used too late. The context was initialized much earlier in main.go:

ctx, err := dep.NewContext()

...and this method had no symlink resolution, and that's why any dep command in a symlinked project failed.

The second PR I could find was #641 (a42708e)

This PR introduced project.ResolvedAbsRoot (along with the existing project.AbsRoot), but it still called:

ip, err := c.SplitAbsoluteProjectRoot(p.AbsRoot)

Instead of using p.ResolvedAbsRoot and that's why dep init still fails with determineProjectRoot: /path/to/symlinked/project not in any GOPATH.

Note that the SplitAbsoluteProjectRoot was later renamed and in the current master it's:

ip, err := c.ImportForAbs(p.AbsRoot)

The current PR is basically just a small improvement over #641. It contain's @ibrasho's suggestion to change ImportForAbs() so that the function argument becomes *Project. And it internally uses project.ResolvedAbsRoot (which is the direct path, not the syminked one).

One of the earlier commits in this PR had a similar solution, but just evaluated every path (for projects and GOPATH) inside ImportForAbs().

Copy link
Collaborator

@ibrasho ibrasho left a 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)
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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?

@carolynvs carolynvs removed their request for review September 18, 2017 18:49
Copy link
Collaborator

@ibrasho ibrasho left a 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
}
Copy link
Collaborator

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.

Copy link
Collaborator

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...

@ddspog
Copy link

ddspog commented Nov 27, 2017

Hey, are you considering the cases when $GOPATH/src is a symlink to a different folder?

I'm troubled with the long path required by Golang setup, and symlinked $GOPATH/src to G:/ so all my github projects are there, and for example, I would like to run:

cd G:\github.com\ddspog\random-repo
dep init

All go tools worked, only dep don't.

I don't know if considering the case when $GOPATH is a symlink also fix my problem. Maybe don't.

@tkrajina
Copy link
Contributor Author

tkrajina commented Nov 28, 2017

@ddspog Nope, this PR is mostly about the situation when you have

ln -s $GOPATH/src/path/to/your/project somewhere/outsite/gopath

...and then you you use all the go tools inside somewhere/outsite/gopath. I suppose your problem is solvable in a similar way. But, I'm still waiting to see what @sdboyer thinks about this symlinks conundrum before proceeding with this :)

@Globegitter
Copy link

@tkrajina Any update on this PR? In the need of a solution for this.

@tkrajina
Copy link
Contributor Author

@Globegitter I don't know. For me, I just use my own fork of dep and it works.

I see that master changed so that the PR isn't "mergeable" at the moment. I'll try to find half an hour in the next few days to at least make it "mergeable" again. I'm not sure if it will affect the long-term status of this PR, but at least you'll have an up-to-date fork which solves a our shared problem :)

@tkrajina
Copy link
Contributor Author

@Globegitter Here you have, the lattest commit makes this PR mergeable again.

For some strange reason the HOMEBREW build fails with:

--- FAIL: TestSlowVcs (0.00s)
	--- FAIL: TestSlowVcs/svn-repo (11.51s)
		vcs_repo_test.go:79: Unable to checkout SVN repo. Err was unable to get repository

But running test locally works on my OSX. I don't have the time to look into this at the moment. Hopefully next weekend.

@malexdev
Copy link

+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.

@mvdan
Copy link
Member

mvdan commented Sep 4, 2020

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!

@mvdan mvdan closed this Sep 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants