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: drop Go branch prefix from presubmit LUCI builders for the main Go repository #66009

Open
1 of 6 tasks
mknyszek opened this issue Feb 28, 2024 · 1 comment
Open
1 of 6 tasks
Labels
Builders x/build issues (builders, bots, dashboards) NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mknyszek
Copy link
Contributor

mknyszek commented Feb 28, 2024

Currently the LUCI builders all have a Go branch prefix. This makes the configuration simpler, and it makes the behavior of x/ repo builders against x/ repo changes explicit in the builder name.

This distinction is also necessary for displaying the Go repository postsubmit builders in the LUCI UI in a sensible way. If we were to drop the prefix and send all builds from all branches to the same Go repo builders in postsubmit, then the LUCI dashboard would display the latest build on any branch as the top-level status for a builder. This would make the overview of each dashboard at https://ci.chromium.org/p/golang very confusing. The LUCI UI generally treats distinct builders as having distinct histories, and we need to capture that distinction.

However, there are a few downsides, specifically for LUCI builders for the main Go repository in presubmit.

  • When the branch prefix is present in a commit message (via Cq-Include-Trybots) and the commit is cherry-picked to a release branch, the person performing the cherry-picking needs to be careful to update the Go branch in the commit message. If they do not, they will run into a failure to start trybots.
  • Because the filtering of the buildbucket Gerrit plugin is not very granular (on a bucket level only), this means the "Choose Tryjobs" dialog contains lots of builders that don't make sense for a particular change. For example, a Go repo change for the main branch should never run go1.21-* builders, yet they appear in the dialog as an option.

Note that dropping the prefix for presubmit makes sense while it doesn't for postsubmit, because presubmit builders don't really have a coherent notion of a history anyway.

I spent some time outlining how we might achieve this. Here are the steps:

  • Split out presubmit from the big config loop in main.star. (https://go.dev/cl/565420)
  • Split out the go project from the presubmit config loop from step (1).
  • Rework all the _version properties to be a map of Go branch names to versions.
    • Today, every builder will have exactly the same map, but builders that want to test against specific versions of some tool (node, clang, whatever) can still do so.
  • Allow "" and None as Go branch values throughout main.star.
    • Builder names simply omit the branch and properties must not depend on the branch name. (Caveat: I don't know yet how to enforce that.)
  • Allow "" as a valid go_branch property value in golangbuild (with an attempt to infer from the input change).
    • This will require some refactoring in golangbuild, because installTools will need to know the version to index into the map.
  • Rework the go project presubmit config loop to generate one builder per builder type, passing a None and/or "" Go branch name.

An alternative implementation would be to create the notion of a "proxy" builder whose only job consists of forwarding its invocation to a more specific build. This is doable, but the "proxy" build would likely need to stay live through the execution of the builder(s) it invokes, which burns an additional machine for no reason.

Though, this choice may have other benefits. For example, we could use it to create builders that represents sets of other builders, for greater ease-of-use during presubmit.

@mknyszek mknyszek added this to the Backlog milestone Feb 28, 2024
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Feb 28, 2024
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. Builders x/build issues (builders, bots, dashboards) and removed Builders x/build issues (builders, bots, dashboards) labels Feb 28, 2024
@mknyszek
Copy link
Contributor Author

Thinking more about this, I think there's a slightly simpler way that is a bit closer to the alternative I mentioned, but fewer downsides:

  1. Generate the per-branch builders as well as the not-per-branch builders.
  2. Have golangbuild fetch the properties for the per-branch builders when the Go branch is not specified, but once it infers it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Builders x/build issues (builders, bots, dashboards) NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: Planned
Development

No branches or pull requests

3 participants