code review 89280043: cmd/go: do not miss an error if import path contains "c...

cmd/go: do not miss an error if import path contains "cmd/something"

Fixes issue 7638

[atom, 2014-04-18 10:48:24]

Hello rsc@golang.org, adg@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[adg, 2014-04-21 05:19:03]

https://codereview.appspot.com/89280043/diff/60001/src/cmd/go/pkg.go
File src/cmd/go/pkg.go (right):

https://codereview.appspot.com/89280043/diff/60001/src/cmd/go/pkg.go#newcode142
src/cmd/go/pkg.go:142: hard          bool     // whether the error is soft or
hard, soft errrors are ignored in some places
maybe this should be

isInvalidPath bool

[rh, 2014-04-21 10:36:42]

https://codereview.appspot.com/89280043/diff/60001/src/cmd/go/pkg.go
File src/cmd/go/pkg.go (right):

https://codereview.appspot.com/89280043/diff/60001/src/cmd/go/pkg.go#newcode142
src/cmd/go/pkg.go:142: hard          bool     // whether the error is soft or
hard, soft errrors are ignored in some places
Replace comma with semicolon, and errrors with errors?

[rh, 2014-04-21 10:46:53]

https://codereview.appspot.com/89280043/diff/60001/src/cmd/go/pkg.go
File src/cmd/go/pkg.go (right):

https://codereview.appspot.com/89280043/diff/60001/src/cmd/go/pkg.go#newcode142
src/cmd/go/pkg.go:142: hard          bool     // whether the error is soft or
hard, soft errrors are ignored in some places
On 2014/04/21 05:19:03, adg wrote:
> maybe this should be
> 
> isInvalidPath bool

isReservedPath?

[atom, 2014-04-21 11:00:55]

https://codereview.appspot.com/89280043/diff/60001/src/cmd/go/pkg.go
File src/cmd/go/pkg.go (right):

https://codereview.appspot.com/89280043/diff/60001/src/cmd/go/pkg.go#newcode142
src/cmd/go/pkg.go:142: hard          bool     // whether the error is soft or
hard, soft errrors are ignored in some places
On 2014/04/21 10:36:43, rh wrote:
> Replace comma with semicolon, and errrors with errors?

Done.

https://codereview.appspot.com/89280043/diff/60001/src/cmd/go/pkg.go#newcode142
src/cmd/go/pkg.go:142: hard          bool     // whether the error is soft or
hard, soft errrors are ignored in some places
On 2014/04/21 05:19:03, adg wrote:
> maybe this should be
> 
> isInvalidPath bool

I am not sure about this as well. It would be better to properly handle the
errors instead of quickly patching struct PackageError.

If we agree that properly handling the errors is the preferred approach I will
give it a try. This will probably involve changes to some function signatures.

[rh, 2014-04-21 11:10:46]

On 2014/04/21 11:00:55, atom wrote:
> https://codereview.appspot.com/89280043/diff/60001/src/cmd/go/pkg.go
> File src/cmd/go/pkg.go (right):
> 
>
https://codereview.appspot.com/89280043/diff/60001/src/cmd/go/pkg.go#newcode142
> src/cmd/go/pkg.go:142: hard          bool     // whether the error is soft or
> hard, soft errrors are ignored in some places
> On 2014/04/21 10:36:43, rh wrote:
> > Replace comma with semicolon, and errrors with errors?
> 
> Done.
> 
>
https://codereview.appspot.com/89280043/diff/60001/src/cmd/go/pkg.go#newcode142
> src/cmd/go/pkg.go:142: hard          bool     // whether the error is soft or
> hard, soft errrors are ignored in some places
> On 2014/04/21 05:19:03, adg wrote:
> > maybe this should be
> > 
> > isInvalidPath bool
> 
> I am not sure about this as well. It would be better to properly handle the
> errors instead of quickly patching struct PackageError.
> 
> If we agree that properly handling the errors is the preferred approach I will
> give it a try. This will probably involve changes to some function signatures.

Thank you for your changes.  FWIW I'd go the proper route, but I defer to those
who must live in the codebase.

[adg, 2014-04-22 02:50:55]

https://codereview.appspot.com/89280043/diff/60001/src/cmd/go/pkg.go
File src/cmd/go/pkg.go (right):

https://codereview.appspot.com/89280043/diff/60001/src/cmd/go/pkg.go#newcode142
src/cmd/go/pkg.go:142: hard          bool     // whether the error is soft or
hard, soft errrors are ignored in some places
On 2014/04/21 11:00:56, atom wrote:
> On 2014/04/21 05:19:03, adg wrote:
> > maybe this should be
> > 
> > isInvalidPath bool
> 
> I am not sure about this as well. It would be better to properly handle the
> errors instead of quickly patching struct PackageError.
> 
> If we agree that properly handling the errors is the preferred approach I will
> give it a try. This will probably involve changes to some function signatures.

There's already some precedent here with isImportCycle.

I think isInvalidPath is probably the simplest way forward, and the
corresponding change in get.go makes more sense ("if there's an error and it's
not an "invalid path" error, proceed with downloading the package").

But I'll defer to Russ as he wrote the code originally and may have another
idea.

[bradfitz, 2014-05-13 21:21:21]

Ping. Is this blocked on adg or rsc?

[adg, 2014-05-13 21:24:19]

On 2014/05/13 21:21:21, bradfitz wrote:
> Ping. Is this blocked on adg or rsc?

rsc

[rsc, 2014-05-28 03:55:10]

LGTM
not great but good enough

[rsc, 2014-05-28 03:58:03]

*** Submitted as https://code.google.com/p/go/source/detail?r=f9c13f6b683b ***

cmd/go: do not miss an error if import path contains "cmd/something"

Fixes issue 7638

LGTM=rsc
R=rsc, adg, robert.hencke, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/89280043

Committer: Russ Cox <rsc@golang.org>

[gobot, 2014-05-28 04:14:08]

This CL appears to have broken the netbsd-amd64-bsiegert builder.
See http://build.golang.org/log/626991bb37db7487741e066b87a4e3dfd09879c6