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: add x/tools trybot for main Go repository #34348

Closed
dmitshur opened this issue Sep 17, 2019 · 8 comments
Closed

x/build: add x/tools trybot for main Go repository #34348

dmitshur opened this issue Sep 17, 2019 · 8 comments
Labels
Builders x/build issues (builders, bots, dashboards) FeatureRequest FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. new-builder
Milestone
Unreleased

Comments

@dmitshur
Copy link
Contributor

As @jayconrod suggested in #34321 (comment), I think we should add an x/tools trybot builder for the main Go repository. It would check whether x/tools tests on master branch pass with the Go CL being tested. We can use linux-amd64 builder type since it's quick and least expensive.

The Go standard library ships with many packages such as go/parser, go/types, and others. The x/tools repository has many tests that provide additional test coverage.

Trybots for the main Go repository already take 5-10 minutes to run, while the x/tools tests take less than 5 minutes, so it wouldn't slow anything down, just provide more test coverage.

/cc @bradfitz @andybons @toothrot
@dmitshur dmitshur added Builders x/build issues (builders, bots, dashboards) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. FeatureRequest new-builder labels Sep 17, 2019
@gopherbot gopherbot added this to the Unreleased milestone Sep 17, 2019
@stamblerre
Copy link
Contributor

Just wanted to chime in here to say that this would be extremely helpful for people developing in x/tools. go/packages and goimports depend on the behavior of go list, so if a breaking change is made in the go command, we may only notice the problem a few hours after the CL is submitted, and then someone gets interrupted figuring out the relevant CL and required fix. It would be really helpful to know about breaking changes upfront (even if they still get submitted). If there's anything I can do to help move this forward, I'd be happy to.

@bradfitz
Copy link
Contributor

@stamblerre, look at x/build/cmd/coordinator and where the recent slowbots code hooks and modifies the try set. I suspect this would also live there. We'd just add a build that's for "tools" instead of "go". There's a func getRepoHead that'll tell you which git rev of x/tools to use.

While I agree we should include x/tools by default, I also think the slowbots should let us opt in to testing any x/ repo. Maybe TRY=x/foo, using x/foo for the opt-in TRY tokens.

@gopherbot
Copy link

Change https://golang.org/cl/203677 mentions this issue: cmd/coordinator: add TRY=x/foo syntax for testing subrepos

@stamblerre
Copy link
Contributor

Thanks for the guidance, @bradfitz! Just mailed CL 203677, though I'm not sure about the correct way to test it.

gopherbot pushed a commit to golang/build that referenced this issue Oct 28, 2019
@stamblerre @bradfitz
cmd/coordinator: add TRY=x/foo syntax for testing subrepos
b0cde52 
This change adds support for user-invoked TryBots through the new TRY=
syntax. If the user comments TRY=x/foo on a CL to the Go repository, the
linux-amd64 builder will run at head for that repository.

Updates golang/go#34348

Change-Id: I0e7f470329866969586057501034385596e5caa0
Reviewed-on: https://go-review.googlesource.com/c/build/+/203677
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/203917 mentions this issue: cmd/coordinators: fix formatting for x/ repo TryBots

gopherbot pushed a commit to golang/build that referenced this issue Oct 28, 2019
@stamblerre @bradfitz
cmd/coordinator: fix formatting for x/ repo TryBots
da8ade5 
This change also adds a test for the (*buildStatus).NameAndBranch
method. It also specifies which x/ repo TryBots ran for a given change.

Finally, it fixes the error of specifying which Go commit to run the x/
repos with.

Updates golang/go#34348

Change-Id: Ib63fa6948c3798a85174b382de38f2bc159b3347
Reviewed-on: https://go-review.googlesource.com/c/build/+/203917
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@stamblerre
Copy link
Contributor

As of https://golang.org/cl/203917, you should now be able to comment "TRY=x/tools" on CLs that may be relevant to changes in x/tools (see https://golang.org/cl/201203 as an example).

@jayconrod and @bcmills: do you mind testing this out on your next few CLs to see if it works well for you? The next step would be to trigger it by default when certain directories or files are changed.

@stamblerre stamblerre mentioned this issue Oct 29, 2019
Closed
@gopherbot
Copy link

Change https://golang.org/cl/204199 mentions this issue: cmd/coordinator: run x/tools TryBots by default on Go CLs

@dmitshur
Copy link
Contributor Author

I think we're in favor of always including x/tools as described in the original issue, so I'll update the status to NeedsFix.

We can always revisit this decision and adjust it if it turns out to be more noisy than helpful.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Oct 30, 2019
codebien pushed a commit to codebien/build that referenced this issue Nov 13, 2019
@stamblerre @codebien
cmd/coordinator: add TRY=x/foo syntax for testing subrepos
8d7cc52 
This change adds support for user-invoked TryBots through the new TRY=
syntax. If the user comments TRY=x/foo on a CL to the Go repository, the
linux-amd64 builder will run at head for that repository.

Updates golang/go#34348

Change-Id: I0e7f470329866969586057501034385596e5caa0
Reviewed-on: https://go-review.googlesource.com/c/build/+/203677
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
codebien pushed a commit to codebien/build that referenced this issue Nov 13, 2019
@stamblerre @codebien
cmd/coordinator: fix formatting for x/ repo TryBots
c844855 
This change also adds a test for the (*buildStatus).NameAndBranch
method. It also specifies which x/ repo TryBots ran for a given change.

Finally, it fixes the error of specifying which Go commit to run the x/
repos with.

Updates golang/go#34348

Change-Id: Ib63fa6948c3798a85174b382de38f2bc159b3347
Reviewed-on: https://go-review.googlesource.com/c/build/+/203917
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
codebien pushed a commit to codebien/build that referenced this issue Nov 13, 2019
@stamblerre @codebien
cmd/coordinator: run x/tools TryBots by default on Go CLs
43fe31d 
Fixes golang/go#34348

Change-Id: I29db60f90f4eb37a37673bb854ae215e1b0c68a9
Reviewed-on: https://go-review.googlesource.com/c/build/+/204199
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Oct 30, 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) FeatureRequest FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. new-builder
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@bradfitz @dmitshur @stamblerre @gopherbot