code review 4433047: go/build: new package for building go programs

go/build: new package for building go programs

[rsc, 2011-04-17 13:40:24]

http://codereview.appspot.com/4433047/diff/4/src/pkg/go/make/build.go
File src/pkg/go/make/build.go (right):

http://codereview.appspot.com/4433047/diff/4/src/pkg/go/make/build.go#newcode20
src/pkg/go/make/build.go:20: switch runtime.GOARCH {
wrong
use os.Getenv("GOARCH") first.
runtime.GOARCH describes the current binary,
not the binary you are trying to build.

http://codereview.appspot.com/4433047/diff/4/src/pkg/go/make/dir.go
File src/pkg/go/make/dir.go (right):

http://codereview.appspot.com/4433047/diff/4/src/pkg/go/make/dir.go#newcode36
src/pkg/go/make/dir.go:36: // - Files in package main (unless allowMain is true)
indent block
s/Files/files/g

http://codereview.appspot.com/4433047/diff/4/src/pkg/go/make/dir.go#newcode38
src/pkg/go/make/dir.go:38: // - Files starting with _ (temporary)
Files starting with . too

http://codereview.appspot.com/4433047/diff/4/src/pkg/go/make/dir.go#newcode39
src/pkg/go/make/dir.go:39: // - Files containing .cgo in their names
This is a strange exception.  Those are intermediate files
and shouldn't be in the directory anyway.

http://codereview.appspot.com/4433047/diff/4/src/pkg/go/make/dir.go#newcode54
src/pkg/go/make/dir.go:54: goFiles := make([]string, 0, len(dirs))
Instead of all these parallel variables just declare

var d DirInfo

and fill it in directly,

and then at the bottom return &d, nil

[adg, 2011-04-18 03:27:33]

http://codereview.appspot.com/4433047/diff/4/src/pkg/go/make/build.go
File src/pkg/go/make/build.go (right):

http://codereview.appspot.com/4433047/diff/4/src/pkg/go/make/build.go#newcode20
src/pkg/go/make/build.go:20: switch runtime.GOARCH {
On 2011/04/17 13:40:24, rsc wrote:
> wrong
> use os.Getenv("GOARCH") first.
> runtime.GOARCH describes the current binary,
> not the binary you are trying to build.

Done.

http://codereview.appspot.com/4433047/diff/4/src/pkg/go/make/dir.go
File src/pkg/go/make/dir.go (right):

http://codereview.appspot.com/4433047/diff/4/src/pkg/go/make/dir.go#newcode36
src/pkg/go/make/dir.go:36: // - Files in package main (unless allowMain is true)
On 2011/04/17 13:40:24, rsc wrote:
> indent block
> s/Files/files/g

Done.

http://codereview.appspot.com/4433047/diff/4/src/pkg/go/make/dir.go#newcode38
src/pkg/go/make/dir.go:38: // - Files starting with _ (temporary)
On 2011/04/17 13:40:24, rsc wrote:
> Files starting with . too

Done.

http://codereview.appspot.com/4433047/diff/4/src/pkg/go/make/dir.go#newcode39
src/pkg/go/make/dir.go:39: // - Files containing .cgo in their names
On 2011/04/17 13:40:24, rsc wrote:
> This is a strange exception.  Those are intermediate files
> and shouldn't be in the directory anyway.

Agree. And the subsequent searches will ignore .cgo files anyway.

The code in this file is actually copied from src/cmd/goinstall/parse.go,
including the comments. I hadn't intended for you to review it, or else I'd have
mentioned that. Good catch regardless.

http://codereview.appspot.com/4433047/diff/4/src/pkg/go/make/dir.go#newcode54
src/pkg/go/make/dir.go:54: goFiles := make([]string, 0, len(dirs))
On 2011/04/17 13:40:24, rsc wrote:
> Instead of all these parallel variables just declare
> 
> var d DirInfo
> 
> and fill it in directly,
> 
> and then at the bottom return &d, nil

Done.

[adg, 2011-05-24 07:41:04]

Russ - go/make now builds packages with go, cgo, and assembly files.

There's still some hard-coded stuff in there that should be derived from the
environment - your guidance on the general approach would be good at this point.
Particularly with the cgo stuff.

[rsc, 2011-05-25 22:22:45]

Let's talk about the build.go API in a bit.
All the supporting stuff around it looks good.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/Makefile
File src/pkg/go/make/Makefile (right):

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/Makefile#new...
src/pkg/go/make/Makefile:17: syslist.go:
syslist.go: ../../../Make.inc Makefile

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build.go
File src/pkg/go/make/build.go (right):

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:25: var arch string
Please make this a separate

// ArchChar returns the architecture character for the given goarch.
// For example, ArchChar("amd64") returns "6".
func ArchChar(goarch string) (string, os.Error) {
}

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:37: const odir = "_obj/"
s/odir/obj/

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:48: // build cgo files
This is mixing planning and execution.  Splitting them
will make the goinstall -n mode easier to write and also
will reduce a lot of repetition here.

Let's talk about this later.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:165: return cp(targ, filepath.Join(dir, obj))
Why not just write directly to targ?

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build_test.go
File src/pkg/go/make/build_test.go (right):

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build_test.g...
src/pkg/go/make/build_test.go:15: {"pkg", "path"},
var buildDirs = []string{
    "src/pkg/path"
    "src/cmd/gofix"
   "src/pkg/big"
   "src/pkg/go/make/cgotest"
}

and adjust below:
    p := filepath.Join(runtime.GOROOT(), filepath.FromSlash(d))

I refuse to be pushed around by Windows.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build_test.g...
src/pkg/go/make/build_test.go:29: func testBuild(t *testing.T, dir, targ string)
{
inline in the one place it is called

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go
File src/pkg/go/make/dir.go (right):

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:20: GoFiles  []string // .go files within dir (excluding
CgoFiles)
s/within/in/g

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:33: // in the given directory. The list of files will NOT
contain the
The file lists exclude:

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:38: // 	- files starting with _ or . (temporary)
s/ (temporary)//

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:40: // The imports map keys are package paths imported by
listed Go files,
delete
(not relevant here)

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:54: importsm := make(map[string]bool)
s/importsm/imported/

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:107: for _, decl := range pf.Decls {
for _, spec := range pf.Imports {

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:110: unquoted, err := strconv.Unquote(quoted)
s/unquoted/path/

[kevlar, 2011-05-26 01:02:08]

FYI

I'm quite looking forward to this utility :).

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go
File src/pkg/go/make/dir.go (right):

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:36: //	- files in package main (unless allowMain is true)
or package documentation?

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:39: //
I don't know how to word it, but mention here that the _$GOOS or _$GOARCH files
are also skipped.  Unless we don't want to document that feature.

Maybe:
- files ending in a different _$GOOS and/or $_GOARCH

[adg, 2011-05-26 06:48:02]

PTAL

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/Makefile
File src/pkg/go/make/Makefile (right):

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/Makefile#new...
src/pkg/go/make/Makefile:17: syslist.go:
On 2011/05/25 22:22:45, rsc wrote:
> syslist.go: ../../../Make.inc Makefile

Done, but why?

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build.go
File src/pkg/go/make/build.go (right):

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:25: var arch string
On 2011/05/25 22:22:45, rsc wrote:
> Please make this a separate
> 
> // ArchChar returns the architecture character for the given goarch.
> // For example, ArchChar("amd64") returns "6".
> func ArchChar(goarch string) (string, os.Error) {
> }
> 

Done.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:37: const odir = "_obj/"
On 2011/05/25 22:22:45, rsc wrote:
> s/odir/obj/

Done.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:48: // build cgo files
On 2011/05/25 22:22:45, rsc wrote:
> This is mixing planning and execution.  Splitting them
> will make the goinstall -n mode easier to write and also
> will reduce a lot of repetition here.
> 
> Let's talk about this later.

Done as discussed.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:165: return cp(targ, filepath.Join(dir, obj))
On 2011/05/25 22:22:45, rsc wrote:
> Why not just write directly to targ?

To avoid clobbering the target if the build fails at the last step. But I guess
the targ will be the pre-install build target anyway.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build_test.go
File src/pkg/go/make/build_test.go (right):

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build_test.g...
src/pkg/go/make/build_test.go:15: {"pkg", "path"},
On 2011/05/25 22:22:45, rsc wrote:
> var buildDirs = []string{
>     "src/pkg/path"
>     "src/cmd/gofix"
>    "src/pkg/big"
>    "src/pkg/go/make/cgotest"
> }
> 
> and adjust below:
>     p := filepath.Join(runtime.GOROOT(), filepath.FromSlash(d))
> 
> I refuse to be pushed around by Windows.

Done.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build_test.g...
src/pkg/go/make/build_test.go:15: {"pkg", "path"},
On 2011/05/25 22:22:45, rsc wrote:
> var buildDirs = []string{
>     "src/pkg/path"
>     "src/cmd/gofix"
>    "src/pkg/big"
>    "src/pkg/go/make/cgotest"
> }
> 
> and adjust below:
>     p := filepath.Join(runtime.GOROOT(), filepath.FromSlash(d))
> 
> I refuse to be pushed around by Windows.

Done.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build_test.g...
src/pkg/go/make/build_test.go:29: func testBuild(t *testing.T, dir, targ string)
{
On 2011/05/25 22:22:45, rsc wrote:
> inline in the one place it is called

This actually makes it simpler now that I have another for loop.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go
File src/pkg/go/make/dir.go (right):

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:20: GoFiles  []string // .go files within dir (excluding
CgoFiles)
On 2011/05/25 22:22:45, rsc wrote:
> s/within/in/g

Done.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:33: // in the given directory. The list of files will NOT
contain the
On 2011/05/25 22:22:45, rsc wrote:
> The file lists exclude:

Done.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:36: //	- files in package main (unless allowMain is true)
On 2011/05/26 01:02:09, kevlar wrote:
> or package documentation?

Done.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:38: // 	- files starting with _ or . (temporary)
On 2011/05/25 22:22:45, rsc wrote:
> s/ (temporary)//

Done.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:39: //
On 2011/05/26 01:02:09, kevlar wrote:
> I don't know how to word it, but mention here that the _$GOOS or _$GOARCH
files
> are also skipped.  Unless we don't want to document that feature.
> 
> Maybe:
> - files ending in a different _$GOOS and/or $_GOARCH

Done.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:40: // The imports map keys are package paths imported by
listed Go files,
On 2011/05/25 22:22:45, rsc wrote:
> delete
> (not relevant here)

Done.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:54: importsm := make(map[string]bool)
On 2011/05/25 22:22:45, rsc wrote:
> s/importsm/imported/

Done.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:107: for _, decl := range pf.Decls {
On 2011/05/25 22:22:45, rsc wrote:
> for _, spec := range pf.Imports {

Done.

http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:110: unquoted, err := strconv.Unquote(quoted)
On 2011/05/25 22:22:45, rsc wrote:
> s/unquoted/path/

Done.

[bsiegert, 2011-05-26 11:29:48]

On Thu, May 26, 2011 at 00:22,  <rsc@golang.org> wrote:

>
http://codereview.appspot.com/4433047/diff/20001/src/pkg/go/make/build.go#new...
> src/pkg/go/make/build.go:25: var arch string
> Please make this a separate
>
> // ArchChar returns the architecture character for the given goarch.
> // For example, ArchChar("amd64") returns "6".
> func ArchChar(goarch string) (string, os.Error) {
> }

Why not make this a map lookup instead?

--Benny.

[rsc, 2011-05-26 13:42:38]

>> syslist.go: ../../../Make.inc Makefile
>
> Done, but why?

Because the rule has no other dependencies.
syslist.go is out of date if the Makefile changes
(because the rule might have) or if Make.inc
changes (because the list of GOARCH or GOOS
might have).

[kevlar, 2011-05-26 16:28:46]

FYI

http://codereview.appspot.com/4433047/diff/27002/src/pkg/go/make/build.go
File src/pkg/go/make/build.go (right):

http://codereview.appspot.com/4433047/diff/27002/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:54: cmds = append(cmds, c)
Is this not clearer to inline?  Even as is, the name might make more sense as
"enq" or "qcmd" or "addCmd" or something that indicates it's not actually
running the command.  You might also just have this append onto a named return
variable, since there are so few other return cases.

http://codereview.appspot.com/4433047/diff/27002/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:80: cgoCfiles[i+2] = obj + fn[:len(fn)-2] + "cgo2.c"
I'm always a little hesitant to assign to indices in a slice over which I am not
ranging... append feels safer, and setting the capacity up front negates almost
all of the performance hit.

http://codereview.appspot.com/4433047/diff/27002/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:206: func (c Cmd) String() string {
Should the Cmd type declaration be down here with its methods?

http://codereview.appspot.com/4433047/diff/27002/src/pkg/go/make/dir.go
File src/pkg/go/make/dir.go (right):

http://codereview.appspot.com/4433047/diff/27002/src/pkg/go/make/dir.go#newco...
src/pkg/go/make/dir.go:39: // Only files that satisfy the goodOSArch function
are included.
Since goodOSArch isn't exported, is this descriptive enough?  Maybe it should be
exported?

[rsc, 2011-05-26 16:39:22]

looking good

http://codereview.appspot.com/4433047/diff/27002/src/pkg/go/make/build.go
File src/pkg/go/make/build.go (right):

http://codereview.appspot.com/4433047/diff/27002/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:52: var cmds []Cmd
should be []*Cmd
i bet callers will want to have maps keyed by command

http://codereview.appspot.com/4433047/diff/27002/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:64: 
I suggest having

type build struct {
    cmds []*Cmd
    obj string
    goarch string
    ... whatever other context you need
}

func (b *build) mkdir(path string) {
    b.cmds = append(b.cmds, &Cmd{Args: []string{"mkdir", "-p", obj}})
}

func (b *build) gc(files []string) {
    b.cmds = append(b.cmds, &Cmd{
        ... whatever
    })
}

and so on

then here init the builder and 

b.mkdir(obj)

if len(d.CgoFiles) > 0 {
    outGo, outObj := b.cgo(d.Cgofiles)
    gofiles = append(gofiles, outGo...)
    ofiles = append(ofiles, outObj...)
}

if len(gofiles) > 0 {
    ofiles = append(ofiles, b.gc(gofiles))
}

if len(d.SFiles) > 0 {
    ofiles = append(ofiles, b.as(d.SFiles)...)
}

...

right now the logic and the command details
are too intertwined.

http://codereview.appspot.com/4433047/diff/27002/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:206: func (c Cmd) String() string {
c *Cmd please

[adg, 2011-05-27 06:04:04]

http://codereview.appspot.com/4433047/diff/27002/src/pkg/go/make/build.go
File src/pkg/go/make/build.go (right):

http://codereview.appspot.com/4433047/diff/27002/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:52: var cmds []Cmd
On 2011/05/26 16:39:22, rsc wrote:
> should be []*Cmd
> i bet callers will want to have maps keyed by command

Done.

http://codereview.appspot.com/4433047/diff/27002/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:54: cmds = append(cmds, c)
On 2011/05/26 16:28:46, kevlar wrote:
> Is this not clearer to inline?  Even as is, the name might make more sense as
> "enq" or "qcmd" or "addCmd" or something that indicates it's not actually
> running the command.  You might also just have this append onto a named return
> variable, since there are so few other return cases.

I renamed it to "add". Closures are useful. Let's use them.

http://codereview.appspot.com/4433047/diff/27002/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:64: 
On 2011/05/26 16:39:22, rsc wrote:
> I suggest having
> 
> type build struct {
>     cmds []*Cmd
>     obj string
>     goarch string
>     ... whatever other context you need
> }
> 
> func (b *build) mkdir(path string) {
>     b.cmds = append(b.cmds, &Cmd{Args: []string{"mkdir", "-p", obj}})
> }
> 
> func (b *build) gc(files []string) {
>     b.cmds = append(b.cmds, &Cmd{
>         ... whatever
>     })
> }
> 
> and so on
> 
> then here init the builder and 
> 
> b.mkdir(obj)
> 
> if len(d.CgoFiles) > 0 {
>     outGo, outObj := b.cgo(d.Cgofiles)
>     gofiles = append(gofiles, outGo...)
>     ofiles = append(ofiles, outObj...)
> }
> 
> if len(gofiles) > 0 {
>     ofiles = append(ofiles, b.gc(gofiles))
> }
> 
> if len(d.SFiles) > 0 {
>     ofiles = append(ofiles, b.as(d.SFiles)...)
> }
> 
> ...
> 
> right now the logic and the command details
> are too intertwined.
> 

Done. The cgo function is still a bit hairy, but not too bad.

http://codereview.appspot.com/4433047/diff/27002/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:80: cgoCfiles[i+2] = obj + fn[:len(fn)-2] + "cgo2.c"
On 2011/05/26 16:28:46, kevlar wrote:
> I'm always a little hesitant to assign to indices in a slice over which I am
not
> ranging... 

You shouldn't be. I've given the slice exactly the right length, and now I'm
filling it in. If there's an off-by-one error here it should be immediately
obvious. This is good style.

> append feels safer, and setting the capacity up front negates almost
> all of the performance hit.

It might feel safer, but it's not.

http://codereview.appspot.com/4433047/diff/27002/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:206: func (c Cmd) String() string {
On 2011/05/26 16:39:22, rsc wrote:
> c *Cmd please

Done.

[rsc, 2011-05-27 15:09:31]

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go
File src/pkg/go/make/build.go (right):

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:83: // builds will be relative to dir, so our target
should be absolute
I don't really understand this.  I know what you mean but
not why the caller would pass in a current-directory-relative
target that would be wrong.

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:97: } else {
no else after return

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:135: b.link(targ, ofiles...)
b.ld

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:137: b.pack(targ, ofiles...)
b.gopack

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:196: args := []string{cc, "-FVw", "-I" + inc, "-o",
ofile}
in general should use "-I", arg not "-I" + arg,
for any flag.  the former works right when arg == "".
the latter turns into a bug.

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:233: gofiles := make([]string, len(cgofiles)+1)
This is weird; it's the only place in the whole program 
where you work so hard on the indices and sizes.
It's premature optimization.  Running cgo
is a billion times more expensive than what this code
saves by being so complex to size the arrays.

You're writing C code.  Let Go do the work.

gofiles := []string{b.obj + "_cgo_gotypes.go"}
cfiles := []string{b.obj + "_cgo_main.c", "_cgo_export.c"}
for _, f := range cgofiles {
    f = f[:len(f)-2]
    gofiles = append(gofiles, b.obj + f + "cgo1.go")
    cfiles = append(cfiles, b.obj + f + "cgo2.c")
}

Same for output below.

[adg, 2011-05-30 00:30:09]

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go
File src/pkg/go/make/build.go (right):

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:83: // builds will be relative to dir, so our target
should be absolute
On 2011/05/27 15:09:31, rsc wrote:
> I don't really understand this.  I know what you mean but
> not why the caller would pass in a current-directory-relative
> target that would be wrong.

It's that the build commands are expected to be run from the package directory.
Perhaps Build should assume the target is relative to the package directory.

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:97: } else {
On 2011/05/27 15:09:31, rsc wrote:
> no else after return

The 'a' variable is scoped to the if block.

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:135: b.link(targ, ofiles...)
On 2011/05/27 15:09:31, rsc wrote:
> b.ld

Done.

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:137: b.pack(targ, ofiles...)
On 2011/05/27 15:09:31, rsc wrote:
> b.gopack

Done.

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:196: args := []string{cc, "-FVw", "-I" + inc, "-o",
ofile}
On 2011/05/27 15:09:31, rsc wrote:
> in general should use "-I", arg not "-I" + arg,
> for any flag.  the former works right when arg == "".
> the latter turns into a bug.

Done.

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:233: gofiles := make([]string, len(cgofiles)+1)
On 2011/05/27 15:09:31, rsc wrote:
> This is weird; it's the only place in the whole program 
> where you work so hard on the indices and sizes.
> It's premature optimization.  Running cgo
> is a billion times more expensive than what this code
> saves by being so complex to size the arrays.
> 
> You're writing C code.  Let Go do the work.
> 
> gofiles := []string{b.obj + "_cgo_gotypes.go"}
> cfiles := []string{b.obj + "_cgo_main.c", "_cgo_export.c"}
> for _, f := range cgofiles {
>     f = f[:len(f)-2]
>     gofiles = append(gofiles, b.obj + f + "cgo1.go")
>     cfiles = append(cfiles, b.obj + f + "cgo2.c")
> }
> 
> Same for output below.

Done.

[rsc, 2011-05-30 19:41:37]

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go
File src/pkg/go/make/build.go (right):

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:97: } else {
On 2011/05/30 00:30:09, adg wrote:
> On 2011/05/27 15:09:31, rsc wrote:
> > no else after return
> 
> The 'a' variable is scoped to the if block.

Unscope it.

[adg, 2011-05-30 23:36:43]

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go
File src/pkg/go/make/build.go (right):

http://codereview.appspot.com/4433047/diff/36001/src/pkg/go/make/build.go#new...
src/pkg/go/make/build.go:97: } else {
On 2011/05/30 19:41:38, rsc wrote:
> On 2011/05/30 00:30:09, adg wrote:
> > On 2011/05/27 15:09:31, rsc wrote:
> > > no else after return
> > 
> > The 'a' variable is scoped to the if block.
> 
> Unscope it.

Done.

[adg, 2011-06-02 10:51:35]

PTAL

I've added the GOPATH parsing stuff from goinstall to a new file path.go, and
tweaked the API a bit as I started adding this to goinstall in another CL:
http://codereview.appspot.com/4515180

[rsc, 2011-06-03 18:42:09]

LGTM

http://codereview.appspot.com/4433047/diff/17012/src/pkg/go/make/path.go
File src/pkg/go/make/path.go (right):

http://codereview.appspot.com/4433047/diff/17012/src/pkg/go/make/path.go#newc...
src/pkg/go/make/path.go:17: // from the GOPATH environment variable at init.
This is fine for now.  I think all these globals will need
to go into a top-level structure at some point.

http://codereview.appspot.com/4433047/diff/17012/src/pkg/go/make/path.go#newc...
src/pkg/go/make/path.go:20: // Root describes a GOPATH root.
s/Root/Tree/

This isn't about the root; it's about the tree.

// Tree describes a Go source tree, either $GOROOT or one from $GOPATH.

http://codereview.appspot.com/4433047/diff/17012/src/pkg/go/make/path.go#newc...
src/pkg/go/make/path.go:37: // SrcDir returns the directory inside this Root
// SrcDir returns the tree's package source directory.
etc

http://codereview.appspot.com/4433047/diff/17012/src/pkg/go/make/path.go#newc...
src/pkg/go/make/path.go:88: // FindRoot takes an import or filesystem path and
returns the
FindTree

[adg, 2011-06-04 02:45:07]

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

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

[adg, 2011-06-04 02:45:17]

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

go/build: new package for building go programs

R=rsc
CC=golang-dev
http://codereview.appspot.com/4433047

http://codereview.appspot.com/4433047/diff/17012/src/pkg/go/make/path.go
File src/pkg/go/make/path.go (right):

http://codereview.appspot.com/4433047/diff/17012/src/pkg/go/make/path.go#newc...
src/pkg/go/make/path.go:20: // Root describes a GOPATH root.
On 2011/06/03 18:42:09, rsc wrote:
> s/Root/Tree/
> 
> This isn't about the root; it's about the tree.
> 
> // Tree describes a Go source tree, either $GOROOT or one from $GOPATH.

Done.

http://codereview.appspot.com/4433047/diff/17012/src/pkg/go/make/path.go#newc...
src/pkg/go/make/path.go:37: // SrcDir returns the directory inside this Root
On 2011/06/03 18:42:09, rsc wrote:
> // SrcDir returns the tree's package source directory.
> etc

Done.

http://codereview.appspot.com/4433047/diff/17012/src/pkg/go/make/path.go#newc...
src/pkg/go/make/path.go:88: // FindRoot takes an import or filesystem path and
returns the
On 2011/06/03 18:42:09, rsc wrote:
> FindTree

Done.