code review 13240044: go.tools/dashboard: add option to build gccgo revisions

go.tools/dashboard: add option to build gccgo revisions

* Added --tool flag that allows user to specify "gccgo" as the tool to build.
* Added builderEnv struct to abstract away setting up the build.
* Made envv and envvWindows methods of the builderEnv struct.
* Modified Builder.envv() to wrap envv and envvWindows
* Added internal builderEnv in Builder.

[cmang, 2013-08-28 18:11:53]

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

I'd like you to review this change to
https://code.google.com/p/go.tools

[adg, 2013-08-30 00:37:40]

https://codereview.appspot.com/13240044/diff/7001/dashboard/builder/env.go
File dashboard/builder/env.go (right):

https://codereview.appspot.com/13240044/diff/7001/dashboard/builder/env.go#ne...
dashboard/builder/env.go:38: type builderEnv struct {
make this an interface instead of a concrete struct
then goEnv, gccgoEnv can be an implementation of that interface

[cmang, 2013-08-30 01:26:54]

https://codereview.appspot.com/13240044/diff/7001/dashboard/builder/env.go
File dashboard/builder/env.go (right):

https://codereview.appspot.com/13240044/diff/7001/dashboard/builder/env.go#ne...
dashboard/builder/env.go:38: type builderEnv struct {
On 2013/08/30 00:37:40, adg wrote:
> make this an interface instead of a concrete struct
> then goEnv, gccgoEnv can be an implementation of that interface

Done.

[adg, 2013-09-02 01:16:07]

https://codereview.appspot.com/13240044/diff/14001/dashboard/builder/env.go
File dashboard/builder/env.go (right):

https://codereview.appspot.com/13240044/diff/14001/dashboard/builder/env.go#n...
dashboard/builder/env.go:19: const gccUrl = "svn://gcc.gnu.org/svn/gcc/trunk"
might be worth making this a flag, in case someone has a local copy of gcc

https://codereview.appspot.com/13240044/diff/14001/dashboard/builder/env.go#n...
dashboard/builder/env.go:47: envvWindows() []string
don't make this part of the interface; just put the check in the respective
envv() methods

https://codereview.appspot.com/13240044/diff/14001/dashboard/builder/env.go#n...
dashboard/builder/env.go:125: func (env *gccgoEnv) envv() []string { return
[]string{} }
there's no build environment at all?

https://codereview.appspot.com/13240044/diff/14001/dashboard/builder/env.go#n...
dashboard/builder/env.go:130: gofrontendpath := filepath.Join(workpath,
"gofrontend")
add a comment with a general description of what this function achieves

https://codereview.appspot.com/13240044/diff/14001/dashboard/builder/env.go#n...
dashboard/builder/env.go:179: if _, err := runOutput(*buildTimeout, nil,
ioutil.Discard, gccobjdir, "make"); err != nil {
this is really part of the setup? does it just happen once?
if not, should we be building gcc only once for multiple gofrontend builds? is
that possible?

https://codereview.appspot.com/13240044/diff/14001/dashboard/builder/env.go#n...
dashboard/builder/env.go:191: return os.Mkdir(strings.Replace(path, src, dst,
1), mkdirPerm)
declare the destination path once at the top of this function

https://codereview.appspot.com/13240044/diff/14001/dashboard/builder/env.go#n...
dashboard/builder/env.go:204: defer dstFile.Close()
you should check the error return value when closing a writer

[cmang, 2013-09-09 18:34:50]

https://codereview.appspot.com/13240044/diff/14001/dashboard/builder/env.go
File dashboard/builder/env.go (right):

https://codereview.appspot.com/13240044/diff/14001/dashboard/builder/env.go#n...
dashboard/builder/env.go:19: const gccUrl = "svn://gcc.gnu.org/svn/gcc/trunk"
On 2013/09/02 01:16:07, adg wrote:
> might be worth making this a flag, in case someone has a local copy of gcc

Done.

https://codereview.appspot.com/13240044/diff/14001/dashboard/builder/env.go#n...
dashboard/builder/env.go:47: envvWindows() []string
On 2013/09/02 01:16:07, adg wrote:
> don't make this part of the interface; just put the check in the respective
> envv() methods

Done.

https://codereview.appspot.com/13240044/diff/14001/dashboard/builder/env.go#n...
dashboard/builder/env.go:125: func (env *gccgoEnv) envv() []string { return
[]string{} }
On 2013/09/02 01:16:07, adg wrote:
> there's no build environment at all?

I'm not really sure the best way to test this. Beyond what things are imported
from extraEnv, like the PATH or CC, there's not much extra environment needed.
We might need ld.gold to be included in the path.

https://codereview.appspot.com/13240044/diff/14001/dashboard/builder/env.go#n...
dashboard/builder/env.go:130: gofrontendpath := filepath.Join(workpath,
"gofrontend")
On 2013/09/02 01:16:07, adg wrote:
> add a comment with a general description of what this function achieves

Done.

https://codereview.appspot.com/13240044/diff/14001/dashboard/builder/env.go#n...
dashboard/builder/env.go:179: if _, err := runOutput(*buildTimeout, nil,
ioutil.Discard, gccobjdir, "make"); err != nil {
On 2013/09/02 01:16:07, adg wrote:
> this is really part of the setup? does it just happen once?
> if not, should we be building gcc only once for multiple gofrontend builds? is
> that possible?

I'm not so sure this is possible because the configuration step might generate
different files depending on the system architecture, so building gcc will have
to happen on each system. We might be able to get away with configuring gcc only
once per machine and only rebuilding the updated files since the last build.

https://codereview.appspot.com/13240044/diff/14001/dashboard/builder/env.go#n...
dashboard/builder/env.go:191: return os.Mkdir(strings.Replace(path, src, dst,
1), mkdirPerm)
On 2013/09/02 01:16:07, adg wrote:
> declare the destination path once at the top of this function

Done.

https://codereview.appspot.com/13240044/diff/14001/dashboard/builder/env.go#n...
dashboard/builder/env.go:204: defer dstFile.Close()
On 2013/09/02 01:16:07, adg wrote:
> you should check the error return value when closing a writer

Done.

[adg, 2013-09-11 23:43:22]

LGTM

[cmang, 2013-09-11 23:45:27]

*** Submitted as
https://code.google.com/p/go/source/detail?r=e6067e8f6d11&repo=tools ***

go.tools/dashboard: add option to build gccgo revisions

* Added --tool flag that allows user to specify "gccgo" as the tool to build.
* Added builderEnv struct to abstract away setting up the build.
* Made envv and envvWindows methods of the builderEnv struct.
* Modified Builder.envv() to wrap envv and envvWindows
* Added internal builderEnv in Builder.

R=adg
CC=golang-dev
https://codereview.appspot.com/13240044