race detector support

[jmhodges]

I know this is in the TODO, but I was wondering what y'all thought this would look like. I'd like to have it for my own tooling and might have some spare cycles to get it done but I'm not sure in which direction to go.

[spxtr]

I was hoping this would be as easy as adding --test_arg=-race or --test_arg=-test.race, which can currently add most test args. That doesn't work. If you run go test -c, the resulting test binary doesn't accept the race flag. If you run go test -c -race then it works as we want.

[spxtr]

After #291, setting gc_linkopts = ["-race"] in go_test or go_binary rules works.

[jayconrod]

@spxtr That probably won't work correctly, even if it successfully produces output, too. At least you'll need to add -race to gc_goopts, too. I don't think this will link against standard library packages compiled with -race.

[spxtr]

It seems to work fine for go_test. From this example:

func TestRace(t *testing.T) {
	c := make(chan bool)
	m := make(map[string]string)
	go func() {
		m["1"] = "a" // First conflicting access.
		c <- true
	}()
	m["2"] = "b" // Second conflicting access.
	<-c
	for k, v := range m {
		fmt.Println(k, v)
	}
}

is failing the test plus giving me a stacktrace and

--- FAIL: TestRace (0.00s)
	testing.go:610: race detected during execution of test
FAIL

This is with only gc_linkopts = ["-race"].

[jmhodges]

Weird! Definitely need both of those over here!

    gc_goopts = ["-race"],
    gc_linkopts = ["-race"],

I think maybe we could close this with a README update, unless we want to make this a new attribute?

It would be handy to have a way to say "run all go tests with -race" instead of specifying it for each, but that might already exist with a bazel cli flag and I'm just ignorant of how rules_go is working right now.

I'm still willing to some work for this, including just that README update!

[mikedanese]

Does that detect a race in a depending package? Move the body of your test out of the package, call it from the test, and compile just the go_test with -race. I suspect that it won't detect the race.

[jmhodges]

Ah, I thought those opts were passed down to each package, but no, you're right.

So the trick that's holding this back the race detector is either passing -race to all go compiles somehow or giving a go_* target the ability to affect go_* targets it depends on.

[jmhodges]

Does that sound right? Other alternatives? Something something bazel aspects?

Willing to help out on whichever of these things is what people are into!

[mikedanese]

@jmhodges I think you would want to use https://docs.bazel.build/versions/master/skylark/rules.html#output-groups like @ianthehat is planning on doing with go plugins and cross compilation. It seems like a good fit for -race as well.

[jmhodges]

Hm, okay, so thinking through this!

With OutputGroups is for creating a second set of outputs so a different name is needed for the outputted binaries and library files.

The first naive idea I had was that you could just tack on a "-race" but then would require us rewriting those file paths when we go from the import paths referenced in Go sources which is a little nuts.

Instead, we could add another directory as a prefix similar to how the go tool works (which makes a top level dir it cds into that's either "$GOOS_$GOARCH" or "$GOOS_$GOARCH_race") and then, figuratively, cd into it in order to keep the import paths in the Go sources files working.

So that would involve swapping out _go_toolchain for a race version with a different go.path attribute on it. (I've not investigated how that figurative swap would work because my laptop is about to die)

Does that sound like the right direction?

[jmhodges]

Hm, but that only works if every target in the chain of deps sets a race parameter to pick that race-enabled toolchain.

I'm not sure how to swap out that go_toolchain whole cloth for every target at once.

[jmhodges]

Oh, duh, duh, the output_group flag

[jmhodges]

Oh shoot, I misread the code and the toolchain doesn't seem to control the pathing. Not sure if there's a nice central place that does

[jmhodges]

Okay, I took a crack at this, but I'm not sure how to operate with OutputGroups correctly and I'm not sure if it's actually a thing. Do you folks have internal repos or something that use it?

Partly, it's been a struggle to use because the documentation is lacking but I know where bazel's maturity is at so that's fine and understandable.

The real problem I have with using OutputGroups is that it seems unused in all of the bazelbuild repos, which would imply that this is not actually an API that's desirable to use.

In fact, there's only one use of it in code in bazelbuild repos I've found. And that one use doesn't even use OutputGroups, but only DefaultInfo.

There are zero uses of it between buildtools, rules_docker, rules_go, rules_closure, and rules_webtesting. The one use of it I've found is in the bazel repo proper is for _java_library_srcs_impl but all that does is return [DefaultInfo(files=depset(dep.java.source_jars))]. No OutputGroups at all!

Further googling doesn't turn up any github.com/bazelbuild repos that use it, either.

Are we sure OutputGroups is a thing?

(I made a gist of stuff I've figured out so far, but was too long to include here.)

[jmhodges]

Okay, I just found bazelbuild/bazel#2219 and its linked document on how to add platforms and maybe OutputGroups isn't used because everyone is waiting for this platform support to go live?

[jayconrod]

@jmhodges Sorry for slow responses. GopherCon is still going on.

The platforms and toolchain functionality is mostly implemented on branches, both in Bazel and in the Go rules. That functionality is being reviewed in chunks and landed in Bazel incrementally. As soon as it's part of a Bazel release, we'll finish wiring it in on our side -- shouldn't take long.

I'm not sure platforms will absolutely be needed to implement -race, but it's probably a good idea to wait. Output groups are a possibility for linking on the host platform since there's a precompiled set with -race. That would make cross-platform support for -race even more complicated though. We should probably take a unified approach.

[ianthehat]

Hi, sorry for the delayed reply, working from London and they took my internet away for a day.

I you want to see an example of output groups, you can look at #617 where I advised jmillikin-stripe on how to use them. The documentation is severely lacking, and there are a couple of gotchas. It took me a few tries to make them work, but they do work right now and don't need the toolchain stuff.

If you want to see where toolchains are going you can look at the wip

For implementing the -race and friends, it's a complicated story...

In theory you could do it with toolchains, but using toolchains for this is nailing with a sledge hammer, and a very expensive one at that. Generating toolchains for all these options would cause exponential toolchain counts, and they are not cheap things. It also leaves you effectively triggering full rebuilds every time you switch between race and normal (because the file names are the same but the options are different) and the selection mechanism is not friendly to this kind of use case (not right now, or any time soon anyway)

Doing it with output groups is great except for one thing, tests always run the "executable" output, which is always in the default output group, so while you could build binaries and test with -race, if you want to run tests under bazel with -race then it's not going to be enough.

My plan was to add the output group support to all the go_library, go_binary and go_test targets, but then also have a flag/setting that allows you to select which of the output's also becomes the default one for tests.

I was planning on working on this some time next month (doing all the various build controls, race, plugin, msan etc), if you want to tackle this, happy to support you, but if I were you I would just wait and let someone else take the pain :)

[jmhodges]

Oh, I wasn't worried about your reply time at all! It was a Friday etc. Thank you so much for giving me your time!

I will def take a look at that toolchain work. I'm right now pretty sure I'll end up deferring to you and your work but if that changes, I'll let you know!

Thanks for letting me know where the design is at and I'm crossing my fingers that the final bazel implementation works out in our favor.

[ianthehat]

I could not sleep last night, so I made #635 for your enjoyment...
Please give it a spin and see if it works for you.

[jmhodges]

Oh wow! Thank you! Wilco!