x/build: where'd the Go 1.13 nacl builders go? #34738

bradfitz · 2019-10-07T16:21:27Z

https://build.golang.org/?branch=release-branch.go1.13 has empty cells for the nacl builders.

They should still be running for Go 1.13. Where'd they go?

x/build/dashboard/builders_test.go has:

                {b("nacl-386", "go"), none},
                {b("nacl-386@go1.13", "go"), onlyPost},
                {b("nacl-386", "net"), none},
                {b("nacl-amd64p32", "go"), none},
                {b("nacl-amd64p32@go1.13", "go"), both},
                {b("nacl-amd64p32", "net"), none},

Suggesting that they should still be tested for Go 1.13, and the test passes.

So where'd they go?

/cc @andybons @dmitshur @bcmills

bcmills · 2019-10-07T16:33:03Z

Looks like they disappeared at some point in between CL 193269 (Sep. 5) and CL 193264 (Sep. 11).

dmitshur · 2019-10-07T17:05:25Z

They're also not running on release-branch.go1.12, last run was on Sep 6.

Coordinator was last deployed 19 days ago (Sep 18~).

The last run on master branch was on Aug 30 (https://build.golang.org/?page=13&branch=master).

dmitshur · 2019-10-07T19:32:49Z

The problem turned out to be an incompatible intersection of behaviors across cmd/coordinator and app/appengine (and serves as further motivation to merge the two codebases into one, made issue #34744).

CL 192537 modified cmd/coordinator to stop running nacl builders on master branch, and only run on release-branches older than 1.14.

However, app/appengine determines a list of builders by checking which builders report positively for bc.BuildsRepoPostSubmit("go", "master", "master"):

for name, bc := range dashboard.Builders {
	if !bc.BuildsRepoPostSubmit("go", "master", "master") {
		continue
	}
	builders[name] = true
}

(Source.)

So the nacl builders got unintentionally filtered out (see "builders" field in https://build.golang.org/?mode=json). That means findWork never considers them, even for release-branches.

I also found an unrelated problem in the TestBuilderConfig test. It wasn't a cause for this issue, but we should improve TestBuilderConfig anyway to remove a potential pitfall and code-readability distraction.

Problem in TestBuilderConfig test

The b helper is documented as:

// builder may end in "@go1.N" (as alias for "@release-branch.go1.N") or "@branch-name".
// repo may end in "@1.N" (as alias for "@release-branch.go1.N")
b := func(builder, repo string) builderAndRepo {

It's doing:

expandBranch(&br.branch)
expandBranch(&br.goBranch)
if br.repo == "go" {
	br.branch = br.goBranch
}

That means when the repo is "go", a line like b("nacl-386", "go@1.13") would be misinterpreted. The "@1.13" suffix would be dropped/overwritten, and "master" would be used instead.

gopherbot · 2019-10-08T14:24:32Z

Change https://golang.org/cl/199800 mentions this issue: app/appengine: don't exclude release-branch-only builders

The filtering of builders in commitBuilders was too strict, causing post-submit builders that are configured to run on release-branches only to not get included in the list of builders. This change makes the following builders no longer skipped: darwin-amd64-10_10 freebsd-386-10_3 freebsd-386-10_4 freebsd-386-11_1 freebsd-amd64-10_3 freebsd-amd64-10_4 freebsd-amd64-11_1 nacl-386 nacl-amd64p32 We may need to adjust the build.golang.org UI after to avoid including unhelpful columns, but this is a first step to get nacl builders to run on release branches again. Updates golang/go#34738 Updates golang/go#34744 Change-Id: Iaf2b93aedd5f44b48b9a63b57f12549fe50b1637 Reviewed-on: https://go-review.googlesource.com/c/build/+/199800 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

dmitshur · 2019-10-08T16:42:56Z

I deployed CL 199800 and it was sufficient to resolve this issue.

gopherbot · 2019-10-08T17:18:13Z

Change https://golang.org/cl/199817 mentions this issue: dashboard: catch potential pitfall in TestBuilderConfig

Previously, it was possible to accidentally try to specify the branch name for the Go repository by writing something like: {b("nacl-386", "go@go1.13"), none}, That looks like it would test the right thing, but in reality it would be equivalent to: {b("nacl-386@master", "go"), none}, Because the branch of the builder always overwrites the branch of the "go" repository. This is not intuitive and easy to miss when reviewing or reading the code. Add a check that detects and panics on this b() API pitfall, so that it is not accidentally introduced in the future, and so that code readers don't need to spend time verifying that none of the existing test cases have fallen victim to it. Updates golang/go#34738 Change-Id: Id2986e88c0e3585eaa7b45f33de97e1902847305 Reviewed-on: https://go-review.googlesource.com/c/build/+/199817 Reviewed-by: Bryan C. Mills <bcmills@google.com>

gopherbot · 2019-10-08T18:52:22Z

Change https://golang.org/cl/199878 mentions this issue: app/appengine: hide release-branch-only builders when viewing master branch

…branch This is a follow-up to CL 199800. That change started to consider more builders as active to allow release-branch-only builders to run. However, it also made those builders show up as new empty columns when viewing builds for the master branch. There are quite a few old FreeBSD builders that only run on release-branch.go1.12, and it's too disruptive to have them appear everywhere. So, hide them when viewing the master branch of Go repo in the UI. There can be more UI improvements to be made, and this can become too much of a whack-a-mole to address them one by one. The scope of this CL is to fix the most disruptive high priority problem for now. Further improvements will happen later, with merging of app/appengine and cmd/coordinator codebases in mind. Updates golang/go#34738 Updates golang/go#34744 Change-Id: I3df75f8b2bbd5f6fe8097c181ee8a1b1b4354dc9 Reviewed-on: https://go-review.googlesource.com/c/build/+/199878 Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>

The filtering of builders in commitBuilders was too strict, causing post-submit builders that are configured to run on release-branches only to not get included in the list of builders. This change makes the following builders no longer skipped: darwin-amd64-10_10 freebsd-386-10_3 freebsd-386-10_4 freebsd-386-11_1 freebsd-amd64-10_3 freebsd-amd64-10_4 freebsd-amd64-11_1 nacl-386 nacl-amd64p32 We may need to adjust the build.golang.org UI after to avoid including unhelpful columns, but this is a first step to get nacl builders to run on release branches again. Updates golang/go#34738 Updates golang/go#34744 Change-Id: Iaf2b93aedd5f44b48b9a63b57f12549fe50b1637 Reviewed-on: https://go-review.googlesource.com/c/build/+/199800 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

Previously, it was possible to accidentally try to specify the branch name for the Go repository by writing something like: {b("nacl-386", "go@go1.13"), none}, That looks like it would test the right thing, but in reality it would be equivalent to: {b("nacl-386@master", "go"), none}, Because the branch of the builder always overwrites the branch of the "go" repository. This is not intuitive and easy to miss when reviewing or reading the code. Add a check that detects and panics on this b() API pitfall, so that it is not accidentally introduced in the future, and so that code readers don't need to spend time verifying that none of the existing test cases have fallen victim to it. Updates golang/go#34738 Change-Id: Id2986e88c0e3585eaa7b45f33de97e1902847305 Reviewed-on: https://go-review.googlesource.com/c/build/+/199817 Reviewed-by: Bryan C. Mills <bcmills@google.com>

…branch This is a follow-up to CL 199800. That change started to consider more builders as active to allow release-branch-only builders to run. However, it also made those builders show up as new empty columns when viewing builds for the master branch. There are quite a few old FreeBSD builders that only run on release-branch.go1.12, and it's too disruptive to have them appear everywhere. So, hide them when viewing the master branch of Go repo in the UI. There can be more UI improvements to be made, and this can become too much of a whack-a-mole to address them one by one. The scope of this CL is to fix the most disruptive high priority problem for now. Further improvements will happen later, with merging of app/appengine and cmd/coordinator codebases in mind. Updates golang/go#34738 Updates golang/go#34744 Change-Id: I3df75f8b2bbd5f6fe8097c181ee8a1b1b4354dc9 Reviewed-on: https://go-review.googlesource.com/c/build/+/199878 Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>

gopherbot added this to the Unreleased milestone Oct 7, 2019

gopherbot added the Builders x/build issues (builders, bots, dashboards) label Oct 7, 2019

dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 7, 2019

dmitshur self-assigned this Oct 7, 2019

dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 7, 2019

gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 7, 2019

dmitshur assigned dmitshur and unassigned dmitshur Oct 7, 2019

dmitshur closed this as completed Oct 8, 2019

golang locked and limited conversation to collaborators Oct 7, 2020

gopherbot added the FrozenDueToAge label Oct 7, 2020

rsc unassigned dmitshur Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/build: where'd the Go 1.13 nacl builders go? #34738

x/build: where'd the Go 1.13 nacl builders go? #34738

bradfitz commented Oct 7, 2019

bcmills commented Oct 7, 2019

dmitshur commented Oct 7, 2019

dmitshur commented Oct 7, 2019

gopherbot commented Oct 8, 2019

dmitshur commented Oct 8, 2019

gopherbot commented Oct 8, 2019

gopherbot commented Oct 8, 2019

x/build: where'd the Go 1.13 nacl builders go? #34738

x/build: where'd the Go 1.13 nacl builders go? #34738

Comments

bradfitz commented Oct 7, 2019

bcmills commented Oct 7, 2019

dmitshur commented Oct 7, 2019

dmitshur commented Oct 7, 2019

gopherbot commented Oct 8, 2019

dmitshur commented Oct 8, 2019

gopherbot commented Oct 8, 2019

gopherbot commented Oct 8, 2019