code review 120600043: cmd/go: implement 'internal' convention

cmd/go: implement 'internal' convention

See golang.org/s/go14internal for design.

[rsc, 2014-08-05 19:48:17]

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

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

[r, 2014-08-05 20:46:53]

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

https://codereview.appspot.com/120600043/diff/60001/src/cmd/go/pkg.go#newcode247
src/cmd/go/pkg.go:247: if perr := checkInternal(srcDir, p, stk); perr != nil {
weird names. perr isn't really an error and checkInternal is ambiguous as to
whether internal is good or bad.
moreover, perr isn't even used. why not just

if !canImport(srcDir, p, stk) {
  return nil // or something descriptive
}

https://codereview.appspot.com/120600043/diff/60001/src/cmd/go/pkg.go#newcode335
src/cmd/go/pkg.go:335: fmt.Println("checkinternal FAIL", p.ImportPath, srcDir,
*stk)
d

https://codereview.appspot.com/120600043/diff/60001/src/cmd/go/pkg.go#newcode344
src/cmd/go/pkg.go:344: return &perr
callers don't use this value. drop it

https://codereview.appspot.com/120600043/diff/60001/src/cmd/go/testdata/testi...
File src/cmd/go/testdata/testinternal/p.go (right):

https://codereview.appspot.com/120600043/diff/60001/src/cmd/go/testdata/testi...
src/cmd/go/testdata/testinternal/p.go:3: import _ "net/http/internal"
ha!

[rsc, 2014-08-06 03:38:00]

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

https://codereview.appspot.com/120600043/diff/60001/src/cmd/go/pkg.go#newcode247
src/cmd/go/pkg.go:247: if perr := checkInternal(srcDir, p, stk); perr != nil {
perr is used. it has the error message inside it.

[adg, 2014-08-06 03:48:43]

No test for the success case?

package "foo/bar" successfully importing "foo/internal/quux"?

[adg, 2014-08-06 03:50:29]

It would also be nice to test for failure when "foo/bar" and
"foo/internal/quux" are in separate workspaces.

I know this is obviously correct given the code as-is, but this might stop
us from breaking it later.


On 6 August 2014 13:48, <adg@golang.org> wrote:

> No test for the success case?
>
> package "foo/bar" successfully importing "foo/internal/quux"?
>
> https://codereview.appspot.com/120600043/
>

[r, 2014-08-06 14:25:18]

LGTM but i don't like the name checkInternal.

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

https://codereview.appspot.com/120600043/diff/60001/src/cmd/go/pkg.go#newcode247
src/cmd/go/pkg.go:247: if perr := checkInternal(srcDir, p, stk); perr != nil {
On 2014/08/06 03:37:59, rsc wrote:
> perr is used. it has the error message inside it.

not sure how i missed the 'return perr'. 

ok. i still don't like the name and the structure is weird. why doesn't
loadImport return an error instead of burying it? but i suspect both are
unlikely to change.

[rsc, 2014-08-06 20:33:17]

The Error being part of the Package simplifies the rest of the program: it
allows gathering as many errors as possible during a load and gives a place
to hang the associated context. I changed checkInternal to
disallowInternal, and it either returns p (not nil) or perr. That should be
a little clearer.

The internal rule only applies to $GOROOT for 1.4, something I forgot to
include but that is now there. Given that, it is not possible to test for
success from outside $GOROOT. Brad has provided a test inside $GOROOT
(net/http and net/http/httputil import net/http/internal). I added a test
that internal has no meaning outside $GOROOT.

Russ

[rsc, 2014-08-06 20:45:10]

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

cmd/go: implement 'internal' convention

See golang.org/s/go14internal for design.

LGTM=r
R=r, adg
CC=golang-codereviews
https://codereview.appspot.com/120600043

[gobot, 2014-08-06 21:00:56]

This CL appears to have broken the windows-amd64 builder.
See http://build.golang.org/log/c1b9367d30e41ef3c429f4336be5ef341ff1eec9