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.

Sign up for GitHub

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: configure TryBots for google.golang.org/protobuf #63597

Closed
hoeppi-google opened this issue Oct 17, 2023 · 9 comments
Closed

x/build: configure TryBots for google.golang.org/protobuf #63597

hoeppi-google opened this issue Oct 17, 2023 · 9 comments
Assignees
@dmitshur
Labels
Builders x/build issues (builders, bots, dashboards) NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@hoeppi-google
Copy link
Contributor

Changes to the google.golang.org/protobuf module are currently not automatically tested before submission. It would be great if we could add TryBots for the repo. Testing for linux amd64 should be sufficient.
@heschi heschi changed the title TryBots for google.golang.org/protobuf x/build: configure TryBots for google.golang.org/protobuf Oct 17, 2023
@heschi heschi added the Builders x/build issues (builders, bots, dashboards) label Oct 17, 2023
@dmitshur dmitshur added this to the Unreleased milestone Oct 17, 2023
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 17, 2023
@dmitshur dmitshur self-assigned this Nov 7, 2023
@gopherbot
Copy link

Change https://go.dev/cl/540496 mentions this issue: main.star: add google.golang.org/protobuf repo

gopherbot pushed a commit to golang/build that referenced this issue Nov 7, 2023
@dmitshur @gopherbot
main.star: add google.golang.org/protobuf repo
11e05a0 
The canonical source for the google.golang.org/protobuf module is
the go.googlesource.com/protobuf repository. It's also currently
being mirrored to GitHub at github.com/protocolbuffers/protobuf-go.

Add it as a special project type configured to test on Linux only,
as requested in the issue. This can be expanded in the future.

For golang/go#63597.

Change-Id: I4c82fac2076cd2cd1660728bdf3b2c21be84fb10
Reviewed-on: https://go-review.googlesource.com/c/build/+/540496
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Bypass: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
@dmitshur
Copy link
Contributor

dmitshur commented Nov 7, 2023 •
edited

The trybots (and post-submit builders) have been turned up for this repo in CL 540496.
The permissions have been updated to require a passing trybot result in CL 540478.

Docs for running trybots is at https://go.dev/wiki/GerritAccess#running-trybots-may-start-trybots.
Please don't hesitate to let us know if you run into problems, or something doesn't work as expected.

@dmitshur dmitshur closed this as completed Nov 7, 2023
@stapelberg
Copy link
Contributor

Thank you @dmitshur for setting this up!

AFAICT, the current configuration is to run "go test -short".

That’s a good first signal, but we also have an integration test that’s started by running https://github.com/protocolbuffers/protobuf-go/blob/master/test.bash, which we would like to run automatically.

Would that be possible to configure as well? Thanks.

@stapelberg stapelberg reopened this Nov 8, 2023
@gopherbot
Copy link

Change https://go.dev/cl/540278 mentions this issue: main.star: enable longtest in presubmit for protobuf

@dmitshur
Copy link
Contributor

dmitshur commented Nov 8, 2023

That seems reasonable to me, especially since this repo only has Linux builders enabled so far, and would save you the trouble of having to opt-in to a longtest builder on each CL via slowbots. Mailed CL 540278.

gopherbot pushed a commit to golang/build that referenced this issue Nov 8, 2023
@dmitshur @gopherbot
main.star: enable longtest in presubmit for protobuf
10e6b97 
The protobuf repository (added in CL 540496) is special (i.e., it's
not one of the many golang.org/x repos that fits into a project type)
and configured to be tested only on Linux. Its owners find it helpful
for the longtest run mod to be enabled in presubmit by default.

After we finish taking care of the needs of special repositories and
see the patterns, we might want to refactor further.

For golang/go#63597.

Change-Id: I7edcefe8a49f1e89a497af390349412a32936462
Reviewed-on: https://go-review.googlesource.com/c/build/+/540278
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Bypass: Dmitri Shuralyov <dmitshur@golang.org>
@dmitshur
Copy link
Contributor

dmitshur commented Nov 8, 2023 •
edited

That's done.

If at some point you think it might make sense to test on more than just Linux, please feel to open a new issue to figure out those details. Thanks.

@dmitshur dmitshur closed this as completed Nov 8, 2023
@stapelberg
Copy link
Contributor

stapelberg commented Nov 9, 2023 •
edited by dmitshur

Thanks for setting up the longtest.

However, I think the longtest might just be doing go test (without -short), which is not sufficient to run the protobuf repository’s integration test. At least that’s how I read the LUCI results on https://go-review.googlesource.com/c/protobuf/+/541075?tab=checks

https://github.com/protocolbuffers/protobuf-go/blob/master/test.bash explicitly runs integration_test.go, which is marked as //go:build ignore and is not included in go test.

Can we get a ./test.bash run as well please?

@stapelberg stapelberg reopened this Nov 9, 2023
@gopherbot
Copy link

Change https://go.dev/cl/541123 mentions this issue: all: run integration test on longtest builders in CI

@dmitshur dmitshur mentioned this issue Nov 10, 2023
Closed
@dmitshur
Copy link
Contributor

dmitshur commented Nov 10, 2023 •
edited

Indeed—thank you for catching that, and apologies: I read your comment quickly and failed to realize there was more to getting the integration test started by tesh.bash to run than just configuring longtest builders to always run. I saw that the longtest builder was already passing and thought I just needed to also configure it to run in pre-submit for this repo (CL 540278).

I took a look at what it might take to make it work in CL 541123, and the good news are that I think there's a good path forward. It's passing on a Linux machine I tried it on in about 7 minutes with a clean cache, and in about 9 minutes with -race flag, but not yet on our LUCI builders because of a problem with bazel dependency, which I want to discuss. I opened #64066 to track that work, so let's continue the conversation there. Thanks.

Closing this issue with the scope limited to turning on basic builders in LUCI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dmitshur dmitshur

Labels
Builders x/build issues (builders, bots, dashboards) NeedsFix The path to resolution is known, but the work has not been done.
Projects
Go Release
Archived in project
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@stapelberg @dmitshur @gopherbot @heschi @hoeppi-google and others