Resolve symlinks if project root has them. #247
Conversation
|
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! |
context.go
Outdated
| path, err := filepath.EvalSymlinks(path) | ||
|
|
||
| if err != nil { | ||
| return "", fmt.Errorf("failed attempt to resolve symlinks: %s", err) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) | |||
There was a problem hiding this comment.
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.
|
Closing and reopening to see if failure goes away |
|
Looks like that's a real error - we need to consider Windows portability here. |
|
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. |
test in to it's own test case
|
The Windows failure isn't happening anymore, hopefully this fixes the issue. I've not tried it on Windows yet. |
|
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? |
|
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 (Also, this is a contrived example - using Totally happy to provide more test cases to address your concerns, I just don't quite grok them yet. |
|
It may not matter atm, but consider this ... $GOPATH=~/go At that point, which package/project is deps dealing with? github.com/foo/bar or github.com/baz/bibble? |
|
I do agree that intra-GOPATH symlinking does make this seem confusing. To your example, the answer would be |
|
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) |
|
bump |
|
Apologies, things got hectic... The intent of this patch was only to be able to use |
|
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:
It seems to me that four of these have clear answers, and one's a bit murky.
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 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 |
|
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? |
|
Looks like a bug in tip. |
|
I suspect this may work now, can we spark a rebuild and check? |
|
duly kicked! |
|
Nice! Looks like it passes now. |
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 | |||
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good call. 👍 On it.
|
quick bump 😄 i'd really like to get this in soon, after such a long delay. |
|
Oh 💩 I didn't notice the review, my bad! I'll hit these changes in
the afternoon.
…On Thu, Apr 6, 2017 at 6:02 AM sam boyer ***@***.***> wrote:
quick bump 😄 i'd really like to get this in soon, after such a long
delay.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#247 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAY4Uh1mngIy-_NsyBpthlebSkLgbW0Zks5rtLgngaJpZM4MCgsw>
.
|
|
Fixed the nits and took a swing at including all GOPATHs in the search - hopefully that's what you meant |
|
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. |
|
bump for laaaaast little bit 😄 |
|
Sorry! I'm on vacation and didn't pick up a laptop until tonight. I added a case within the current test. |
|
OK, LGTM, merging. Thanks so much, and double-thanks for your patience through the process! |
Resolve symlinks if project root has them.
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.