code review 155050043: go/build: Return MultiplePackageError on importing a di...

go/build: Return MultiplePackageError on importing a dir containing multiple packages

When the Import function in go/build encounters a directory
without any buildable Go source files, it returns a handy
NoGoError. Now if, instead it encounters multiple Go source files
from multiple packages, it returns a handy MultiplePackageError.

A new test for NoGoError and MultiplePackageError is also provided.

Fixes issue 8286.

[jfrederich, 2014-10-07 15:54:46]

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

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

[rsc, 2014-10-07 16:19:00]

please use

// MultiplePackageError describes a directory containing
// buildable Go source files for multiple packages.
type MultiplePackageError struct {
    Dir string // directory containing files
    Packages []string // package names found
    Files []string // file mentioning each package: Files[i] declares
package Packages[i]
}

where right now it will always be the case that len(Packages) = len(Files)
= 2.

please keep the returned error string *exactly* the same as it is today.
the CL as written throws away useful information.

[jfrederich, 2014-10-07 16:22:51]

On 2014/10/07 16:19:00, rsc wrote:
> please use
> 
> // MultiplePackageError describes a directory containing
> // buildable Go source files for multiple packages.
> type MultiplePackageError struct {
>     Dir string // directory containing files
>     Packages []string // package names found
>     Files []string // file mentioning each package: Files[i] declares
> package Packages[i]
> }
> 
> where right now it will always be the case that len(Packages) = len(Files)
> = 2.
> 
> please keep the returned error string *exactly* the same as it is today.
> the CL as written throws away useful information.

okay, you are right

[jfrederich, 2014-10-08 07:18:39]

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

Please take another look.

[adg, 2014-10-08 15:10:18]

Please rename the file ".empty" to "dummy" (no leading dot).

The Go tool doesn't care about dotfiles. A file is a file.

https://codereview.appspot.com/155050043/diff/60001/src/go/build/build.go
File src/go/build/build.go (right):

https://codereview.appspot.com/155050043/diff/60001/src/go/build/build.go#new...
src/go/build/build.go:425: Files    []string // file mentioning each package:
Files[i] desclares package Packages[i]
s/desclares/declares/

https://codereview.appspot.com/155050043/diff/60001/src/go/build/build.go#new...
src/go/build/build.go:429: return fmt.Sprintf("found packages %s (%s) and %s
(%s) in %s", e.Packages[0], e.Files[0], e.Packages[1], e.Files[1], e.Dir)
add a comment above this line

// Error string limited to two entires for compatibility.

https://codereview.appspot.com/155050043/diff/60001/src/go/build/build_test.go
File src/go/build/build_test.go (right):

https://codereview.appspot.com/155050043/diff/60001/src/go/build/build_test.g...
src/go/build/build_test.go:91: t.Fatal(`Import("testdata/empty") doesn't
returned NoGoError.`)
s/doesn't returned/did not return/

https://codereview.appspot.com/155050043/diff/60001/src/go/build/build_test.g...
src/go/build/build_test.go:98: t.Fatal(`Import("testdata/multi") doesn't
returned MultiplePackageError.`)
ditto

https://codereview.appspot.com/155050043/diff/60001/src/go/build/testdata/mul...
File src/go/build/testdata/multi/file_appengine.go (right):

https://codereview.appspot.com/155050043/diff/60001/src/go/build/testdata/mul...
src/go/build/testdata/multi/file_appengine.go:3: package appengine
please pick a less special name for this package

like "test_package"

[jfrederich, 2014-10-08 19:58:30]

On 2014/10/08 15:10:18, adg wrote:
> Please rename the file ".empty" to "dummy" (no leading dot).
> 
> The Go tool doesn't care about dotfiles. A file is a file.
> 
> https://codereview.appspot.com/155050043/diff/60001/src/go/build/build.go
> File src/go/build/build.go (right):
> 
>
https://codereview.appspot.com/155050043/diff/60001/src/go/build/build.go#new...
> src/go/build/build.go:425: Files    []string // file mentioning each package:
> Files[i] desclares package Packages[i]
> s/desclares/declares/
> 
>
https://codereview.appspot.com/155050043/diff/60001/src/go/build/build.go#new...
> src/go/build/build.go:429: return fmt.Sprintf("found packages %s (%s) and %s
> (%s) in %s", e.Packages[0], e.Files[0], e.Packages[1], e.Files[1], e.Dir)
> add a comment above this line
> 
> // Error string limited to two entires for compatibility.
> 
> https://codereview.appspot.com/155050043/diff/60001/src/go/build/build_test.go
> File src/go/build/build_test.go (right):
> 
>
https://codereview.appspot.com/155050043/diff/60001/src/go/build/build_test.g...
> src/go/build/build_test.go:91: t.Fatal(`Import("testdata/empty") doesn't
> returned NoGoError.`)
> s/doesn't returned/did not return/
> 
>
https://codereview.appspot.com/155050043/diff/60001/src/go/build/build_test.g...
> src/go/build/build_test.go:98: t.Fatal(`Import("testdata/multi") doesn't
> returned MultiplePackageError.`)
> ditto
> 
>
https://codereview.appspot.com/155050043/diff/60001/src/go/build/testdata/mul...
> File src/go/build/testdata/multi/file_appengine.go (right):
> 
>
https://codereview.appspot.com/155050043/diff/60001/src/go/build/testdata/mul...
> src/go/build/testdata/multi/file_appengine.go:3: package appengine
> please pick a less special name for this package
> 
> like "test_package"

Andrew thanks for your comments.

[jfrederich, 2014-10-08 20:01:29]

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

Please take another look.

[adg, 2014-10-09 07:48:23]

The empty directory seems to have disappeared?

https://codereview.appspot.com/155050043/diff/80001/src/go/build/build.go
File src/go/build/build.go (right):

https://codereview.appspot.com/155050043/diff/80001/src/go/build/build.go#new...
src/go/build/build.go:425: Files    []string // file mentioning each package:
Files[i] declares package Packages[i]
s/file mentioning each package/corresponding files/

[jfrederich, 2014-10-09 08:17:15]

On 2014/10/09 07:48:23, adg wrote:
> The empty directory seems to have disappeared?
> 
Sorry, I forgot to run command 'hg file'.

[jfrederich, 2014-10-09 08:17:36]

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

Please take another look.

[adg, 2014-10-09 08:44:54]

LGTM

But Russ should approve/submit. Thanks.

[jfrederich, 2014-10-09 09:16:25]

Fine, I'll move on to next issue.

Jens

On Thu, Oct 9, 2014 at 10:44 AM,  <adg@golang.org> wrote:
> LGTM
>
> But Russ should approve/submit. Thanks.
>
> https://codereview.appspot.com/155050043/

[jfrederich, 2014-10-14 15:11:53]

> > But Russ should approve/submit. Thanks.
> >
> > https://codereview.appspot.com/155050043/

Russ, is it possible to get it submitted?

[rsc, 2014-10-15 03:08:40]

LGTM

[rsc, 2014-10-15 03:25:03]

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

go/build: Return MultiplePackageError on importing a dir containing multiple
packages

When the Import function in go/build encounters a directory
without any buildable Go source files, it returns a handy
NoGoError. Now if, instead it encounters multiple Go source files
from multiple packages, it returns a handy MultiplePackageError.

A new test for NoGoError and MultiplePackageError is also provided.

Fixes issue 8286.

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

Committer: Russ Cox <rsc@golang.org>

[gobot, 2014-10-15 03:37:36]

This CL appears to have broken the windows-386 builder.
See http://build.golang.org/log/77a6724c0757e19224d8a13ae85d859c807d314f