New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show a clear error message when trying to 'dep init' in $GOPATH/src #529

Merged
merged 5 commits into from May 11, 2017

Conversation

Projects
None yet
3 participants
@ibrasho
Copy link
Collaborator

ibrasho commented May 8, 2017

This should solve #471.

The panic was caused by line 186 trying to get a slice starting at larger index than the path string length:

return filepath.ToSlash(path[len(srcprefix):]), nil

But len(srcprefix) will be larger than the length of path since it will include an additional filepath.Separator

I don't like the message honestly, but it's the best I could come up with currently. 馃槥

@googlebot googlebot added the cla: yes label May 8, 2017

@sdboyer
Copy link
Member

sdboyer left a comment

little things

context.go Outdated
@@ -179,6 +179,10 @@ func (c *Ctx) resolveProjectRoot(path string) (string, error) {
//
// The second returned string indicates which GOPATH value was used.
func (c *Ctx) SplitAbsoluteProjectRoot(path string) (string, error) {
if path == filepath.Join(c.GOPATH, "src") {

This comment has been minimized.

@sdboyer

sdboyer May 8, 2017

Member

We need to be a little tricksier about this, I think:

  1. Do the filepath.Join() and assign into a var
  2. Check if lens of path and the new var are the same
  3. Compare path and new var with internal.HasFilepathPrefix()

That should take care of any icky capitalization issues.

This comment has been minimized.

@sdboyer

sdboyer May 8, 2017

Member

(It might be necessary to account for a trailing slash...if so, above algorithm may need tweaking)

This comment has been minimized.

@ibrasho

ibrasho May 8, 2017

Collaborator

Wouldn't comparing the lens of path and $GOPATH/src cause this error to be triggered if they happen to be of the same length. (e.g. if $GOPATH=/Users/user/Code/go/src and the dep init is run in /Users/user/Code/marvel...)?

I originally compared the lens and then switched to comparing the strings.

This comment has been minimized.

@ibrasho

ibrasho May 8, 2017

Collaborator

I made the following changes:
Instead of comparing the path to $GOPATH/src, we start by ensuring we are actually within $GOPATH/src (through internal.HasFilepathPrefix).
If that's the case, we check if path is shorter than or equal to srcprefix (at most it could be shorter than srcprefix by the len of filepath.Separator, otherwise if it has a trailing slash it would be equal). If this is true, then the project root is $GOPATH/src and we return an error.

This should handle case sensitivity and trailing separators if my assumptions are correct. @sdboyer What do you think?

context.go Outdated
@@ -179,6 +179,10 @@ func (c *Ctx) resolveProjectRoot(path string) (string, error) {
//
// The second returned string indicates which GOPATH value was used.
func (c *Ctx) SplitAbsoluteProjectRoot(path string) (string, error) {
if path == filepath.Join(c.GOPATH, "src") {
return "", errors.Errorf("Initializing a project in $GOPATH/src directly is not supported currently")

This comment has been minimized.

@sdboyer

sdboyer May 8, 2017

Member

just nits 馃槃

  1. use errors.New() instead of errors.Errorf()
  2. wordsmithing: "Initializing a project directly within $GOPATH/src is not currently supported"
@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 8, 2017

yaaaaayyyy first time contributions!

also, this needs a test 馃槃

@ibrasho ibrasho force-pushed the ibrasho-forks:handle-init-in-GOPATH/src-issue branch from be87458 to a11483a May 8, 2017

context.go Outdated
@@ -181,6 +181,11 @@ func (c *Ctx) resolveProjectRoot(path string) (string, error) {
func (c *Ctx) SplitAbsoluteProjectRoot(path string) (string, error) {
srcprefix := filepath.Join(c.GOPATH, "src") + string(filepath.Separator)
if internal.HasFilepathPrefix(path, srcprefix) {
if len(path) <= len(srcprefix) {
// the project is directly within $GOPATH/src

This comment has been minimized.

@ibrasho

ibrasho May 8, 2017

Collaborator

This comment looks redundant now...

This comment has been minimized.

@sdboyer

sdboyer May 9, 2017

Member

err - i was hoping you'd reword the error message itself with the text i wrote, not add a comment 馃槃

@ibrasho

This comment has been minimized.

Copy link
Collaborator

ibrasho commented May 9, 2017

I changed the error message to: "projects directly within $GOPATH/src are not supported currently"
I choose to not specify initializing since this error could occur in when running dep remove.

I hope this is ok?

@ibrasho

This comment has been minimized.

Copy link
Collaborator

ibrasho commented May 9, 2017

I also still not convinced about the test case I wrote. It used to cause a panic, so should I recover the panic and show a proper message?

I was thinking I could add this before the test case to do that:

defer func() {
	if r := recover(); r != nil {
		t.Fatalf("should have gotten an error but got a panic: %s", r)
	}
}()
@sdboyer
Copy link
Member

sdboyer left a comment

nearly there, just some little nits (sorry, i now understand your issue with my initial suggestion)

context.go Outdated
@@ -181,6 +181,10 @@ func (c *Ctx) resolveProjectRoot(path string) (string, error) {
func (c *Ctx) SplitAbsoluteProjectRoot(path string) (string, error) {
srcprefix := filepath.Join(c.GOPATH, "src") + string(filepath.Separator)
if internal.HasFilepathPrefix(path, srcprefix) {
if len(path) <= len(srcprefix) {
return "", errors.New("projects directly within $GOPATH/src are not supported currently")

This comment has been minimized.

@sdboyer

sdboyer May 10, 2017

Member

I changed the error message to: "projects directly within $GOPATH/src are not supported currently"
I choose to not specify initializing since this error could occur in when running dep remove.

Ahh, I'm sorry, I wasn't understanding the issue with my suggested wording until now. My phrasing suggested that this was only about initializing a project, but the wording needs to be equally applicable for other dep subcommands.

Here's a revised suggestion:

"dep does not currently support using $GOPATH/src as the project root."

// test where it should return error
got, err := depCtx.SplitAbsoluteProjectRoot("tra/la/la/la")
// test where it should return an error when directly within $GOPATH/src
got, err := depCtx.SplitAbsoluteProjectRoot(filepath.Join(depCtx.GOPATH, "src"))
if err == nil {

This comment has been minimized.

@sdboyer

sdboyer May 10, 2017

Member

nit: let's do at least a cursory check for the error text we expect - e.g., if err == nil || strings.Contains(err.Error(), "$GOPATH/src") {, to give us a bit of an assurance that we're getting the error we expect to.

This comment has been minimized.

@ibrasho

ibrasho May 10, 2017

Collaborator

it should be if err == nil || !strings.Contains(err.Error(), "$GOPATH/src") { right?
If strings.Contains(err.Error(), "$GOPATH/src") it try then we go the correct error.

This comment has been minimized.

@sdboyer

sdboyer May 11, 2017

Member

yes indeed, thank you 馃槃

if err == nil {
t.Fatalf("should have gotten error but did not for tra/la/la/la: %s", got)
t.Fatalf("should have gotten an error but did not for %s", got)

This comment has been minimized.

@sdboyer

sdboyer May 10, 2017

Member

Error should be clearer about the exact we're testing - "should have gotten an error for use directly in $GOPATH/src, but got %s"

This comment has been minimized.

@ibrasho

ibrasho May 10, 2017

Collaborator

Another thing... This used to cause a panic. Should I try to recover and handle that in the test?

This comment has been minimized.

@sdboyer

sdboyer May 11, 2017

Member

Nope, the panic was showing us a bug; this fixes that bug. Panics aren't like exceptions - they indicate bugs 馃槃

@ibrasho ibrasho force-pushed the ibrasho-forks:handle-init-in-GOPATH/src-issue branch from df21c86 to f81437d May 10, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 11, 2017

looks good, in we go - happy first contribution!!! 馃帀 馃帀

@sdboyer sdboyer merged commit 30cce2a into golang:master May 11, 2017

4 checks passed

cla/google All necessary CLAs are signed
codeclimate no new or fixed issues
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ibrasho ibrasho deleted the ibrasho-forks:handle-init-in-GOPATH/src-issue branch May 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment