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: where'd the Go 1.13 nacl builders go? #34738

Closed
bradfitz opened this issue Oct 7, 2019 · 7 comments

Comments

@bradfitz
Copy link
Member

commented Oct 7, 2019

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

@gopherbot gopherbot added this to the Unreleased milestone Oct 7, 2019
@gopherbot gopherbot added the Builders label Oct 7, 2019
@bcmills

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

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

@dmitshur

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

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 dmitshur self-assigned this Oct 7, 2019
@dmitshur

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

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.

@dmitshur dmitshur added the NeedsFix label Oct 7, 2019
@dmitshur dmitshur assigned dmitshur and unassigned dmitshur Oct 7, 2019
@gopherbot

This comment has been minimized.

Copy link

commented Oct 8, 2019

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

gopherbot pushed a commit to golang/build that referenced this issue Oct 8, 2019
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

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

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

image

@dmitshur dmitshur closed this Oct 8, 2019
@gopherbot

This comment has been minimized.

Copy link

commented Oct 8, 2019

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

gopherbot pushed a commit to golang/build that referenced this issue Oct 8, 2019
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

This comment has been minimized.

Copy link

commented Oct 8, 2019

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

gopherbot pushed a commit to golang/build that referenced this issue Oct 8, 2019
…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>
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.