code review 103800044: dashboard: support building patches

dashboard: support building patches

[cmang, 2014-06-03 01:54:42]

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

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

[adg, 2014-06-03 04:51:57]

FYI: I'll be able to review this late next week at the earliest.

[rsc, 2014-08-24 02:54:36]

Andrew, can we get this in please?

[adg, 2014-08-25 02:11:32]

I'm not crazy about overloading the meaning of Commit.Hash
Can we just add a new field which, if set, means this is a trypatch?
(Not saying we must, just asking why not)

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/build.go
File dashboard/app/build/build.go (right):

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/build...
dashboard/app/build/build.go:104: PatchAttempt      bool
this is less clear than the previous definition.
what does it mean? that we should attempt a patch? or that one has been
attempted?

plus i'm not sure we can rename this now, as we have entities in the datastore
with this field set. (any entity in the datastore since this field was added, at
least)

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/build...
dashboard/app/build/build.go:423: // RemovePatch removes p from the set of
Patches to be built.
this needs to be run inside a transaction

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/build...
dashboard/app/build/build.go:434: if tp.CL == p.CL && tp.Diff == p.Diff {
if tp == p {
??
surely commithash is relevant too?

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/build...
dashboard/app/build/build.go:444: todo.Patches[pos] =
todo.Patches[len(todo.Patches)-1]
Clearer, and in one line:
todo.Patches = append(todo.Patches[:pos], todo.Patches[pos+1:]...)

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
File dashboard/app/build/handler.go (right):

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:193: if com != nil {
delete this if block

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:200: if com != nil {
delete this if block

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:206: p, err := patchTodo(c, builder)
have patchTodo return a Commit
so this can just be

com, err = patchTodo(c, builder)

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:216: com.ResultData = []string{}
add below this
if kind != "benchmark-go-commit" {
  com.PerfResults = []string{}
}

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:311: if err == datastore.ErrNoSuchEntity {
move this above the previous if statement, then it can become

if err == datastore.NoSuchEntity {
  todo, err = buildPatchTodo(c, builder)
}
if err != nil {
  return nil, fmt.Errorf("patchTodo: %v", err)
}

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:326: tx := func(c appengine.Context) error {
err := datastore.RunInTransaction(c, func(c appengine.Context) error {
...
}, nil)
if err != nil {
  return nil, fmt.Errorf("buildPatchTodo: %v", err)
}
return todo, nil

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:329: return fmt.Errorf("fetching PatchTodo: %v",
err)
don't wrap these errors in fmt.Errorf, just do it once at the bottom of the
function

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:533: splitHash := strings.Split(com.Hash, "+")
write a function to split up the hash etc, so this code becomes:

p, err := patchFromHash
if err != nil {
  return err
}

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:760: if r.Method != "POST" {
we need some way to authenticate these requests; not just anyone should be able
to request our builders run arbitrary code

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:812: return addPatch(c, p)
why not just run both of these in the same transaction?

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

https://codereview.appspot.com/103800044/diff/80001/dashboard/builder/env.go#...
dashboard/builder/env.go:274: patchfile, err := downloadPatch(gccpath, CL)
rather than downloading the patch to disk and applying it, why not fetch it to
memory and pipe it into the 'patch' command's standard input? much easier to
apply any transformations to it, that way.

also means we can implement 'patch' in go at some point with minimal changes
(makes deployment easier on windows)

https://codereview.appspot.com/103800044/diff/80001/dashboard/builder/http.go
File dashboard/builder/http.go (right):

https://codereview.appspot.com/103800044/diff/80001/dashboard/builder/http.go...
dashboard/builder/http.go:226: if err := run(*cmdTimeout, nil, path,
wgetArgs...); err != nil {
don't use wget! we have a perfectly good net/http package

[cmang, 2014-08-27 22:00:05]

I'm not too attached to overriding the commit hash, either. It seemed like the
easiest way to have the dashboard application interact with the builders that
allowed the builders to stay simple. Since the builders don't know anything
about the internals of a commit except for the hashes the todoHandler tells them
to build, I thought just modifying the hash would be the best way to pass along
that information. In this way, the only extra step in the build process would be
optionally applying a patch.

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/build.go
File dashboard/app/build/build.go (right):

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/build...
dashboard/app/build/build.go:104: PatchAttempt      bool
On 2014/08/25 02:11:31, adg wrote:
> this is less clear than the previous definition.
> what does it mean? that we should attempt a patch? or that one has been
> attempted?
> 

The original definition was accidentally added in a previous CL while I was
thinking about this.  This was supposed to be a flag that indicates that the
given commit was a patch attempt and shouldn't be added to front-page of commits
or considered for updating the dashboard tip.  When I read TryPatch, especially
next to the NeedsBenchmarking flag, it seems to imply to me that there will be
some extra step taken at a later stage, when really it just means this commit's
hash has the patch modification added to it.

> plus i'm not sure we can rename this now, as we have entities in the datastore
> with this field set. (any entity in the datastore since this field was added,
at
> least)

Acknowledged.

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/build...
dashboard/app/build/build.go:423: // RemovePatch removes p from the set of
Patches to be built.
On 2014/08/25 02:11:31, adg wrote:
> this needs to be run inside a transaction

Done.

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/build...
dashboard/app/build/build.go:434: if tp.CL == p.CL && tp.Diff == p.Diff {
On 2014/08/25 02:11:31, adg wrote:
> if tp == p {
> ??
> surely commithash is relevant too?

Done.

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/build...
dashboard/app/build/build.go:444: todo.Patches[pos] =
todo.Patches[len(todo.Patches)-1]
On 2014/08/25 02:11:31, adg wrote:
> Clearer, and in one line:
> todo.Patches = append(todo.Patches[:pos], todo.Patches[pos+1:]...)

Done.

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
File dashboard/app/build/handler.go (right):

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:193: if com != nil {
On 2014/08/25 02:11:31, adg wrote:
> delete this if block

Done.

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:200: if com != nil {
On 2014/08/25 02:11:32, adg wrote:
> delete this if block

Done.

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:206: p, err := patchTodo(c, builder)
On 2014/08/25 02:11:32, adg wrote:
> have patchTodo return a Commit
> so this can just be
> 
> com, err = patchTodo(c, builder)

Done.

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:216: com.ResultData = []string{}
On 2014/08/25 02:11:31, adg wrote:
> add below this
> if kind != "benchmark-go-commit" {
>   com.PerfResults = []string{}
> }

Done.

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:311: if err == datastore.ErrNoSuchEntity {
On 2014/08/25 02:11:31, adg wrote:
> move this above the previous if statement, then it can become
> 
> if err == datastore.NoSuchEntity {
>   todo, err = buildPatchTodo(c, builder)
> }
> if err != nil {
>   return nil, fmt.Errorf("patchTodo: %v", err)
> }

Done.

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:326: tx := func(c appengine.Context) error {
On 2014/08/25 02:11:31, adg wrote:
> err := datastore.RunInTransaction(c, func(c appengine.Context) error {
> ...
> }, nil)
> if err != nil {
>   return nil, fmt.Errorf("buildPatchTodo: %v", err)
> }
> return todo, nil

Done.

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:329: return fmt.Errorf("fetching PatchTodo: %v",
err)
On 2014/08/25 02:11:32, adg wrote:
> don't wrap these errors in fmt.Errorf, just do it once at the bottom of the
> function
> 

Done.

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:533: splitHash := strings.Split(com.Hash, "+")
On 2014/08/25 02:11:32, adg wrote:
> write a function to split up the hash etc, so this code becomes:
> 
> p, err := patchFromHash
> if err != nil {
>   return err
> }

Done.

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:760: if r.Method != "POST" {
On 2014/08/25 02:11:32, adg wrote:
> we need some way to authenticate these requests; not just anyone should be
able
> to request our builders run arbitrary code

Who should be allowed to run `hg try`? The codereview extension does minor
authentication on the client's side, but I'm not sure if we can access that
information at this point. We could use chromium's method and just give trybot
access to all committers and have other non-committers that specifically have
try job access.

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...
dashboard/app/build/handler.go:812: return addPatch(c, p)
On 2014/08/25 02:11:31, adg wrote:
> why not just run both of these in the same transaction?

Done.

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

https://codereview.appspot.com/103800044/diff/80001/dashboard/builder/env.go#...
dashboard/builder/env.go:274: patchfile, err := downloadPatch(gccpath, CL)
On 2014/08/25 02:11:32, adg wrote:
> rather than downloading the patch to disk and applying it, why not fetch it to
> memory and pipe it into the 'patch' command's standard input? much easier to
> apply any transformations to it, that way.
> 
> also means we can implement 'patch' in go at some point with minimal changes
> (makes deployment easier on windows)

Done.

https://codereview.appspot.com/103800044/diff/80001/dashboard/builder/http.go
File dashboard/builder/http.go (right):

https://codereview.appspot.com/103800044/diff/80001/dashboard/builder/http.go...
dashboard/builder/http.go:226: if err := run(*cmdTimeout, nil, path,
wgetArgs...); err != nil {
On 2014/08/25 02:11:32, adg wrote:
> don't use wget! we have a perfectly good net/http package

Done.

[adg, 2014-08-28 23:59:20]

You need to "hg mail" or "hg upload" the changes.

[adg, 2014-08-29 00:05:50]

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/build.go
File dashboard/app/build/build.go (right):

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/build...
dashboard/app/build/build.go:104: PatchAttempt      bool
Rename it back to TryPatch; we can't go back now.

[cmang, 2014-08-29 00:33:03]

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/build.go
File dashboard/app/build/build.go (right):

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/build...
dashboard/app/build/build.go:104: PatchAttempt      bool
On 2014/08/29 00:05:49, adg wrote:
> Rename it back to TryPatch; we can't go back now.

Done.

[adg, 2014-09-04 01:53:25]

I would like Brad to take a look at this to see how it might affect his work on
the builders. In particular, the commit hash being overloaded with patch info
seems like it might trip something up.

https://codereview.appspot.com/103800044/diff/120001/dashboard/app/build/hand...
File dashboard/app/build/handler.go (right):

https://codereview.appspot.com/103800044/diff/120001/dashboard/app/build/hand...
dashboard/app/build/handler.go:312: err = datastore.Get(c, com.Key(c), com)
if err := ...; err != nil {
  return nil, err
}
return com, nil

[cmang, 2014-09-04 18:05:21]

Also, I didn't know about the taskqueue API beforehand, but it seems like we
could avoid the weird altering of commit hashes if both the application and
builders used them to communicate with each other instead of the raw hash.

https://codereview.appspot.com/103800044/diff/120001/dashboard/app/build/hand...
File dashboard/app/build/handler.go (right):

https://codereview.appspot.com/103800044/diff/120001/dashboard/app/build/hand...
dashboard/app/build/handler.go:312: err = datastore.Get(c, com.Key(c), com)
On 2014/09/04 01:53:25, adg wrote:
> if err := ...; err != nil {
>   return nil, err
> }
> return com, nil

Done.

[bradfitz, 2014-09-05 01:57:53]

https://codereview.appspot.com/103800044/diff/120001/dashboard/app/build/hand...
File dashboard/app/build/handler.go (right):

https://codereview.appspot.com/103800044/diff/120001/dashboard/app/build/hand...
dashboard/app/build/handler.go:172: Data interface{}
list the types for each of these 4

https://codereview.appspot.com/103800044/diff/120001/dashboard/app/build/hand...
dashboard/app/build/handler.go:206: if kind != "benchmark-go-commit" {
is this related to this CL?

https://codereview.appspot.com/103800044/diff/120001/dashboard/app/build/hand...
dashboard/app/build/handler.go:337: // patchFromHash takes a modified commit
hash and returns the corresponding patch.
what is a modified commit hash? list an example

https://codereview.appspot.com/103800044/diff/120001/dashboard/app/build/hand...
dashboard/app/build/handler.go:952: http.HandleFunc(d.RelPath+"try", tryHandler)
this can't be public, otherwise anybody could own our builders. where are the
auth checks?

[cmang, 2014-09-05 21:56:31]

https://codereview.appspot.com/103800044/diff/120001/dashboard/app/build/hand...
File dashboard/app/build/handler.go (right):

https://codereview.appspot.com/103800044/diff/120001/dashboard/app/build/hand...
dashboard/app/build/handler.go:172: Data interface{}
On 2014/09/05 01:57:53, bradfitz wrote:
> list the types for each of these 4

Done.

https://codereview.appspot.com/103800044/diff/120001/dashboard/app/build/hand...
dashboard/app/build/handler.go:206: if kind != "benchmark-go-commit" {
On 2014/09/05 01:57:52, bradfitz wrote:
> is this related to this CL?

https://codereview.appspot.com/103800044/diff/80001/dashboard/app/build/handl...

https://codereview.appspot.com/103800044/diff/120001/dashboard/app/build/hand...
dashboard/app/build/handler.go:337: // patchFromHash takes a modified commit
hash and returns the corresponding patch.
On 2014/09/05 01:57:52, bradfitz wrote:
> what is a modified commit hash? list an example

Done.

https://codereview.appspot.com/103800044/diff/120001/dashboard/app/build/hand...
dashboard/app/build/handler.go:952: http.HandleFunc(d.RelPath+"try", tryHandler)
On 2014/09/05 01:57:53, bradfitz wrote:
> this can't be public, otherwise anybody could own our builders. where are the
> auth checks?

It's still not very clear to me what authentication we can do here with what we
currently have.  We do authentication on the codereview extensions' side, but we
can't guarantee that this request was sent from that client as opposed to a curl
command.

We can't just use the email/username of the requester without having the
password that the codereview extension/rietvald application was given and it
doesn't seem right to send that data in this request.

Perhaps we can store a set of keys for trusted testers similar to the set of
keys we store/generate for different builders?

[gobot, 2014-12-19 05:08:38]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/103800044 is best) in the description in your
new CL.

Thanks very much.