code review 4172056: gobuilder: add -package flag to build external packages

gobuilder: add -package flag to build external packages

Also add -v for verbose logging.

[adg, 2011-02-18 23:18:57]

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

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

[adg, 2011-02-23 03:20:31]

PTAL

I now pull the docs here without using goinstall/godoc. It was just as easy as
postulated. ;-)

[gri, 2011-02-23 04:35:23]

FYI.
The doc extraction looks good.
The rest of the code looks fine to me, but perhaps somebody who has looked at
this code before should have a look as well.

http://codereview.appspot.com/4172056/diff/3001/misc/dashboard/builder/exec.go
File misc/dashboard/builder/exec.go (right):

http://codereview.appspot.com/4172056/diff/3001/misc/dashboard/builder/exec.g...
misc/dashboard/builder/exec.go:33: log.Println("run", argv)
runLog ?

[r, 2011-02-23 04:42:37]

http://codereview.appspot.com/4172056/diff/3001/misc/dashboard/builder/doc.go
File misc/dashboard/builder/doc.go (right):

http://codereview.appspot.com/4172056/diff/3001/misc/dashboard/builder/doc.go...
misc/dashboard/builder/doc.go:45: -package: Third-party package builder mode
rev, cmd, package? how about rev, cmd, pkg or else revision, command, package?

http://codereview.appspot.com/4172056/diff/3001/misc/dashboard/builder/hg.go
File misc/dashboard/builder/hg.go (right):

http://codereview.appspot.com/4172056/diff/3001/misc/dashboard/builder/hg.go#...
misc/dashboard/builder/hg.go:62: var revisionRe =
regexp.MustCompile("([0-9]+):[0-9a-f]+$")
use `` for regexps, just on principle (unless you need \t or something)

http://codereview.appspot.com/4172056/diff/3001/misc/dashboard/builder/main.go
File misc/dashboard/builder/main.go (right):

http://codereview.appspot.com/4172056/diff/3001/misc/dashboard/builder/main.g...
misc/dashboard/builder/main.go:47: packageMode   = flag.Bool("package", false,
"Build third-party packages")
the name of this flag doesn't seem the same style as the others.

http://codereview.appspot.com/4172056/diff/3001/misc/dashboard/builder/main.g...
misc/dashboard/builder/main.go:104: if *packageMode {
these big hunky if blocks cry out for separate functions with nice comments

[rsc, 2011-02-23 17:07:37]

http://codereview.appspot.com/4172056/diff/3001/misc/dashboard/builder/http.go
File misc/dashboard/builder/http.go (right):

http://codereview.appspot.com/4172056/diff/3001/misc/dashboard/builder/http.g...
misc/dashboard/builder/http.go:12: "strconv"
|sort

http://codereview.appspot.com/4172056/diff/3001/misc/dashboard/builder/main.go
File misc/dashboard/builder/main.go (right):

http://codereview.appspot.com/4172056/diff/3001/misc/dashboard/builder/main.g...
misc/dashboard/builder/main.go:47: packageMode   = flag.Bool("package", false,
"Build third-party packages")
package is a strange name for this.
i expected it to take an argument,
among other things.

-thirdparty

might be more accurate.

http://codereview.appspot.com/4172056/diff/3001/misc/dashboard/builder/packag...
File misc/dashboard/builder/package.go (right):

http://codereview.appspot.com/4172056/diff/3001/misc/dashboard/builder/packag...
misc/dashboard/builder/package.go:1: package main
copyright notice

[adg, 2011-02-23 22:15:04]

PTAL

On 2011/02/23 04:42:37, r wrote:
> http://codereview.appspot.com/4172056/diff/3001/misc/dashboard/builder/doc.go
> File misc/dashboard/builder/doc.go (right):
> 
>
http://codereview.appspot.com/4172056/diff/3001/misc/dashboard/builder/doc.go...
> misc/dashboard/builder/doc.go:45: -package: Third-party package builder mode
> rev, cmd, package? how about rev, cmd, pkg or else revision, command, package?

It's now -thirdparty. Seems reasonable to be different to the other arguments,
as it changes the entire behavior of the program. What do you think?

[r2, 2011-02-24 00:21:10]

http://codereview.appspot.com/4172056/diff/13001/misc/dashboard/builder/doc.go
File misc/dashboard/builder/doc.go (right):

http://codereview.appspot.com/4172056/diff/13001/misc/dashboard/builder/doc.g...
misc/dashboard/builder/doc.go:48: state to dashboard, issue releases, or run
benchmarks)
sorry but i dislike the term "third party".  there can only be one third party. 
and it's a stupid legal term that spilled over into code. i'd like to keep it
out of the go tree.

looks to me like 'build' might be the right name

build
bench
release

[rsc, 2011-02-24 03:45:54]

> looks to me like 'build' might be the right name

The program is called gobuilder.  There's no obvious
difference between gobuilder and gobuilder -build.

The flag in question says "instead of building the
Go distribution, build all the packages listed at
http://godashboard.appspot.com/package".
I don't have any good names.

-nonstandard
-external
-the.packages.not.in.the.standard.distribution

Maybe -goinstall, for "build all known goinstall-able packages"?

Russ

[r2, 2011-02-24 04:09:00]

On Feb 23, 2011, at 7:44 PM, Russ Cox wrote:

>> looks to me like 'build' might be the right name
> 
> The program is called gobuilder.  There's no obvious
> difference between gobuilder and gobuilder -build.
> 
> The flag in question says "instead of building the
> Go distribution, build all the packages listed at
> http://godashboard.appspot.com/package".
> I don't have any good names.
> 
> -nonstandard
> -external
> -the.packages.not.in.the.standard.distribution
> 
> Maybe -goinstall, for "build all known goinstall-able packages"?

"external" was my first choice before "build"

but anything reasonable other than some form of "third party" is fine.

-rob

[adg, 2011-02-24 04:16:11]

http://codereview.appspot.com/4172056/diff/13001/misc/dashboard/builder/doc.go
File misc/dashboard/builder/doc.go (right):

http://codereview.appspot.com/4172056/diff/13001/misc/dashboard/builder/doc.g...
misc/dashboard/builder/doc.go:48: state to dashboard, issue releases, or run
benchmarks)
On 2011/02/24 00:21:10, r2 wrote:
> sorry but i dislike the term "third party". 

-external it is. PTAL.

[adg, 2011-03-01 00:37:24]

Ping!

[rsc, 2011-03-02 20:04:23]

LGTM

http://codereview.appspot.com/4172056/diff/15002/misc/dashboard/builder/main.go
File misc/dashboard/builder/main.go (right):

http://codereview.appspot.com/4172056/diff/15002/misc/dashboard/builder/main....
misc/dashboard/builder/main.go:390: // envv returns an environment for
build/bench execution
while you are here, a request:

top-level

// These variables are copied from the gobuilder's environment.
var extraEnv = []string{
    "DISABLE_NET_TESTS",
    "GOARM",
}

and then in envv if any of these are set, add to the returned result.

[adg, 2011-03-02 23:21:58]

Good suggestion, thanks.

http://codereview.appspot.com/4172056/diff/15002/misc/dashboard/builder/main.go
File misc/dashboard/builder/main.go (right):

http://codereview.appspot.com/4172056/diff/15002/misc/dashboard/builder/main....
misc/dashboard/builder/main.go:390: // envv returns an environment for
build/bench execution
On 2011/03/02 20:04:23, rsc wrote:
> while you are here, a request:
> 
> top-level
> 
> // These variables are copied from the gobuilder's environment.
> var extraEnv = []string{
>     "DISABLE_NET_TESTS",
>     "GOARM",
> }
> 
> and then in envv if any of these are set, add to the returned result.

Done.

[adg, 2011-03-03 03:41:13]

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

gobuilder: add -package flag to build external packages

Also add -v for verbose logging.

R=rsc, gri, r, r2
CC=golang-dev
http://codereview.appspot.com/4172056