code review 5018044: go/build: handle cgo, //build comments

go/build: handle cgo, //build comments

[rsc, 2011-09-14 17:31:28]

Hello adg (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg

[adg, 2011-09-15 01:07:05]

LGTM

http://codereview.appspot.com/5018044/diff/5001/src/cmd/goinstall/main.go
File src/cmd/goinstall/main.go (right):

http://codereview.appspot.com/5018044/diff/5001/src/cmd/goinstall/main.go#new...
src/cmd/goinstall/main.go:240: terrorf(tree, "%s: found package %s in %s, want
package %s", pkg, dirInfo.Package, dir, name)
This is an interesting new restriction? Why?

http://codereview.appspot.com/5018044/diff/5001/src/pkg/go/build/dir.go
File src/pkg/go/build/dir.go (right):

http://codereview.appspot.com/5018044/diff/5001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:99: SFiles   []string // .s files in dir
trailing space

http://codereview.appspot.com/5018044/diff/5001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:115: return d.Package == "main"
How does this interact with gotest and commands?

http://codereview.appspot.com/5018044/diff/5001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:194: for _, com := range pf.Comments {
Maybe put this in a function? It's really only asking one question.

http://codereview.appspot.com/5018044/diff/5001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:234: isCgo := false
again, a function?

[rsc, 2011-09-15 01:31:11]

Comments now, fixes in the morning.

http://codereview.appspot.com/5018044/diff/5001/src/cmd/goinstall/main.go
File src/cmd/goinstall/main.go (right):

http://codereview.appspot.com/5018044/diff/5001/src/cmd/goinstall/main.go#new...
src/cmd/goinstall/main.go:240: terrorf(tree, "%s: found package %s in %s, want
package %s", pkg, dirInfo.Package, dir, name)
On 2011/09/15 01:07:05, adg wrote:
> This is an interesting new restriction? Why?

There's no allowMain flag on ScanDir anymore.
The real point of this check is to preserve the
existing behavior of rejecting package main
when the parent != "".

I had not intended to enforce dir = package.
Will fix to just reject main.

http://codereview.appspot.com/5018044/diff/5001/src/pkg/go/build/dir.go
File src/pkg/go/build/dir.go (right):

http://codereview.appspot.com/5018044/diff/5001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:115: return d.Package == "main"
On 2011/09/15 01:07:05, adg wrote:
> How does this interact with gotest and commands?

It's not a change.
The only change is the addition of the // TODO.

I understand that we need some way to identify commands.
It might be that the current test is fine.  I do want to
think a bit more about it though, so I flagged it.

[rsc, 2011-09-15 16:11:46]

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

go/build: handle cgo, //build comments

R=adg
CC=golang-dev
http://codereview.appspot.com/5018044

http://codereview.appspot.com/5018044/diff/5001/src/pkg/go/build/dir.go
File src/pkg/go/build/dir.go (right):

http://codereview.appspot.com/5018044/diff/5001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:194: for _, com := range pf.Comments {
On 2011/09/15 01:07:05, adg wrote:
> Maybe put this in a function? It's really only asking one question.

Done.

http://codereview.appspot.com/5018044/diff/5001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:234: isCgo := false
On 2011/09/15 01:07:05, adg wrote:
> again, a function?

Not as obvious.  This is recording the imports as well 
as figuring out whether cgo is in use.  I'd rather leave it
as is.  I added an overview comment.