Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/build: run full codegen testsuite (-all_codegen) on amd64 trybots #34297

Closed
rasky opened this issue Sep 14, 2019 · 6 comments
Closed

x/build: run full codegen testsuite (-all_codegen) on amd64 trybots #34297

rasky opened this issue Sep 14, 2019 · 6 comments
Milestone

Comments

@rasky
Copy link
Member

@rasky rasky commented Sep 14, 2019

In 4f248e9 (CL 177577), @rsc introduced -all_codegen flag to test/run.go as a way to gate running the codegen testsuite with cross-compilation to all target architectures. In particular, it is now disabled in all.sh saving ~10 seconds (most of the time comes from the need to cross-compile runtime, math and a few other packages on all target architectures).

Unfortunately, none of the trybots has been updated to use -all_codegen and we don't have trybots for all possible target architectures and variants. This already caused the build to break 3 times:

Codegen bugs are hairy, often affect multiple platforms, and are hard to detect. The codegen testsuite was explicitly design to cross-compile by default as the only way for compiler developers to catch those bugs.

Disabling it in all.sh can make sense as most CLs and most developers are not working on the compiler, but not running it at all before landing a CL sounds too risky.

My proposal is to run run.go -all_codegen on one trybot, for instance the linux-amd64-ssacheck one. We don't need it in multiple trybots as the results would be the same. I would also change the output of the testsuite to explicitly mention using the -all_codegen flag to reproduce (as running in the ssacheck builder might confuse people that what they are seeing requires SSA checks to be the on).

/cc @randall77 @dr2chase

@ALTree ALTree changed the title Run full codegen testsuite (-all_codegen) on amd64 trybots x/build: run full codegen testsuite (-all_codegen) on amd64 trybots Sep 14, 2019
@gopherbot gopherbot added this to the Unreleased milestone Sep 14, 2019
@gopherbot gopherbot added the Builders label Sep 14, 2019
@ALTree ALTree added NeedsDecision and removed Builders labels Sep 14, 2019
@gopherbot gopherbot added the Builders label Sep 14, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Sep 16, 2019

@martisch

This comment has been minimized.

Copy link
Member

@martisch martisch commented Sep 21, 2019

I think we can add 9c384cc (@agnivade) to the list of CLs breaking codegen tests and not being caught by trybots. Although the builder did not fail test$ ../bin/go run run.go -all_codegen -v codegen fails for me locally.

@rasky

This comment has been minimized.

Copy link
Member Author

@rasky rasky commented Sep 26, 2019

Who is in charge of making a decision here? Who owns the -ssacheck builder? I think codegen is part of compiler / SSA checking.

@ALTree

This comment has been minimized.

Copy link
Member

@ALTree ALTree commented Sep 26, 2019

AFAIK @dmitshur and @bradfitz own the trybots and the builders; but as long as you don't make them too slow (IIRC the goal is to have them return a result within 5 minutes) I don't think they will have anything against adding a new check to one of them.

So the question is: how fast the linux-amd64-ssacheck trybot is? And if you add -all_codegen, it'll be still faster than the currently slowest trybot? If yes, adding the flag won't slow down the trybots overall, so it should be fine.

EDIT: looks like we have a decision : )

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Sep 26, 2019

I'll be the decider here.

I decide that the loss of test coverage as an accident. I don't think Russ meant to affect the builders. He just wanted all.bash to be faster on his laptop.

We should fix it.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 26, 2019

Change https://golang.org/cl/197539 mentions this issue: test: make -all_codegen default to true on linux-amd64 builder

@gopherbot gopherbot closed this in cb418dd Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.