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

Resolve symlinks if project root has them. #247

Merged
merged 15 commits into from
Apr 11, 2017

Conversation

brianstarke
Copy link
Contributor

I use a symbolic link for my work directory (e.g go/src/github.com/my_org -> gocc) and I imagine others do as well. This patch is intended to solve for that by resolving the symlinks before executing the project root split and avoiding the below error.

~/gocc/common(master) » dep ensure -update                                                                                  
split absolute project root: /Users/brianstarke/gocc/common not in any $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 you signed the CLA as a corporation, please let us know the company's name.

@brianstarke
Copy link
Contributor Author

I signed it!

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

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.

Thanks for the contribution!

Symlinks can be a nasty thing, but this seems harmless enough. Just a couple small changes and this'll be good to merge.

context.go Outdated
path, err := filepath.EvalSymlinks(path)

if err != nil {
return "", fmt.Errorf("failed attempt to resolve symlinks: %s", err)
Copy link
Member

Choose a reason for hiding this comment

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

Our patterns around error handling aren't exactly the best right now, but let's be consistent - when wrapping an error returned from a called function, please use errors.Wrapf().

context_test.go Outdated

os.Symlink(
filepath.Join(depCtx.GOPATH, "src", want),
symlinkedPath)
Copy link
Member

Choose a reason for hiding this comment

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

nit: just one line is fine, please

context.go Outdated
@@ -124,6 +124,13 @@ func (c *Ctx) LoadProject(path string) (*Project, error) {
//
// The second returned string indicates which GOPATH value was used.
func (c *Ctx) SplitAbsoluteProjectRoot(path string) (string, error) {
// the path may lie within a symlinked directory, resolve those first
path, err := filepath.EvalSymlinks(path)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of calling this within SplitAbsoluteProjectRoot(), let's do the symlink evaluation up in LoadProject(). That keeps this a bit cleaner, by keeping this function solely dealing with symbolic strings, and LoadProject() doing the actual filesystem work.

@sdboyer
Copy link
Member

sdboyer commented Feb 21, 2017

Closing and reopening to see if failure goes away

@sdboyer sdboyer closed this Feb 21, 2017
@sdboyer sdboyer reopened this Feb 21, 2017
@sdboyer
Copy link
Member

sdboyer commented Feb 21, 2017

Looks like that's a real error - we need to consider Windows portability here.

@brianstarke
Copy link
Contributor Author

I was worried that the fail might be real. I don't have a Windows dev environment set up at this time, but I can throw something together and see what's up.

Thanks for the review! I'll make the changes later today.

@brianstarke
Copy link
Contributor Author

The Windows failure isn't happening anymore, hopefully this fixes the issue. I've not tried it on Windows yet.

@freeformz
Copy link

This get's complicated though if the location we are also working in is in $GOPATH, along with the target. What is our package name then? The original or the target?

@brianstarke
Copy link
Contributor Author

I might not understand your comment fully, but basically the package names shouldn't be effected by this change, it basically just uses the full path in place of the symbolic one.

If you mean the package path listed in the manifest... something like symlink/my-project would have been resolved to github.com/my-org/my-project which seems like it would be preferable.

(Also, this is a contrived example - using dep from within a symlinked directory doesn't actually work at all.)

Totally happy to provide more test cases to address your concerns, I just don't quite grok them yet.

@freeformz
Copy link

freeformz commented Feb 22, 2017

It may not matter atm, but consider this ...

$GOPATH=~/go
current directory: ~/go/src/github.com/foo/bar
~/go/src/github.com/foo/bar is a symlink to ~/go/src/github.com/baz/bibble.

At that point, which package/project is deps dealing with? github.com/foo/bar or github.com/baz/bibble?

@sdboyer
Copy link
Member

sdboyer commented Feb 23, 2017

I do agree that intra-GOPATH symlinking does make this seem confusing. To your example, the answer would be github.com/baz/bibble. I think the big question is in what cases other go commands would agree or disagree.

@freeformz
Copy link

Interestingly go (1.8 anyway, older versions have various issues with symlinks and I haven't fully re-evaluated in years) handle both cases as good as can be expected, although there are some gotchas. For instance your main package that imports child packages of the project root needs to do so by name, so imagine (using the example situation above) ~/go/src/github.com/foo/bar/cmd/thing imports github.com/baz/bibble/hahaha. Is that then part of the the current project (it is, but the symlink confuses things) ? Or something that needs to be vendored? Symlinks from outside of the $GOPATH seem to DTRT with the go tool currently w/o these complications. Maybe gps just already deals with this though?

@sdboyer
Copy link
Member

sdboyer commented Mar 3, 2017

bump

@brianstarke
Copy link
Contributor Author

Apologies, things got hectic...

The intent of this patch was only to be able to use dep from a symlinked directory, which currently just fails. So it looks like the issue with fixing that is that expectations are set when it succeeds now, and we're not clear on what those expectations would be?

@sdboyer
Copy link
Member

sdboyer commented Mar 4, 2017

OK, let's back this up a tic, and see if we can actually enumerate the possibilities here.

When the tail element of the directory hierarchy is a symlink, I can imagine five basic possibilities:

  1. cwd is not in a GOPATH, and the target directory is not in a GOPATH. (cwd is $HOME/code/dep, and dep -> $HOME/code/other)
  2. cwd is in a GOPATH, and the target directory is not in a GOPATH. (cwd is $GOPATH/src/github.com/golang/dep, and dep -> $HOME/code/dep)
  3. cwd is not in a GOPATH, and the target directory is in a GOPATH. (cwd is $HOME/code/dep, and dep -> $GOPATH/src/github.com/golang/dep)
  4. cwd is in a GOPATH, and the target directory is in the same GOPATH. (cwd is $GOPATH/src/github.com/golang/dep, and dep -> $GOPATH/src/github.com/sdboyer/dep)
  5. cwd is in a GOPATH, and the target directory is in a different GOPATH. (cwd is $GOPATH[0]/src/github.com/golang/dep, and dep -> $GOPATH[1]/src/github.com/golang/dep)

It seems to me that four of these have clear answers, and one's a bit murky.

  • For both 2 and 3, there's only one possible outcome that could work - we take the option that's in a GOPATH. This seems fine.
  • For 1, there's no GOPATH in sight, so that's clearly an error.
  • 5 is a subtly different situation, but the GOPATH we're working in matters, and because we're not crazy people who delight in inviting chaos into our lives, we need to work within one GOPATH at a time. So, let's also treat this one as an error.
  • 4 is the case @freeformz raised, and is the only one that seems a little ambiguous. Because I can't think of a real, substantial use case for doing this at the moment, I say we opt for the conservative choice and also make this one an error. Someone can always come along later and present a compelling argument, but if we allow it now without good reason, it's harder to put the cat in the bag (even if we are experimental).

That covers the cwd's tail element, which is most important here (I think). However, there's still the question of symlinks within the parent directory structure. That could be either in the cwd, or in the named target of the symlink. That is, if cwd is $HOME/code/dep, and dep -> $GOPATH/src/github.com/golang/dep, then it's possible that $HOME/code -> $HOME/other, and/or that $GOPATH/src -> /mnt/gocode/src, or whatever.

But I'm hoping we can ignore all that, and care only about whatever the literal path identified by cwd, and the literal path pointed to by the symlink. That is, no recursive link-walking to make it all the way down to a canonical path. It seems like that would be adequate for our purposes, since our main goal here is doing name inference.

At first blush, it seems like this would cover almost all typical cases, while maybe insulating us from the unknown crazy that might happen from letting filesystems become full-on graphs. The flipside, though, is that this would put us out of line with how current go tooling works, and I'm not sure how much point there is to having the package manager work in places that the rest of the toolchain doesn't.

None of this touches on relative symlinks, of course, particularly ... I need to read https://9p.io/sys/doc/lexnames.html again.

@brianstarke
Copy link
Contributor Author

Ah, now I get the issue with case 4. That's a sticky wicket. There are legitimate reasons why someone might symlink within the same GOPATH, but I agree that this should error out. What if I check for that case, and return the same error you'd get now?

@sdboyer
Copy link
Member

sdboyer commented Mar 14, 2017

Looks like a bug in tip.

@brianstarke
Copy link
Contributor Author

I suspect this may work now, can we spark a rebuild and check?

@sdboyer sdboyer closed this Mar 30, 2017
@sdboyer sdboyer reopened this Mar 30, 2017
@sdboyer
Copy link
Member

sdboyer commented Mar 30, 2017

duly kicked!

@brianstarke
Copy link
Contributor Author

Nice! Looks like it passes now.

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.

So close, so close!

context.go Outdated
@@ -75,6 +76,13 @@ func (c *Ctx) LoadProject(path string) (*Project, error) {
return nil, err
}

// the path may lie within a symlinked directory, resolve that
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'm learning that go/stdlib coding standards generally look for complete sentences w/punctuation and capitalization in comments. It's also OK to have a just a couple explanatory words - but this sorta thing (a complete sentence, but lacking proper in these details) is a no-no.

(Something like that, @rakyll ? 😄 )

context.go Outdated
return "", fmt.Errorf("''%s' is linked to another path within GOPATH", path)
}

// Return the resolved path
Copy link
Member

Choose a reason for hiding this comment

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

nit: i don't think this comment adds much

context.go Outdated

// Determine if the symlink is within the GOPATH, in which case we're not
// sure how to resolve it.
if filepath.HasPrefix(path, c.GOPATH) {
Copy link
Member

Choose a reason for hiding this comment

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

This search needs to be more expansive - it should check all other GOPATHs, not just the selected one (see newContext() for how they're selected).

To minimize our exposure to other globals, let's update newContext to store all the GOPATHs (in addition to and separately from the selected one), then do the comparison here against what's stored locally on the struct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. 👍 On it.

@sdboyer
Copy link
Member

sdboyer commented Apr 6, 2017

quick bump 😄 i'd really like to get this in soon, after such a long delay.

@brianstarke
Copy link
Contributor Author

brianstarke commented Apr 6, 2017 via email

@brianstarke
Copy link
Contributor Author

Fixed the nits and took a swing at including all GOPATHs in the search - hopefully that's what you meant

@sdboyer
Copy link
Member

sdboyer commented Apr 7, 2017

Yep! That's what I was picturing. 🦄

Let's get one last test case in to cover the situation where the link points to a different GOPATH, then we're good to go.

@sdboyer
Copy link
Member

sdboyer commented Apr 10, 2017

bump for laaaaast little bit 😄

@brianstarke
Copy link
Contributor Author

Sorry! I'm on vacation and didn't pick up a laptop until tonight. I added a case within the current test.

@sdboyer
Copy link
Member

sdboyer commented Apr 11, 2017

OK, LGTM, merging. Thanks so much, and double-thanks for your patience through the process!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants