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

Jump to bottom

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
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@rasky
Copy link
Member

rasky commented Sep 14, 2019 •
edited

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 x/build issues (builders, bots, dashboards) label Sep 14, 2019
@ALTree ALTree added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed Builders x/build issues (builders, bots, dashboards) labels Sep 14, 2019
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Sep 14, 2019
@bcmills
Copy link
Contributor

bcmills commented Sep 16, 2019

CC @dmitshur @toothrot @bradfitz

@martisch
Copy link
Contributor

martisch commented Sep 21, 2019 •
edited

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.

@bcmills bcmills mentioned this issue Sep 24, 2019
Closed
@rasky
Copy link
Member Author

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
Copy link
Member

ALTree commented Sep 26, 2019 •
edited

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
Copy link
Contributor

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.

@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 26, 2019
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 26, 2019
@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Sep 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@bradfitz @martisch @rasky @ALTree @bcmills @gopherbot