code review 3891042: goinstall: Initial support for cgo packages

goinstall: Initial support for cgo packages

This will modify goinstall so that it is able to build and
install simple cgo-based pacakges as well.

As a follow up change, a way to tweak CGO_LDFLAGS and
CGO_CFLAGS should be introduced.  I'm considering extracting
those out of the existing Makefile, if available, but that's
still up for discussion and independent of this first branch.

[niemeyer, 2011-01-11 13:20:58]

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

I'd like you to review this change.

[rsc, 2011-01-11 16:20:57]

Looks reasonable.  I had started this at
http://codereview.appspot.com/1853050
but this looks a little cleaner.

As you note, it's not useful without being able
to set CGO_LDFLAGS, etc.  I do not want to read
them out of existing Makefiles.  I think what we
need is some kind of notation (probably a special
comment) to include that information in the cgo 
input files instead.  I haven't figured out what
that notation should be, though, as it must be
able to select different results based on $GOOS
and possibly $GOARCH.

http://codereview.appspot.com/3891042/diff/2001/src/cmd/goinstall/make.go
File src/cmd/goinstall/make.go (right):

http://codereview.appspot.com/3891042/diff/2001/src/cmd/goinstall/make.go#new...
src/cmd/goinstall/make.go:28: cmd := []string{"gomake", "-C", dir}
why -C dir?
run already runs it in dir.

http://codereview.appspot.com/3891042/diff/2001/src/cmd/goinstall/make.go#new...
src/cmd/goinstall/make.go:59: if len(cgoFiles) > 0 {
Is this a typo?  I can't figure out why len(cgoFiles) is relevant here.
If it should be len(oFiles), drop the if and let the for run an empty loop.

http://codereview.appspot.com/3891042/diff/2001/src/cmd/goinstall/parse.go
File src/cmd/goinstall/parse.go (right):

http://codereview.appspot.com/3891042/diff/2001/src/cmd/goinstall/parse.go#ne...
src/cmd/goinstall/parse.go:22: imports map[string][]string // Map with
{importPath: []filesImportingPath}
This is probably a bit too general.
I'd be happy to just have

cgoFiles []string  // .go files that import "C"

http://codereview.appspot.com/3891042/diff/2001/src/cmd/goinstall/parse.go#ne...
src/cmd/goinstall/parse.go:27: // listDir returns a structure with details about
the Go content found
s/listDir/scanDir/

List sounds like it is running ls.

[niemeyer, 2011-01-11 17:14:07]

> As you note, it's not useful without being able
> to set CGO_LDFLAGS, etc.  I do not want to read
> them out of existing Makefiles.  I think what we
> need is some kind of notation (probably a special
> comment) to include that information in the cgo 
> input files instead.  I haven't figured out what
> that notation should be, though, as it must be
> able to select different results based on $GOOS
> and possibly $GOARCH.

Here are a few alternatives I can think of:

- Using a comment within the code as you suggest.
- Using a metadata file (e.g. ".goinstall") with JSON or
  some other format.
- Using a "Makefile.goin" file which is included before
  Make.pkg if found.

The advantage of having that in a Makefile (either parsing or
with an inclusion like Makefile.goin) is that local building
can use the same information, so it's less error prone, but
I'll be happy to pursue any means for doing it on a follow up
branch once I understand your concern.

> why -C dir?
> run already runs it in dir.

Oops.. fixed, thanks.

> src/cmd/goinstall/make.go:59: if len(cgoFiles) > 0 {
> Is this a typo?  I can't figure out why len(cgoFiles) is relevant here.
> If it should be len(oFiles), drop the if and let the for run an empty loop.

Not a typo, but perhaps a mistake. If there are no .go files in
the current project importing from "C", does it make sense to
define CGO_OFILES?

I've added a comment explaining.  If that's wrong, please let me
know and I'll drop the check.

> This is probably a bit too general.
> I'd be happy to just have
> 
> cgoFiles []string  // .go files that import "C"

Done.  Also turned imports into a []string.

> s/listDir/scanDir/
> 
> List sounds like it is running ls.

Indeed. Changed.

PTAL

[rsc, 2011-01-11 17:31:31]

> Here are a few alternatives I can think of:
>
> - Using a comment within the code as you suggest.
> - Using a metadata file (e.g. ".goinstall") with JSON or
>  some other format.
> - Using a "Makefile.goin" file which is included before
>  Make.pkg if found.
>
> The advantage of having that in a Makefile (either parsing or
> with an inclusion like Makefile.goin) is that local building
> can use the same information, so it's less error prone, but
> I'll be happy to pursue any means for doing it on a follow up
> branch once I understand your concern.

It's a delicate position, but I don't want to tie anything to make.
As it stands now I can change the makefile conventions or
even replace make entirely and not break any goinstallable
packages (as long as I update goinstall).

The majority of the important information about building
(things like dependencies) is already learned from reading
the source code.  I would prefer to have the cgo information
in the source code too, and then there is no separate
metadata file to maintain.

Let's not worry about this for this CL.
Other related issues include how goinstall should figure
out which files to build when there is, say, per-architecture
assembly.

> Not a typo, but perhaps a mistake. If there are no .go files in
> the current project importing from "C", does it make sense to
> define CGO_OFILES?

That depends on whether goinstall should be able to
build packages that include C files for 6c.  The assumption
is that if using cgo then the C files are for gcc, which is
a safe bet.  Without cgo the odds are more in favor of 6c.

I'd just emit an error for now: C files in non-cgo package.

Russ

[niemeyer, 2011-01-11 18:01:24]

> It's a delicate position, but I don't want to tie anything to make.
> As it stands now I can change the makefile conventions or
> even replace make entirely and not break any goinstallable
> packages (as long as I update goinstall).
>
> The majority of the important information about building
> (things like dependencies) is already learned from reading
> the source code.  I would prefer to have the cgo information
> in the source code too, and then there is no separate
> metadata file to maintain.

Understood.

> Let's not worry about this for this CL.
> Other related issues include how goinstall should figure
> out which files to build when there is, say, per-architecture
> assembly.

In this case, one idea would be to use some directory structure
after the architecture name, and then apply scanDir() or
something similar to the matching directory only, if found.

Different CL, either way.

(...)
> I'd just emit an error for now: C files in non-cgo package.

Sounds good.  That's done.

[rsc, 2011-01-11 18:13:42]

LGTM

[rsc, 2011-01-11 18:14:54]

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

goinstall: preliminary support for cgo packages

Can handle cgo packages now but only if they
do not need to set CGO_LDFLAGS and CGO_CFLAGS.

R=adg, rsc
CC=golang-dev
http://codereview.appspot.com/3891042

Committer: Russ Cox <rsc@golang.org>