code review 181520043: x/tools/dashboard/builder: support git; remove commit w...

x/tools/dashboard/builder: support git; remove commit watcher

This binary isn't intended to support hg anymore.
It almost might not support gccgo; I'm not sure.
But there'll be more work to do there anyway.

[adg, 2014-12-03 11:24:01]

Hello cmang@golang.org, bradfitz@golang.org (cc:
golang-codereviews@googlegroups.com),

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

[dave_cheney.net, 2014-12-03 11:43:06]

LGTM. Good riddance to that commit watcher.

https://codereview.appspot.com/181520043/diff/40001/dashboard/builder/main.go
File dashboard/builder/main.go (right):

https://codereview.appspot.com/181520043/diff/40001/dashboard/builder/main.go...
dashboard/builder/main.go:56: gcPath        = flag.String("gcpath",
"go.googlesource.com/go", "Path to download gc from")
path to clone gc from ?

[cmang, 2014-12-03 14:41:22]

https://codereview.appspot.com/181520043/diff/40001/dashboard/builder/main.go
File dashboard/builder/main.go (right):

https://codereview.appspot.com/181520043/diff/40001/dashboard/builder/main.go...
dashboard/builder/main.go:56: gcPath        = flag.String("gcpath",
"go.googlesource.com/go", "Path to download gc from")
On 2014/12/03 11:43:06, dfc wrote:
> path to clone gc from ?

If you do this, also change the description for gccPath.

https://codereview.appspot.com/181520043/diff/40001/dashboard/builder/main.go...
dashboard/builder/main.go:164: goroot, err = goroot.Clone(goroot.Path, "HEAD")
This will probably break gccgo (downloading gofrontend) until it is on github.
I'd say just pass in nothing here and if the revision is empty in repo.Clone,
call vcs.Cmd.Create instead of vcs.Cmd.CreateAtRev. Since this is only taking
into consideration the default/master branch, `hg clone {repo} {path}` and `git
clone {repo} {path}` will use the tipmost head of the default branch and the
HEAD of the master branch, respectively.

https://codereview.appspot.com/181520043/diff/40001/dashboard/builder/vcs.go
File dashboard/builder/vcs.go (right):

https://codereview.appspot.com/181520043/diff/40001/dashboard/builder/vcs.go#...
dashboard/builder/vcs.go:51: return r.Master.VCS.CreateAtRev(path, downloadPath,
rev)
wrt to the other comment,

`
if rev == "" {
        return r.Master.VCS.Create(path, downloadPath)
}
return r.Master.VCS.CreateAtRev(path, downloadPath, rev)
`

https://codereview.appspot.com/181520043/diff/40001/dashboard/builder/vcs.go#...
dashboard/builder/vcs.go:76: _, err := r.Clone(path, rev)
Each of the vcs we support has an archive/export command, so it might be worth
adding vcs.Cmd.Export/Archive and just calling that.

[bradfitz, 2014-12-03 16:39:03]

LGTM yay deletes

[adg1, 2014-12-03 23:29:19]

PTAL

https://codereview.appspot.com/181520043/diff/40001/dashboard/builder/main.go
File dashboard/builder/main.go (right):

https://codereview.appspot.com/181520043/diff/40001/dashboard/builder/main.go...
dashboard/builder/main.go:56: gcPath        = flag.String("gcpath",
"go.googlesource.com/go", "Path to download gc from")
On 2014/12/03 11:43:06, dfc wrote:
> path to clone gc from ?

Acknowledged.

https://codereview.appspot.com/181520043/diff/40001/dashboard/builder/main.go...
dashboard/builder/main.go:56: gcPath        = flag.String("gcpath",
"go.googlesource.com/go", "Path to download gc from")
On 2014/12/03 14:41:22, cmang wrote:
> On 2014/12/03 11:43:06, dfc wrote:
> > path to clone gc from ?
> 
> If you do this, also change the description for gccPath.

Acknowledged.

https://codereview.appspot.com/181520043/diff/40001/dashboard/builder/main.go...
dashboard/builder/main.go:164: goroot, err = goroot.Clone(goroot.Path, "HEAD")
On 2014/12/03 14:41:22, cmang wrote:
> This will probably break gccgo (downloading gofrontend) until it is on github.
> I'd say just pass in nothing here and if the revision is empty in repo.Clone,
> call vcs.Cmd.Create instead of vcs.Cmd.CreateAtRev. Since this is only taking
> into consideration the default/master branch, `hg clone {repo} {path}` and
`git
> clone {repo} {path}` will use the tipmost head of the default branch and the
> HEAD of the master branch, respectively.

Done.

https://codereview.appspot.com/181520043/diff/40001/dashboard/builder/vcs.go
File dashboard/builder/vcs.go (right):

https://codereview.appspot.com/181520043/diff/40001/dashboard/builder/vcs.go#...
dashboard/builder/vcs.go:51: return r.Master.VCS.CreateAtRev(path, downloadPath,
rev)
On 2014/12/03 14:41:22, cmang wrote:
> wrt to the other comment,
> 
> `
> if rev == "" {
>         return r.Master.VCS.Create(path, downloadPath)
> }
> return r.Master.VCS.CreateAtRev(path, downloadPath, rev)
> `

Done.

https://codereview.appspot.com/181520043/diff/40001/dashboard/builder/vcs.go#...
dashboard/builder/vcs.go:76: _, err := r.Clone(path, rev)
On 2014/12/03 14:41:22, cmang wrote:
> Each of the vcs we support has an archive/export command, so it might be worth
> adding vcs.Cmd.Export/Archive and just calling that.

Maybe we can do that later. I'll add a TODO

[cmang, 2014-12-03 23:41:19]

LGTM

[cmang, 2014-12-03 23:41:35]

https://codereview.appspot.com/181520043/diff/60001/dashboard/builder/main.go
File dashboard/builder/main.go (right):

https://codereview.appspot.com/181520043/diff/60001/dashboard/builder/main.go...
dashboard/builder/main.go:532: } // XXX
Is the // XXX some kind of editor artifact?

[adg, 2014-12-03 23:42:16]

Hello cmang@golang.org, bradfitz@golang.org, dave@cheney.net, adg@google.com
(cc: golang-codereviews@googlegroups.com),

Please take another look.

[adg1, 2014-12-03 23:42:32]

https://codereview.appspot.com/181520043/diff/60001/dashboard/builder/main.go
File dashboard/builder/main.go (right):

https://codereview.appspot.com/181520043/diff/60001/dashboard/builder/main.go...
dashboard/builder/main.go:532: } // XXX
On 2014/12/03 23:41:35, cmang wrote:
> Is the // XXX some kind of editor artifact?

no they're markers I put there to make sure that I remove those bits of code
before submitting

[adg, 2014-12-03 23:44:00]

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

x/tools/dashboard/builder: support git; remove commit watcher

This binary isn't intended to support hg anymore.
It almost might not support gccgo; I'm not sure.
But there'll be more work to do there anyway.

LGTM=dave, bradfitz, cmang
R=cmang, bradfitz, dave, adg
CC=golang-codereviews
https://codereview.appspot.com/181520043