Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.
20 changes: 16 additions & 4 deletions context.go
Original file line number Diff line number Diff line change
Expand Up @@ -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).

"log"
"os"
"path/filepath"
Expand Down Expand Up @@ -222,15 +223,26 @@ func (c *Ctx) detectGOPATH(path string) (string, error) {
// 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) {
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.

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 👍

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)

}

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.

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

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

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

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


srcprefix := filepath.Join(gopathEvaluated, "src") + string(filepath.Separator)
if fs.HasFilepathPrefix(pathEvaluated, srcprefix) {
if len(pathEvaluated) <= len(srcprefix) {
return "", errors.New("dep does not currently support using GOPATH/src as the project root")
}

// filepath.ToSlash because we're dealing with an import path now,
// not an fs path
return filepath.ToSlash(path[len(srcprefix):]), nil
return filepath.ToSlash(pathEvaluated[len(srcprefix):]), nil
}

return "", errors.Errorf("%s not in GOPATH", path)
Expand Down
29 changes: 29 additions & 0 deletions context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -491,3 +491,32 @@ func TestDetectGOPATH(t *testing.T) {
}
}
}

func TestDetectProjectSymlinkedOutsideGOPATH(t *testing.T) {
h := test.NewHelper(t)
defer h.Cleanup()

h.TempDir("src")

h.Setenv("GOPATH", h.Path("."))
depCtx := &Ctx{GOPATH: h.Path(".")}

importPath := "ti/ga/fato/la/večera"
h.TempDir(filepath.Join("src", importPath))
h.TempDir("symlink/to/project")

symlinkSrc := h.OriginalPath(filepath.Join("src", importPath))
symlinkTarget := h.OriginalPath("symlink/to/project/sym")
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())
}


got, err := depCtx.ImportForAbs(symlinkTarget)
if err != nil {
t.Fatal(err)
}
if got != importPath {
t.Fatalf("expected %s, got %s", importPath, got)
}
}
22 changes: 15 additions & 7 deletions internal/test/test.go
Original file line number Diff line number Diff line change
Expand Up @@ -498,8 +498,21 @@ func (h *Helper) TempDir(path string) {
}

// Path returns the absolute pathname to file with the temporary
// directory.
// directory. Symlinks are evaluated.
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?


// Ensure it's the absolute, symlink-less path we're returning
abs, err := filepath.EvalSymlinks(path)
if err != nil {
h.t.Fatalf("%+v", errors.Wrapf(err, "internal testsuite error: could not get absolute path for dir(%q)", path))
}
return abs
}

// OriginalPath returns the absolute pathname to file with the temporary
// directory. Symlinks are NOT evaluated.
func (h *Helper) OriginalPath(name string) string {
if h.tempdir == "" {
h.t.Fatalf("%+v", errors.Errorf("internal testsuite error: path(%q) with no tempdir", name))
}
Expand All @@ -511,12 +524,7 @@ func (h *Helper) Path(name string) string {
joined = filepath.Join(h.tempdir, name)
}

// Ensure it's the absolute, symlink-less path we're returning
abs, err := filepath.EvalSymlinks(joined)
if err != nil {
h.t.Fatalf("%+v", errors.Wrapf(err, "internal testsuite error: could not get absolute path for dir(%q)", joined))
}
return abs
return joined
}

// MustExist fails if path does not exist.
Expand Down