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>

cmd/coordinator/coordinator.go

@@ -798,7 +798,6 @@ func workaroundFlush(w http.ResponseWriter) {
 
 // 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 @@ func findWork(work chan<- buildgo.BuilderRev) error {
 			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 @@ func findWork(work chan<- buildgo.BuilderRev) error {
 			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 @@ func (st *buildStatus) runSubrepoTests() (remoteErr, err error) {
 		"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,
 	})
 }
 

cmd/coordinator/coordinator_test.go

@@ -8,6 +8,7 @@ package main
 
 import (
 	"io/ioutil"
+	"log"
 	"net/http"
 	"net/http/httptest"
 	"reflect"
@@ -15,6 +16,7 @@ import (
 	"testing"
 	"time"
 
+	"golang.org/x/build/buildenv"
 	"golang.org/x/build/internal/buildgo"
 	"golang.org/x/build/maintner/maintnerd/apipb"
 )
@@ -169,3 +171,30 @@ func TestNewTrySetBuildRepoGo110(t *testing.T) {
 		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)
+	}
+}

dashboard/builders.go

@@ -652,7 +652,7 @@ type BuildConfig struct {
 	// 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 @@ func (c *BuildConfig) IsRace() bool {
 	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 @@ func (c *BuildConfig) BuildsRepoTryBot(repo, branch, goBranch string) bool {
 // 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 @@ func init() {
 		Name:              "linux-amd64-race",
 		HostType:          "host-linux-jessie",
 		tryBot:            defaultTrySet(),
-		buildsRepo:        onlyGo,
 		MaxAtOnce:         1,
 		ShouldRunDistTest: fasterTrybots,
 		numTestHelpers:    1,
@@ -1452,11 +1473,13 @@ func init() {
 		},
 	})
 	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 @@ func init() {
 		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 @@ func init() {
 		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",

dashboard/builders_test.go

@@ -147,6 +147,7 @@ func TestTrybots(t *testing.T) {
 				"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 @@ func TestBuilderConfig(t *testing.T) {
 		{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 @@ func TestHostConfigsAllUsed(t *testing.T) {
 		}
 	}
 }
+
+// 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")
+	}
+}

internal/buildgo/buildgo.go

@@ -67,10 +67,15 @@ func (br *BuilderRev) SnapshotURL(buildEnv *buildenv.Environment) string {
 	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)