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

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

Comments

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Sep 17, 2019

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

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Oct 25, 2019

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

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Oct 25, 2019

@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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 28, 2019

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

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Oct 28, 2019

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
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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 28, 2019

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
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

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Oct 29, 2019

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.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 30, 2019

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

@dmitshur

This comment has been minimized.

Copy link
Member Author

@dmitshur dmitshur commented Oct 30, 2019

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 and removed NeedsDecision labels Oct 30, 2019
codebien added a commit to codebien/build that referenced this issue Nov 13, 2019
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 added a commit to codebien/build that referenced this issue Nov 13, 2019
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 added a commit to codebien/build that referenced this issue Nov 13, 2019
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.