code review 4515180: goinstall: use go/make package to scan and build packages

goinstall: use go/make package to scan and build packages

[adg, 2011-06-02 10:52:07]

Hello golang-dev@googlegroups.com,

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

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

Not ready for an in-depth review; more of a companion piece to the
main action going on in http://codereview.appspot.com/4433047

[rsc, 2011-06-03 18:43:04]

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

http://codereview.appspot.com/4515180/diff/7001/src/cmd/goinstall/main.go#new...
src/cmd/goinstall/main.go:12: gomake "go/make"
Ugh.  I missed that the package was called make.
What about go/build ?

[niemeyer, 2011-06-03 19:02:08]

> Ugh.  I missed that the package was called make.
> What about go/build ?

If the intention is to have this being the merger of goinstall and make, can we
call it go<something> so that it can fit in /usr/bin properly?

[rsc, 2011-06-03 19:04:23]

> If the intention is to have this being the merger of goinstall and make,
> can we call it go<something> so that it can fit in /usr/bin properly?

That's not the intention.  There will be a separate gomake command.
This is a core package that it will use.

Russ

[niemeyer, 2011-06-03 19:10:36]

> That's not the intention.  There will be a separate gomake command.
> This is a core package that it will use.

Ah, I see the big picture now, thanks.  I've missed the review of go/make.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/blog
http://niemeyer.net/twitter

[adg, 2011-06-07 00:24:14]

This is ready for review, now.

On 2011/06/03 18:43:04, rsc wrote:
> src/cmd/goinstall/main.go:12: gomake "go/make"
> Ugh.  I missed that the package was called make.
> What about go/build ?

Did that.

[niemeyer, 2011-06-07 01:11:48]

LGTM.  Only some minor observations:

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

http://codereview.appspot.com/4515180/diff/13001/src/cmd/goinstall/main.go#ne...
src/cmd/goinstall/main.go:165: visit[pkg] = done
Nice.

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

http://codereview.appspot.com/4515180/diff/13001/src/cmd/goinstall/make.go#ne...
src/cmd/goinstall/make.go:15: func domake(dir, pkg string, di *build.DirInfo,
tree *build.Tree) os.Error {
Just for clarity, I'd likely declare it as:

(pkg, dir string, dirInfo *build.DirInfo, tree *build.Tree)

http://codereview.appspot.com/4515180/diff/13001/src/cmd/goinstall/make.go#ne...
src/cmd/goinstall/make.go:36: if err = c.Clean(dir); err != nil {
I'm not entirely sure this makes sense.  Cleaning on make is important because
it's dependency-based and will figure what actions it has to perform for
bringing the tree up-to-date.  Given the logic here, doesn't it mean we're
overwriting the files anyway at all times, and thus cleaning before a build is a
NOOP?

[adg, 2011-06-07 01:18:03]

On 7 June 2011 11:11,  <n13m3y3r@gmail.com> wrote:
>
http://codereview.appspot.com/4515180/diff/13001/src/cmd/goinstall/make.go#ne...
> src/cmd/goinstall/make.go:36: if err = c.Clean(dir); err != nil {
> I'm not entirely sure this makes sense.  Cleaning on make is important
> because it's dependency-based and will figure what actions it has to
> perform for bringing the tree up-to-date.  Given the logic here, doesn't
> it mean we're overwriting the files anyway at all times, and thus
> cleaning before a build is a NOOP?

Yes. What's missing from this CL is incremental building, and I don't
think this CL should go in until that's ready. (My fault for
forgetting.) With that working, -clean would become meaningful again.

The strategy I intend to employ is to rebuild if input file mtime is >
output file mtime. Should it be any cleverer than that?

Andrew

[niemeyer, 2011-06-07 01:30:09]

> Yes. What's missing from this CL is incremental building, and I don't
> think this CL should go in until that's ready. (My fault for
> forgetting.) With that working, -clean would become meaningful again.

I see.

> The strategy I intend to employ is to rebuild if input file mtime is >
> output file mtime. Should it be any cleverer than that?

That's what make does, so it should be a safe first step. It may
get a bit fancier in the future, and check the mtime of the direct
package dependencies too.  This would likely avoid most of the
current uses of -clean too.

[kevlar, 2011-06-07 01:37:40]

>
> That's what make does, so it should be a safe first step. It may
> get a bit fancier in the future, and check the mtime of the direct
> package dependencies too.  This would likely avoid most of the
> current uses of -clean too.


As long as you're making these decisions on the fly (e.g. you try to make a
target before you examine its mtime), it works out.  The only other thing
that I would think to do initially (and you may have already thought of
this) is including the standard libraries as dependencies whose mtimes
should be examined.  It's always nice when the builder notices that you
recompiled Go or made a stdlib edit without you having to tell it.

[adg, 2011-06-07 06:33:48]

On 7 June 2011 11:30,  <n13m3y3r@gmail.com> wrote:
>> The strategy I intend to employ is to rebuild if input file mtime is >
>> output file mtime. Should it be any cleverer than that?
>
> That's what make does, so it should be a safe first step.

Done. PTAL

> It may
> get a bit fancier in the future, and check the mtime of the direct
> package dependencies too.  This would likely avoid most of the
> current uses of -clean too.

That's the idea. It should also check package object versions and
re-build when deps are out-of-date.

Andrew

[niemeyer, 2011-06-07 13:34:14]

Still looks good.  Some additional input:

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

http://codereview.appspot.com/4515180/diff/22001/src/cmd/goinstall/main.go#ne...
src/cmd/goinstall/main.go:41: noInstall         = flag.Bool("noinstall", false,
"don't build and install")
Should this be ("install", true, ...)?  That's what we've been using in other
cases.

http://codereview.appspot.com/4515180/diff/22001/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (right):

http://codereview.appspot.com/4515180/diff/22001/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:108: fn := file
The extra fn variable doesn't look necessary.

http://codereview.appspot.com/4515180/diff/22001/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:122: fi, err := os.Stat(filepath.Join(dir, file))
Should this check for abs too?  Not sure about what's your idea for dealing with
$GOROOT based package files (will we ever have those under Input?).

http://codereview.appspot.com/4515180/diff/22001/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:134: fn := file
Also doesn't look necessary.

[adg, 2011-06-08 00:49:54]

http://codereview.appspot.com/4515180/diff/22001/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (right):

http://codereview.appspot.com/4515180/diff/22001/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:108: fn := file
On 2011/06/07 13:34:14, niemeyer wrote:
> The extra fn variable doesn't look necessary.

Done.

http://codereview.appspot.com/4515180/diff/22001/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:122: fi, err := os.Stat(filepath.Join(dir, file))
On 2011/06/07 13:34:14, niemeyer wrote:
> Should this check for abs too?  Not sure about what's your idea for dealing
with
> $GOROOT based package files (will we ever have those under Input?).

Done.

http://codereview.appspot.com/4515180/diff/22001/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:134: fn := file
On 2011/06/07 13:34:14, niemeyer wrote:
> Also doesn't look necessary.

Done.

[rsc, 2011-06-08 02:59:28]

This is pretty close but not quite there yet.
It's possible there should be a struct returned
instead of []*Cmd that has the Cmd []*Cmd as
a field.  Then it could list the inputs, outputs,
and intermediate files/directories of the overall
set of commands.

Clean would be
for i := len(d.Intermediate)-1; i >= 0; i-- {
    os.Remove(d.Intermediate[i])
}

Nuke would be
for i := len(d.Output)-1; i >= 0; i-- {
    os.Remove(d.Output[i])
}
Clean()

The build for an entire directory could be skipped
if all the d.Output were newer than all the d.Input.

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

http://codereview.appspot.com/4515180/diff/17002/src/cmd/goinstall/main.go#ne...
src/cmd/goinstall/main.go:41: noInstall         = flag.Bool("noinstall", false,
"don't build and install")
double negation.

install = flag.Bool("install", true, "install after downloading")

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

http://codereview.appspot.com/4515180/diff/17002/src/cmd/goinstall/make.go#ne...
src/cmd/goinstall/make.go:16: targ := filepath.Join(tree.PkgDir(), pkg+".a")
Seems like this logic belongs in go/build.

http://codereview.appspot.com/4515180/diff/17002/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (right):

http://codereview.appspot.com/4515180/diff/17002/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:108: fi, err := os.Stat(absPath(dir, file))
This shouldn't be the caller's responsibility.
The returned command should have absolute paths here already.
I let it go during the initial review but it's a must 
once you start doing multiple directories.  They should
be clean too, so you have some chance of identifying
files that are outputs in one place and inputs in another.

http://codereview.appspot.com/4515180/diff/17002/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:128: func (c *Cmd) Clean(dir string) (err os.Error) {
This is more of a Nuke.  Clean doesn't remove the final target, just the
intermediate ones.

http://codereview.appspot.com/4515180/diff/17002/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:130: if e := os.RemoveAll(absPath(dir, file)); err ==
nil {
RemoveAll is kind of scary here.  Should really just be Remove, in reverse
dependency order.

[adg, 2011-06-09 04:33:40]

PTAL

[rsc, 2011-06-14 17:31:55]

LGTM

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

http://codereview.appspot.com/4515180/diff/35001/src/cmd/goinstall/main.go#ne...
src/cmd/goinstall/main.go:211: if !errors {
All this nesting was to make sur we got to visit[pkg] = done.
Since you took care of that with a defer, it can all goa way.

if errors {
    return
}

http://codereview.appspot.com/4515180/diff/35001/src/cmd/goinstall/main.go#ne...
src/cmd/goinstall/main.go:213: errorf("%s: install: %v\n", pkg, err)
and return

http://codereview.appspot.com/4515180/diff/35001/src/cmd/goinstall/main.go#ne...
src/cmd/goinstall/main.go:226: errorf("%s: install: %v\n", pkg, err)
and return

http://codereview.appspot.com/4515180/diff/35001/src/cmd/goinstall/main.go#ne...
src/cmd/goinstall/main.go:233: if remote && !errors {
if remote {

http://codereview.appspot.com/4515180/diff/35001/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (right):

http://codereview.appspot.com/4515180/diff/35001/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:39: addI := func(file ...string) {
Make this a method like the others.

http://codereview.appspot.com/4515180/diff/35001/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:53: s.Input = append(s.Input, cgoFiles...)
Shouldn't this be a method too?

http://codereview.appspot.com/4515180/diff/35001/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:145: func (s *Script) Clean() (err os.Error) {
doc comment

http://codereview.appspot.com/4515180/diff/35001/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:146: for i := len(s.Intermediate) - 1; i >= 0; i-- {
// Reverse order so that directories get removed after the files they contain.

http://codereview.appspot.com/4515180/diff/35001/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:154: func (s *Script) Nuke() (err os.Error) {
doc comment

http://codereview.appspot.com/4515180/diff/35001/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:155: for i := len(s.Output) - 1; i >= 0; i-- {
// Reverse order so that directories get removed after the files they contain.

[adg, 2011-06-15 03:28:47]

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

goinstall: use go/make package to scan and build packages

R=rsc, n13m3y3r, kevlar
CC=golang-dev
http://codereview.appspot.com/4515180

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

http://codereview.appspot.com/4515180/diff/35001/src/cmd/goinstall/main.go#ne...
src/cmd/goinstall/main.go:211: if !errors {
On 2011/06/14 17:31:55, rsc wrote:
> All this nesting was to make sur we got to visit[pkg] = done.
> Since you took care of that with a defer, it can all goa way.
> 
> if errors {
>     return
> }

Done.

http://codereview.appspot.com/4515180/diff/35001/src/cmd/goinstall/main.go#ne...
src/cmd/goinstall/main.go:213: errorf("%s: install: %v\n", pkg, err)
On 2011/06/14 17:31:55, rsc wrote:
> and return

Done.

http://codereview.appspot.com/4515180/diff/35001/src/cmd/goinstall/main.go#ne...
src/cmd/goinstall/main.go:226: errorf("%s: install: %v\n", pkg, err)
On 2011/06/14 17:31:55, rsc wrote:
> and return

Done.

http://codereview.appspot.com/4515180/diff/35001/src/cmd/goinstall/main.go#ne...
src/cmd/goinstall/main.go:233: if remote && !errors {
On 2011/06/14 17:31:55, rsc wrote:
> if remote {

Done.

http://codereview.appspot.com/4515180/diff/35001/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (right):

http://codereview.appspot.com/4515180/diff/35001/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:39: addI := func(file ...string) {
On 2011/06/14 17:31:55, rsc wrote:
> Make this a method like the others.

Done.

http://codereview.appspot.com/4515180/diff/35001/src/pkg/go/build/build.go#ne...
src/pkg/go/build/build.go:53: s.Input = append(s.Input, cgoFiles...)
On 2011/06/14 17:31:55, rsc wrote:
> Shouldn't this be a method too?

It only happens once. If I do it again at some later stage, I'll add a method.
Same for Output.

[adg, 2011-06-15 04:30:04]

On 15 June 2011 13:28,  <adg@golang.org> wrote:
> It only happens once. If I do it again at some later stage, I'll add a
> method. Same for Output.

This comment is out of date. I made it a method.