code review 8266046: misc/dashboard/builder: add -race builder support

misc/dashboard/builder: add -race builder support

If the build key contains -race, the builder will invoke to the race.{bat,bash} build command. This allows {darwin,linux,windows}-amd64 builders to do race and non race builds in sequence.

[dave_cheney.net, 2013-04-05 01:39:02]

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

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

[dvyukov, 2013-04-05 05:01:00]

>This allows {darwin,linux,windows}-amd64 builders to do race and
non race builds in sequence.

I do not understand this part.
If the builder does not contain -race in it's name, then it won't do race build
at all. What I am missing?

Perhaps add -race flag, which enables race.bash after all.bash?
Then existing builders can add -race to builder command line and test
norace/race in sequence.

[dave_cheney.net, 2013-04-05 05:12:22]

> If the builder does not contain -race in it's name, then it won't do
> race build at all. What I am missing?

If the builder doesn't contain -race, then it will use whatever the
value of -buildCmd is (which defaults to all.{bash,bat}). Of the two
race builders we have, both have -race in their name.

> Perhaps add -race flag, which enables race.bash after all.bash?
> Then existing builders can add -race to builder command line and test
> norace/race in sequence.

No, I don't want to do that because then the builder is doing two
things and can only report one failure  The second reason is the
builders use whatever is in their workspace to build the subrepos, so
they cannot be both race and non race.

[albert.strasheim, 2013-04-05 05:17:29]

FWIW, I see race.bash does this:

go install -race std
go test -race -short std
go test -race -run=nothingplease -bench=.* -benchtime=.1s -cpu=4 std

go install -race std installs a race-enabled cmd/go, which sometimes hits this
bug when you run it:

https://code.google.com/p/go/issues/detail?id=4840

[dvyukov, 2013-04-05 05:42:06]

On Thu, Apr 4, 2013 at 10:17 PM, <fullung@gmail.com> wrote:

> FWIW, I see race.bash does this:
>
> go install -race std
> go test -race -short std
> go test -race -run=nothingplease -bench=.* -benchtime=.1s -cpu=4 std
>
> go install -race std installs a race-enabled cmd/go, which sometimes
> hits this bug when you run it:
>
>
https://code.google.com/p/go/**issues/detail?id=4840<https://code.google.com/...


Yeah, I know. I can't figure out what is the way to fix it.
The idea is that it should not systematically affect users. And for our own
purposes (more stable builders) we can fix it after Go1.1.

[albert.strasheim, 2013-04-05 05:44:08]

On 2013/04/05 05:42:06, dvyukov wrote:
> On Thu, Apr 4, 2013 at 10:17 PM, <mailto:fullung@gmail.com> wrote:
> > go install -race std installs a race-enabled cmd/go, which sometimes
> > hits this bug when you run it:
> Yeah, I know. I can't figure out what is the way to fix it.
> The idea is that it should not systematically affect users. And for our own
> purposes (more stable builders) we can fix it after Go1.1.

Sure. Maybe one idea is to do:

go install -race std
go install cmd/go

in race.bash until this gets fixed?

Regards

Albert

[dvyukov, 2013-04-05 05:44:18]

On Thu, Apr 4, 2013 at 10:12 PM, Dave Cheney <dave@cheney.net> wrote:

> > If the builder does not contain -race in it's name, then it won't do
> > race build at all. What I am missing?
>
> If the builder doesn't contain -race, then it will use whatever the
> value of -buildCmd is (which defaults to all.{bash,bat}). Of the two
> race builders we have, both have -race in their name.



Hey, wait, we have 2 race builders?



> > Perhaps add -race flag, which enables race.bash after all.bash?
> > Then existing builders can add -race to builder command line and test
> > norace/race in sequence.
>
> No, I don't want to do that because then the builder is doing two
> things and can only report one failure  The second reason is the
> builders use whatever is in their workspace to build the subrepos, so
> they cannot be both race and non race.
>


How people are supposed to use this.

[dave_cheney.net, 2013-04-05 05:46:39]

> Hey, wait, we have 2 race builders?

Yup, using this modification to the builder my darwin builder now runs
a -race job as well using the build key darwin-amd64-race-cheney.


> How people are supposed to use this.

They don't need to use this, but they can, like I did, add an extra
build job to their existing builder.

[dvyukov, 2013-04-05 05:47:45]

Have you seen such failures on race builders?
I can't recall it on my linux builder. And it was hard to reproduce on
synthetic stress test.


On Thu, Apr 4, 2013 at 10:44 PM, <fullung@gmail.com> wrote:

> On 2013/04/05 05:42:06, dvyukov wrote:
>
>> On Thu, Apr 4, 2013 at 10:17 PM, <mailto:fullung@gmail.com> wrote:
>> > go install -race std installs a race-enabled cmd/go, which sometimes
>> > hits this bug when you run it:
>> Yeah, I know. I can't figure out what is the way to fix it.
>> The idea is that it should not systematically affect users. And for
>>
> our own
>
>> purposes (more stable builders) we can fix it after Go1.1.
>>
>
> Sure. Maybe one idea is to do:
>
> go install -race std
> go install cmd/go
>
> in race.bash until this gets fixed?
>
> Regards
>
> Albert
>
>
https://codereview.appspot.**com/8266046/<https://codereview.appspot.com/8266...
>

[dvyukov, 2013-04-05 05:50:26]

On Thu, Apr 4, 2013 at 10:46 PM, Dave Cheney <dave@cheney.net> wrote:

> > Hey, wait, we have 2 race builders?
>
> Yup, using this modification to the builder my darwin builder now runs
> a -race job as well using the build key darwin-amd64-race-cheney.
>
>
> > How people are supposed to use this.
>
> They don't need to use this, but they can, like I did, add an extra
> build job to their existing builder.
>


It looks somewhat implicit and fragile that builder name affects the
command.
I would prefer -cmd=race.bash flag.

[dave_cheney.net, 2013-04-05 05:52:28]

> It looks somewhat implicit and fragile that builder name affects the
> command.
> I would prefer -cmd=race.bash flag.

Sure, but then you have to run two builders, or have two machines.

[albert.strasheim, 2013-04-05 05:54:27]

Hello

On 2013/04/05 05:47:45, dvyukov wrote:
> Have you seen such failures on race builders?
> I can't recall it on my linux builder. And it was hard to reproduce on
> synthetic stress test.

It's a numbers game. I ran into this about once a day with my test builder.

You'll see from my initial report in issue 4840 that I started by debugging
exactly such a stuck build.

I only brought this up because I seem to recall that unsticking a dashboard
builder is a manual process and you're making the race ones more likely to get
stuck by running with a race-enabled go command.

Cheers

Albert

[adg, 2013-04-08 04:39:33]

I would also prefer to just use -cmd=race.bash for now.

https://codereview.appspot.com/8266046/diff/5001/misc/dashboard/builder/main.go
File misc/dashboard/builder/main.go (right):

https://codereview.appspot.com/8266046/diff/5001/misc/dashboard/builder/main....
misc/dashboard/builder/main.go:219: if strings.Index(b.name, "-race") >= 0 {
strings.Contains

[adg, 2013-04-08 04:40:07]

LGTM

Actually, no. This is fine.

Please just address my comment first.

[dave_cheney.net, 2013-04-09 02:47:42]

Done. Submitting.

https://codereview.appspot.com/8266046/diff/5001/misc/dashboard/builder/main.go
File misc/dashboard/builder/main.go (right):

https://codereview.appspot.com/8266046/diff/5001/misc/dashboard/builder/main....
misc/dashboard/builder/main.go:219: if strings.Index(b.name, "-race") >= 0 {
On 2013/04/08 04:39:33, adg wrote:
> strings.Contains

Done.

[dave_cheney.net, 2013-04-09 02:48:21]

*** Submitted as https://code.google.com/p/go/source/detail?r=01679e274dd1 ***

misc/dashboard/builder: add -race builder support

If the build key contains -race, the builder will invoke to the race.{bat,bash}
build command. This allows {darwin,linux,windows}-amd64 builders to do race and
non race builds in sequence.

R=adg, dvyukov, fullung
CC=golang-dev
https://codereview.appspot.com/8266046