dashboard, cmd/coordinator: various fixes
* add support for longtest builders for subrepos.
* fix race builders for subrepos (they weren't passing the -race flag).
* adjust the policy for the js-wasm builders to build fewer subrepos
where it'll never work or isn't worth it.
* fix the android emu builders which disappeared because an empty
string was being passed to buildsRepoAtAll. In some places in the
code an empty string for goBranch for the "go" repo meant the same
as branch, but I forgot that in the new code, so an old caller was
confusing the new config hooks. Rather than make all policy funcs be
aware of both ways, the new code in this CL now maps an empty string
to the same as the repo's branch when the repo == "go". Adds a test
too.
* fix some outdated comments.
Change-Id: Icf3fb85e5542a4d314443b59d02517b306ef46b7
Reviewed-on: https://go-review.googlesource.com/c/build/+/166897
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
diff --git a/cmd/coordinator/coordinator.go b/cmd/coordinator/coordinator.go
index 26f3541..2f037bf 100644
--- a/cmd/coordinator/coordinator.go
+++ b/cmd/coordinator/coordinator.go
@@ -798,7 +798,6 @@
// findWorkLoop polls https://build.golang.org/?mode=json looking for new work
// for the main dashboard. It does not support gccgo.
-// TODO(bradfitz): it also currently does not support subrepos.
func findWorkLoop(work chan<- buildgo.BuilderRev) {
// Useful for debugging a single run:
if inStaging && false {
@@ -907,7 +906,7 @@
builder := bs.Builders[i]
builderInfo, ok := dashboard.Builders[builder]
if !ok {
- // Not managed by the coordinator, or a trybot-only one.
+ // Not managed by the coordinator.
continue
}
if !builderInfo.BuildsRepoPostSubmit(br.Repo, br.Branch, br.GoBranch) {
@@ -916,12 +915,12 @@
var rev buildgo.BuilderRev
if br.Repo == "go" {
rev = buildgo.BuilderRev{
- Name: bs.Builders[i],
+ Name: builder,
Rev: br.Revision,
}
} else {
rev = buildgo.BuilderRev{
- Name: bs.Builders[i],
+ Name: builder,
Rev: br.GoRevision,
SubName: br.Repo,
SubRev: br.Revision,
@@ -2441,13 +2440,23 @@
"GOPROXY="+moduleProxy(), // GKE value but will be ignored/overwritten by reverse buildlets
)
env = append(env, st.conf.ModulesEnv(st.SubName)...)
+
+ args := []string{"test"}
+ if !st.conf.IsLongTest() {
+ args = append(args, "-short")
+ }
+ if st.conf.IsRace() {
+ args = append(args, "-race")
+ }
+ args = append(args, subrepoPrefix+st.SubName+"/...")
+
return st.bc.Exec(path.Join("go", "bin", "go"), buildlet.ExecOpts{
Debug: true, // make buildlet print extra debug in output for failures
Output: st,
Dir: "gopath/src/golang.org/x/" + st.SubName,
ExtraEnv: env,
Path: []string{"$WORKDIR/go/bin", "$PATH"},
- Args: []string{"test", "-short", subrepoPrefix + st.SubName + "/..."},
+ Args: args,
})
}
diff --git a/cmd/coordinator/coordinator_test.go b/cmd/coordinator/coordinator_test.go
index 0ed8962..330d99c 100644
--- a/cmd/coordinator/coordinator_test.go
+++ b/cmd/coordinator/coordinator_test.go
@@ -8,6 +8,7 @@
import (
"io/ioutil"
+ "log"
"net/http"
"net/http/httptest"
"reflect"
@@ -15,6 +16,7 @@
"testing"
"time"
+ "golang.org/x/build/buildenv"
"golang.org/x/build/internal/buildgo"
"golang.org/x/build/maintner/maintnerd/apipb"
)
@@ -169,3 +171,30 @@
t.Logf("build[%d]: %s", i, v)
}
}
+
+func TestFindWork(t *testing.T) {
+ if testing.Short() {
+ t.Skip("skipping in short mode")
+ }
+ defer func(old *buildenv.Environment) { buildEnv = old }(buildEnv)
+ buildEnv = buildenv.Production
+ defer func() { buildgo.TestHookSnapshotExists = nil }()
+ buildgo.TestHookSnapshotExists = func(br *buildgo.BuilderRev) bool {
+ if strings.Contains(br.Name, "android") {
+ log.Printf("snapshot check for %+v", br)
+ }
+ return false
+ }
+
+ c := make(chan buildgo.BuilderRev, 1000)
+ go func() {
+ defer close(c)
+ err := findWork(c)
+ if err != nil {
+ t.Error(err)
+ }
+ }()
+ for br := range c {
+ t.Logf("Got: %v", br)
+ }
+}
diff --git a/dashboard/builders.go b/dashboard/builders.go
index 6615902..a9e51a9 100644
--- a/dashboard/builders.go
+++ b/dashboard/builders.go
@@ -652,7 +652,7 @@
// buildsRepo optionally specifies whether this
// builder does builds (of any type) for the given repo ("go",
// "net", etc) and its branch ("master", "release-branch.go1.12").
- // If nil, a default policy is used.
+ // If nil, a default policy is used. (see buildsRepoAtAll for details)
// goBranch is the branch of "go" to build against. If repo == "go",
// goBranch == branch.
buildsRepo func(repo, branch, goBranch string) bool
@@ -840,6 +840,10 @@
return strings.HasSuffix(c.Name, "-race")
}
+func (c *BuildConfig) IsLongTest() bool {
+ return strings.HasSuffix(c.Name, "-longtest")
+}
+
func (c *BuildConfig) GoInstallRacePackages() []string {
if c.InstallRacePackages != nil {
return append([]string(nil), c.InstallRacePackages...)
@@ -929,7 +933,25 @@
// repo ("go", "sys", "net", etc). This applies to both post-submit
// and trybot builds. Use BuildsRepoPostSubmit for only post-submit
// or BuildsRepoTryBot for trybots.
+//
+// The branch is the branch of repo ("master",
+// "release-branch.go1.12", etc); it is required. The goBranch is the
+// branch of Go itself. It's required if repo != "go". When repo ==
+// "go", the goBranch defaults to the value of branch.
func (c *BuildConfig) buildsRepoAtAll(repo, branch, goBranch string) bool {
+ if goBranch == "" {
+ if repo == "go" {
+ goBranch = branch
+ } else {
+ panic("missing goBranch")
+ }
+ }
+ if branch == "" {
+ panic("missing branch")
+ }
+ if repo == "" {
+ panic("missing repo")
+ }
// Don't build old branches.
const minGo1x = 11
if strings.HasPrefix(goBranch, "release-branch.go1") && !atLeastGo1(goBranch, minGo1x) {
@@ -1388,7 +1410,6 @@
Name: "linux-amd64-race",
HostType: "host-linux-jessie",
tryBot: defaultTrySet(),
- buildsRepo: onlyGo,
MaxAtOnce: 1,
ShouldRunDistTest: fasterTrybots,
numTestHelpers: 1,
@@ -1452,11 +1473,13 @@
},
})
addBuilder(BuildConfig{
- Name: "linux-amd64-longtest",
- HostType: "host-linux-stretch",
- MaxAtOnce: 1,
- Notes: "Debian Stretch with go test -short=false",
- buildsRepo: onlyGo,
+ Name: "linux-amd64-longtest",
+ HostType: "host-linux-stretch",
+ MaxAtOnce: 1,
+ Notes: "Debian Stretch with go test -short=false",
+ buildsRepo: func(repo, branch, goBranch string) bool {
+ return repo == "go" || (branch == "master" && goBranch == "master")
+ },
needsGoProxy: true, // for cmd/go module tests
env: []string{
"GO_TEST_SHORT=0",
@@ -1531,7 +1554,14 @@
HostType: "host-js-wasm",
tryBot: explicitTrySet("go"),
buildsRepo: func(repo, branch, goBranch string) bool {
- return repo == "go" || (branch == "master" && goBranch == "master")
+ switch repo {
+ case "go":
+ return true
+ case "mobile", "benchmarks", "debug", "perf", "talks", "tools", "tour", "website":
+ return false
+ default:
+ return branch == "master" && goBranch == "master"
+ }
},
ShouldRunDistTest: func(distTest string, isTry bool) bool {
if isTry {
@@ -1712,10 +1742,9 @@
numTryTestHelpers: 5,
})
addBuilder(BuildConfig{
- Name: "windows-amd64-race",
- HostType: "host-windows-amd64-2008",
- Notes: "Only runs -race tests (./race.bat)",
- buildsRepo: onlyGo,
+ Name: "windows-amd64-race",
+ HostType: "host-windows-amd64-2008",
+ Notes: "Only runs -race tests (./race.bat)",
env: []string{
"GOARCH=amd64",
"GOHOSTARCH=amd64",
diff --git a/dashboard/builders_test.go b/dashboard/builders_test.go
index 9b43db6..b305f5d 100644
--- a/dashboard/builders_test.go
+++ b/dashboard/builders_test.go
@@ -147,6 +147,7 @@
"freebsd-amd64-12_0",
"linux-386",
"linux-amd64",
+ "linux-amd64-race",
"netbsd-amd64-8_0",
"openbsd-386-64",
"openbsd-amd64-64",
@@ -345,10 +346,39 @@
{b("nacl-amd64p32", "go"), both},
{b("nacl-amd64p32", "net"), none},
- // Only test tip for js/wasm:
+ // Only test tip for js/wasm, and only for some repos:
{b("js-wasm", "go"), both},
+ {b("js-wasm", "arch"), onlyPost},
+ {b("js-wasm", "crypto"), onlyPost},
+ {b("js-wasm", "sys"), onlyPost},
{b("js-wasm", "net"), onlyPost},
{b("js-wasm@go1.12", "net"), none},
+ {b("js-wasm", "benchmarks"), none},
+ {b("js-wasm", "debug"), none},
+ {b("js-wasm", "mobile"), none},
+ {b("js-wasm", "perf"), none},
+ {b("js-wasm", "talks"), none},
+ {b("js-wasm", "tools"), none},
+ {b("js-wasm", "tour"), none},
+ {b("js-wasm", "website"), none},
+
+ // Race builders. Linux for all, GCE buidlers for
+ // post-submit, and only post-submit for "go" for
+ // Darwin (limited resources).
+ {b("linux-amd64-race", "go"), both},
+ {b("linux-amd64-race", "net"), both},
+ {b("windows-amd64-race", "go"), onlyPost},
+ {b("windows-amd64-race", "net"), onlyPost},
+ {b("freebsd-amd64-race", "go"), onlyPost},
+ {b("freebsd-amd64-race", "net"), onlyPost},
+ {b("darwin-amd64-race", "go"), onlyPost},
+ {b("darwin-amd64-race", "net"), none},
+
+ // Long test.
+ {b("linux-amd64-longtest", "go"), onlyPost},
+ {b("linux-amd64-longtest", "net"), onlyPost},
+ {b("linux-amd64-longtest@go1.12", "go"), onlyPost},
+ {b("linux-amd64-longtest@go1.12", "net"), none},
}
for _, tt := range tests {
t.Run(tt.br.testName, func(t *testing.T) {
@@ -393,3 +423,12 @@
}
}
}
+
+// tests that goBranch is optional for repo == "go"
+func TestBuildsRepoAtAllImplicitGoBranch(t *testing.T) {
+ builder := Builders["android-amd64-emu"]
+ got := builder.buildsRepoAtAll("go", "master", "")
+ if !got {
+ t.Error("got = false; want true")
+ }
+}
diff --git a/internal/buildgo/buildgo.go b/internal/buildgo/buildgo.go
index 7b019b7..3456155 100644
--- a/internal/buildgo/buildgo.go
+++ b/internal/buildgo/buildgo.go
@@ -67,10 +67,15 @@
return buildEnv.SnapshotURL(br.Name, br.Rev)
}
+var TestHookSnapshotExists func(*BuilderRev) bool
+
// snapshotExists reports whether the snapshot exists in storage.
// It returns potentially false negatives on network errors.
// Callers must not depend on this as more than an optimization.
func (br *BuilderRev) SnapshotExists(ctx context.Context, buildEnv *buildenv.Environment) bool {
+ if f := TestHookSnapshotExists; f != nil {
+ return f(br)
+ }
req, err := http.NewRequest("HEAD", br.SnapshotURL(buildEnv), nil)
if err != nil {
panic(err)
