code review 5483069: go: implement build, install

go: implement build, install, run

clean is gone; all the intermediate files are created
in a temporary tree that is wiped when the command ends.

Not using go/build's Script because it is not well aligned
with this API.  The various builder methods are copied from
go/build and adapted.  Probably once we delete goinstall
we can delete the Script API too.

[rsc, 2011-12-14 05:25:17]

Hello golang-dev@googlegroups.com,

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

[rog, 2011-12-14 10:41:28]

looks good. any plans for some tests?

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go
File src/cmd/go/build.go (right):

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode231
src/cmd/go/build.go:231: obj := filepath.Join(b.work,
filepath.FromSlash(a.p.ImportPath+"/_obj")) + string(filepath.Separator)
filepath.Join(b.work, filepath.FromSlash(a.p.ImportPath), objDir) +
string(filepath.Separator) ?

it's a pity Join does a Clean.

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode374
src/cmd/go/build.go:374: err := cmd.Run()
cmd.CombinedOutput?

[rsc, 2011-12-14 16:08:12]

R=adg?

[adg1, 2011-12-14 20:59:15]

On it.

On Thursday, December 15, 2011, Russ Cox <rsc@golang.org> wrote:
> R=adg?
>

[adg, 2011-12-14 22:00:39]

http://codereview.appspot.com/5483069/diff/5002/src/cmd/cgo/main.go
File src/cmd/cgo/main.go (right):

http://codereview.appspot.com/5483069/diff/5002/src/cmd/cgo/main.go#newcode138
src/cmd/cgo/main.go:138: flag.StringVar(&objDir, "objdir", "", "object
directory")
It would be nice if this were a flag.String like the other ones.

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go
File src/cmd/go/build.go (right):

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode109
src/cmd/go/build.go:109: work  string
comment?

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode144
src/cmd/go/build.go:144: if !needInstall(p) && (i > len(pkgs) || !Aflag) {
please comment on the significance of i > len(pkgs)

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode149
src/cmd/go/build.go:149: a.f = (*builder).install
I didn't now you could do this!

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode195
src/cmd/go/build.go:195: if fi.ModTime().After(t) {
nice

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode196
src/cmd/go/build.go:196: fmt.Printf("%s needs install: %s > %s\n", p.ImportPath,
src, p.targ)
seems gratuitously noisy? a debug print?

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode245
src/cmd/go/build.go:245: println(a.p.info)
debug print

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode273
src/cmd/go/build.go:273: if err := b.gc(a.p.Dir, obj+out, a.p.ImportPath, inc,
gofiles); err != nil {
I'm confused. go/build already does all this, and it's tested. why are we doing
it again here?

[adg, 2011-12-14 22:18:31]

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go
File src/cmd/go/build.go (right):

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode291
src/cmd/go/build.go:291: if err := b.gopack(obj, a.targ, objects); err != nil {
return b.gopack(...

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode361
src/cmd/go/build.go:361: if b.nflag || b.vflag {
you could put this test in showcmd

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode423
src/cmd/go/build.go:423: // TODO: None of these are right.
what are 'these'?

[rsc, 2011-12-14 22:47:07]

PTAL

http://codereview.appspot.com/5483069/diff/5002/src/cmd/cgo/main.go
File src/cmd/cgo/main.go (right):

http://codereview.appspot.com/5483069/diff/5002/src/cmd/cgo/main.go#newcode138
src/cmd/cgo/main.go:138: flag.StringVar(&objDir, "objdir", "", "object
directory")
On 2011/12/14 22:00:39, adg wrote:
> It would be nice if this were a flag.String like the other ones.

Done.

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go
File src/cmd/go/build.go (right):

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode109
src/cmd/go/build.go:109: work  string
On 2011/12/14 22:00:39, adg wrote:
> comment?

Done.

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode144
src/cmd/go/build.go:144: if !needInstall(p) && (i > len(pkgs) || !Aflag) {
On 2011/12/14 22:00:39, adg wrote:
> please comment on the significance of i > len(pkgs)

Gone.

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode196
src/cmd/go/build.go:196: fmt.Printf("%s needs install: %s > %s\n", p.ImportPath,
src, p.targ)
On 2011/12/14 22:00:39, adg wrote:
> seems gratuitously noisy? a debug print?

Done.

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode245
src/cmd/go/build.go:245: println(a.p.info)
On 2011/12/14 22:00:39, adg wrote:
> debug print

Done.

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode273
src/cmd/go/build.go:273: if err := b.gc(a.p.Dir, obj+out, a.p.ImportPath, inc,
gofiles); err != nil {
On 2011/12/14 22:00:39, adg wrote:
> I'm confused. go/build already does all this, and it's tested. why are we
doing
> it again here?

Updated CL description (last one got lost).

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode291
src/cmd/go/build.go:291: if err := b.gopack(obj, a.targ, objects); err != nil {
On 2011/12/14 22:18:31, adg wrote:
> return b.gopack(...

no longer applicable

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode361
src/cmd/go/build.go:361: if b.nflag || b.vflag {
On 2011/12/14 22:18:31, adg wrote:
> you could put this test in showcmd

i'd rather have it here.

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode374
src/cmd/go/build.go:374: err := cmd.Run()
On 2011/12/14 10:41:28, rog wrote:
> cmd.CombinedOutput?

i like having control over the pieces.
sugar is only good for so long.

http://codereview.appspot.com/5483069/diff/5002/src/cmd/go/build.go#newcode423
src/cmd/go/build.go:423: // TODO: None of these are right.
On 2011/12/14 22:18:31, adg wrote:
> what are 'these'?

Done.

[adg, 2011-12-14 23:04:33]

http://codereview.appspot.com/5483069/diff/2007/src/cmd/go/build.go
File src/cmd/go/build.go (right):

http://codereview.appspot.com/5483069/diff/2007/src/cmd/go/build.go#newcode56
src/cmd/go/build.go:56: if len(args) > 0 && strings.HasSuffix(args[0], ".go") {
some external package paths end in .go, this will make them uninstallable

http://codereview.appspot.com/5483069/diff/2007/src/cmd/go/build.go#newcode153
src/cmd/go/build.go:153: b.gobin = os.Getenv("GOBIN")
we shouldn't be using gobin unless the source tree is GOROOT. go/build does this
for us.

[rsc, 2011-12-15 02:55:51]

On Wed, Dec 14, 2011 at 18:04,  <adg@golang.org> wrote:
> some external package paths end in .go, this will make them
> uninstallable

It makes them unbuildable with 'go build', but go build
only builds; it does not install.  The main command
that will be invoked with an external package is 'go install',
which does not have this special case.  I think this is fine,
especially since the case you raise is rare.
Also, the case you raise is already against existing
conventions since the package name cannot possibly
end in .go, so the directory and package names disagree.
And .go is a file suffix, not a directory suffix.  We don't
have to be bullied around by people picking bad names.

> we shouldn't be using gobin unless the source tree is GOROOT. go/build
> does this for us.

weird.  will fix.

[rsc, 2011-12-15 03:10:05]

PTAL

[adg, 2011-12-15 03:16:20]

LGTM

[adg, 2011-12-15 03:16:35]

On 2011/12/15 02:55:51, rsc wrote:
> On Wed, Dec 14, 2011 at 18:04,  <mailto:adg@golang.org> wrote:
> > some external package paths end in .go, this will make them
> > uninstallable
> 
> It makes them unbuildable with 'go build', but go build
> only builds; it does not install.  The main command
> that will be invoked with an external package is 'go install',
> which does not have this special case.  I think this is fine,
> especially since the case you raise is rare.
> Also, the case you raise is already against existing
> conventions since the package name cannot possibly
> end in .go, so the directory and package names disagree.
> And .go is a file suffix, not a directory suffix.  We don't
> have to be bullied around by people picking bad names.

Fine.

[rsc, 2011-12-15 03:42:52]

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

go: implement build, install, run

clean is gone; all the intermediate files are created
in a temporary tree that is wiped when the command ends.

Not using go/build's Script because it is not well aligned
with this API.  The various builder methods are copied from
go/build and adapted.  Probably once we delete goinstall
we can delete the Script API too.

R=rogpeppe, adg, adg
CC=golang-dev
http://codereview.appspot.com/5483069