code review 114780043: go.tools/dashboard/builder: automatically choose naclte...

go.tools/dashboard/builder: automatically choose nacltest.bash or androidtest.bash

This change extends the buildCmd logic to automatically choose the nacltest.bash or androidtest.bash build script if the builder key represents a nacl or android job respectively.

[dave_cheney.net, 2014-07-12 07:00:35]

Hello minux@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

[crawshaw, 2014-07-12 13:33:43]

While you're here, could you do android-* and androidtest.bash too?

[dave_cheney.net, 2014-07-13 09:04:43]

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

Please take another look.

[crawshaw, 2014-07-13 13:07:29]

LGTM with CL description update.

[dave_cheney.net, 2014-07-13 22:51:14]

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

Please take another look.

[minux, 2014-07-14 19:28:09]

LGTM.

[bradfitz, 2014-07-14 20:49:14]

Seems like this should only change the default or be ignored if the an explicit
build command is specified. The flag documentation wasn't updated either, so
it's now misleading.

[minux, 2014-07-14 21:44:11]

As an alternative, we can keep the builder code, and do the switch
(only for nacl and android) in all.bash.

This would also make it easier for our users: always run all.bash
after optionally set GOOS and GOARCH.
(right now running all.bash with GOOS=nacl will fail miserably.)

[dave_cheney.net, 2014-07-14 22:15:38]

Running GOOS=nacl ./all.bash works on my arm builder, it only fails under
the builder (for me)
On 15 Jul 2014 07:44, <minux@golang.org> wrote:

> As an alternative, we can keep the builder code, and do the switch
> (only for nacl and android) in all.bash.
>
> This would also make it easier for our users: always run all.bash
> after optionally set GOOS and GOARCH.
> (right now running all.bash with GOOS=nacl will fail miserably.)
>
> https://codereview.appspot.com/114780043/
>

[minux, 2014-07-14 22:23:14]

On Mon, Jul 14, 2014 at 6:15 PM, Dave Cheney <dave@cheney.net> wrote:

> Running GOOS=nacl ./all.bash works on my arm builder, it only fails under
> the builder (for me)
>
all.bash will succeed only when there is a up-to-date
src/pkg/syscall/fstest_nacl.go

so, running that in a freshly checked out tree will fail whereas running it
in your development tree
(which you have already run nacltest.bash at least once) will succeed.

[dave_cheney.net, 2014-07-14 22:26:38]

Thanks for the explanation, that explains what I was seeing.
On 15 Jul 2014 08:23, "minux" <minux@golang.org> wrote:

>
> On Mon, Jul 14, 2014 at 6:15 PM, Dave Cheney <dave@cheney.net> wrote:
>
>> Running GOOS=nacl ./all.bash works on my arm builder, it only fails under
>> the builder (for me)
>>
> all.bash will succeed only when there is a up-to-date
> src/pkg/syscall/fstest_nacl.go
>
> so, running that in a freshly checked out tree will fail whereas running
> it in your development tree
> (which you have already run nacltest.bash at least once) will succeed.
>

[dave_cheney.net, 2014-07-16 11:20:51]

PTAL. I've updated the flag documentation as requested.

I appreciate the alternative suggestions for how to handle the choice of builder
command, please let me know if I should abandon this CL.

[minux, 2014-07-16 17:20:38]

I agree with Brad that if the user passes a non-default
value ("all.bash") to -buildcmd, then we shouldn't
override it.

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

https://codereview.appspot.com/114780043/diff/80001/dashboard/builder/doc.go#...
dashboard/builder/doc.go:39: This flag is overrididen in the following
condtions:
typos (conditions, overridden)

https://codereview.appspot.com/114780043/diff/80001/dashboard/builder/doc.go#...
dashboard/builder/doc.go:41: - If the build key ends in -race, then race.bash or
race.bat will be chosen.
build key is not defined here, please add some explanation to the
top (Usage:)

And then build key in line 53 refers to something completely
different.

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

https://codereview.appspot.com/114780043/diff/80001/dashboard/builder/main.go...
dashboard/builder/main.go:74: naclCmd     = "nacltest" + suffix
no need to add suffix here, we don't have nacltest.bat,
and probably won't.

https://codereview.appspot.com/114780043/diff/80001/dashboard/builder/main.go...
dashboard/builder/main.go:75: androidCmd  = "androidtest" + suffix
similarly for this, although I don't know if we
want to have a androidtest.bat in the future.

I guess we can keep the "+ suffix" here just in
case.

[gobot, 2014-12-19 05:11:00]

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/114780043 is best) in the description in your
new CL.

Thanks very much.