cmd/coordinator,dashboard: drop subrepo GOPATH testing support
All of the subrepos we test use modules, so GOPATH test mode here is
unused and can be deleted.
Fixes golang/go#48875
Change-Id: Id5143e9fcad7fc91e8e0f63dcca35f5661be5a03
Reviewed-on: https://go-review.googlesource.com/c/build/+/355089
Trust: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
diff --git a/cmd/coordinator/coordinator.go b/cmd/coordinator/coordinator.go
index 99ce1df..7563c5c 100644
--- a/cmd/coordinator/coordinator.go
+++ b/cmd/coordinator/coordinator.go
@@ -2821,23 +2821,6 @@
goroot := st.conf.FilePathJoin(workDir, "go")
gopath := st.conf.FilePathJoin(workDir, "gopath")
- // Check out the provided sub-repo to the buildlet's workspace.
- // Need to do this first, so we can run go env GOMOD in it.
- err = buildgo.FetchSubrepo(st.ctx, st, st.bc, st.SubName, st.SubRev, importPathOfRepo(st.SubName))
- if err != nil {
- return nil, err
- }
-
- // Determine if we're invoked in module mode.
- // If using module mode, the absolute path to the go.mod of the main module.
- // If using GOPATH mode, the empty string.
- goMod, err := st.goMod(importPathOfRepo(st.SubName), goroot, gopath)
- if err != nil {
- return nil, fmt.Errorf("failed to determine go env GOMOD value: %v", err)
- }
-
- // TODO(dmitshur): For some subrepos, test in both module and GOPATH modes. See golang.org/issue/30233.
-
// A goTestRun represents a single invocation of the 'go test' command.
type goTestRun struct {
Dir string // Directory where 'go test' should be executed.
@@ -2845,94 +2828,51 @@
}
// The default behavior is to test the pattern "golang.org/x/{repo}/..."
// in the repository root.
+ repoPath := importPathOfRepo(st.SubName)
testRuns := []goTestRun{{
- Dir: "gopath/src/" + importPathOfRepo(st.SubName),
- Patterns: []string{importPathOfRepo(st.SubName) + "/..."},
+ Dir: "gopath/src/" + repoPath,
+ Patterns: []string{repoPath + "/..."},
}}
- // The next large step diverges into two code paths,
- // one for module mode and another for GOPATH mode.
- //
- // Each path does the ad-hoc work that is needed to
- // prepare for being able to run 'go test' at the end.
- //
- if goMod != "" {
- fmt.Fprintf(st, "testing in module mode; GOMOD=%s\n\n", goMod)
+ // Check out the provided sub-repo to the buildlet's workspace so we
+ // can find go.mod files and run tests in it.
+ err = buildgo.FetchSubrepo(st.ctx, st, st.bc, st.SubName, st.SubRev, repoPath)
+ if err != nil {
+ return nil, err
+ }
- // No need to place the repo's dependencies into a GOPATH workspace.
-
- // Look for inner modules, in order to test them too. See golang.org/issue/32528.
- repoPath := importPathOfRepo(st.SubName)
- sp := st.CreateSpan("listing_subrepo_modules", st.SubName)
- err = st.bc.ListDir(st.ctx, "gopath/src/"+repoPath, buildlet.ListDirOpts{Recursive: true}, func(e buildlet.DirEntry) {
- goModFile := path.Base(e.Name()) == "go.mod" && !e.IsDir()
- if !goModFile {
- return
- }
- // Found a go.mod file in a subdirectory, which indicates the root of a module.
- modulePath := path.Join(repoPath, path.Dir(e.Name()))
- if modulePath == repoPath {
- // This is the go.mod file at the repository root.
- // It's already a part of testRuns, so skip it.
- return
- } else if ignoredByGoTool(modulePath) || isVendored(modulePath) {
- // go.mod file is in a directory we're not looking to support, so skip it.
- return
- }
- // Add an additional test run entry that will test this entire module.
- testRuns = append(testRuns, goTestRun{
- Dir: "gopath/src/" + modulePath,
- Patterns: []string{modulePath + "/..."},
- })
+ // Look for inner modules, in order to test them too. See golang.org/issue/32528.
+ sp := st.CreateSpan("listing_subrepo_modules", st.SubName)
+ err = st.bc.ListDir(st.ctx, "gopath/src/"+repoPath, buildlet.ListDirOpts{Recursive: true}, func(e buildlet.DirEntry) {
+ goModFile := path.Base(e.Name()) == "go.mod" && !e.IsDir()
+ if !goModFile {
+ return
+ }
+ // Found a go.mod file in a subdirectory, which indicates the root of a module.
+ modulePath := path.Join(repoPath, path.Dir(e.Name()))
+ if modulePath == repoPath {
+ // This is the go.mod file at the repository root.
+ // It's already a part of testRuns, so skip it.
+ return
+ } else if ignoredByGoTool(modulePath) || isVendored(modulePath) {
+ // go.mod file is in a directory we're not looking to support, so skip it.
+ return
+ }
+ // Add an additional test run entry that will test this entire module.
+ testRuns = append(testRuns, goTestRun{
+ Dir: "gopath/src/" + modulePath,
+ Patterns: []string{modulePath + "/..."},
})
- sp.Done(err)
- if err != nil {
- return nil, err
- }
-
- } else {
- fmt.Fprintf(st, "testing in GOPATH mode\n\n")
-
- // Place the repo's dependencies into the GOPATH workspace.
- if rErr, err := st.fetchDependenciesToGOPATHWorkspace(goroot, gopath); err != nil {
- return nil, err
- } else if rErr != nil {
- return rErr, nil
- }
-
- // The dashboard offers control over what packages to test in GOPATH mode.
- // Compute a list of packages by calling 'go list'. See golang.org/issue/34190.
- repoPath := importPathOfRepo(st.SubName)
- var buf bytes.Buffer
- sp := st.CreateSpan("listing_subrepo_packages", st.SubName)
- rErr, err := st.bc.Exec(st.ctx, "go/bin/go", buildlet.ExecOpts{
- Output: &buf,
- Dir: "gopath/src/" + repoPath,
- ExtraEnv: append(st.conf.Env(), "GOROOT="+goroot, "GOPATH="+gopath),
- Path: []string{"$WORKDIR/go/bin", "$PATH"},
- Args: []string{"list", repoPath + "/..."},
- })
- sp.Done(firstNonNil(err, rErr))
- if err != nil {
- return nil, fmt.Errorf("exec go list on buildlet: %v", err)
- }
- if rErr != nil {
- fmt.Fprintf(st, "go list error: %v\noutput:\n%s", rErr, &buf)
- return rErr, nil
- }
- testRuns[0].Patterns = nil
- for _, importPath := range strings.Fields(buf.String()) {
- if !st.conf.ShouldTestPackageInGOPATHMode(importPath) {
- continue
- }
- testRuns[0].Patterns = append(testRuns[0].Patterns, importPath)
- }
+ })
+ sp.Done(err)
+ if err != nil {
+ return nil, err
}
// Finally, execute all of the test runs.
// If any fail, keep going so that all test results are included in the output.
- sp := st.CreateSpan("running_subrepo_tests", st.SubName)
+ sp = st.CreateSpan("running_subrepo_tests", st.SubName)
defer func() { sp.Done(err) }()
env := append(st.conf.Env(),
@@ -2977,118 +2917,6 @@
return nil, nil
}
-// goMod determines and reports the value of go env GOMOD
-// for the given import path, GOROOT, and GOPATH values.
-// It uses module-specific environment variables from st.conf.ModulesEnv.
-func (st *buildStatus) goMod(importPath, goroot, gopath string) (string, error) {
- var buf bytes.Buffer
- rErr, err := st.bc.Exec(st.ctx, "go/bin/go", buildlet.ExecOpts{
- Output: &buf,
- Dir: "gopath/src/" + importPath,
- ExtraEnv: append(append(st.conf.Env(), "GOROOT="+goroot, "GOPATH="+gopath), st.conf.ModulesEnv(st.SubName)...),
- Path: []string{"$WORKDIR/go/bin", "$PATH"},
- Args: []string{"env", "-json", "GOMOD"},
- })
- if err != nil {
- return "", fmt.Errorf("exec go env on buildlet: %v", err)
- } else if rErr != nil {
- return "", fmt.Errorf("go env error: %v\noutput:\n%s", rErr, &buf)
- }
- var env struct {
- GoMod string
- }
- err = json.Unmarshal(buf.Bytes(), &env)
- return env.GoMod, err
-}
-
-// fetchRepoDependenciesToGOPATHWorkspace recursively fetches
-// the golang.org/x/* dependencies of repo st.SubName
-// and places them into the buildlet's GOPATH workspace.
-// The st.SubName repo itself must already be there.
-//
-// Dependencies are always fetched at master, which
-// isn't great but the dashboard data model doesn't
-// track non-golang.org/x/* dependencies. For those, we
-// require on the code under test to be using Go modules.
-func (st *buildStatus) fetchDependenciesToGOPATHWorkspace(goroot, gopath string) (remoteErr, err error) {
- fetched := map[string]bool{}
- toFetch := []string{st.SubName}
-
- // fetch checks out the provided sub-repo to the buildlet's workspace.
- fetch := func(repo, rev string) error {
- fetched[repo] = true
- return buildgo.FetchSubrepo(st.ctx, st, st.bc, repo, rev, importPathOfRepo(repo))
- }
-
- // findDeps uses 'go list' on the checked out repo to find its
- // dependencies, and adds any not-yet-fetched deps to toFetch.
- findDeps := func(repo string) (rErr, err error) {
- repoPath := importPathOfRepo(repo)
- var buf bytes.Buffer
- rErr, err = st.bc.Exec(st.ctx, "go/bin/go", buildlet.ExecOpts{
- Output: &buf,
- Dir: "gopath/src/" + repoPath,
- ExtraEnv: append(st.conf.Env(), "GOROOT="+goroot, "GOPATH="+gopath),
- Path: []string{"$WORKDIR/go/bin", "$PATH"},
- Args: []string{"list", "-f", `{{range .Deps}}{{printf "%v\n" .}}{{end}}`, repoPath + "/..."},
- })
- if err != nil {
- return nil, fmt.Errorf("exec go list on buildlet: %v", err)
- }
- if rErr != nil {
- fmt.Fprintf(st, "go list error: %v\noutput:\n%s", rErr, &buf)
- return rErr, nil
- }
- for _, p := range strings.Fields(buf.String()) {
- if !strings.HasPrefix(p, "golang.org/x/") ||
- p == repoPath || strings.HasPrefix(p, repoPath+"/") {
- continue
- }
- repo := strings.TrimPrefix(p, "golang.org/x/")
- if i := strings.Index(repo, "/"); i >= 0 {
- repo = repo[:i]
- }
- if !fetched[repo] {
- toFetch = append(toFetch, repo)
- }
- }
- return nil, nil
- }
-
- for i := 0; i < len(toFetch); i++ {
- repo := toFetch[i]
- if fetched[repo] {
- continue
- }
- // Fetch the HEAD revision by default.
- rev, err := getRepoHead(repo)
- if err != nil {
- return nil, err
- }
- if rev == "" {
- rev = "master" // should happen rarely; ok if it does.
- }
- if i == 0 {
- // The repo under test has already been fetched earlier,
- // just need to mark it as fetched.
- fetched[repo] = true
- } else {
- if err := fetch(repo, rev); err != nil {
- return nil, err
- }
- }
- if rErr, err := findDeps(repo); err != nil {
- return nil, err
- } else if rErr != nil {
- // An issue with the package may cause "go list" to
- // fail and this is a legimiate build error.
- return rErr, nil
- }
- }
-
- return nil, nil
-}
-
// ignoredByGoTool reports whether the given import path corresponds
// to a directory that would be ignored by the go tool.
//
@@ -3117,16 +2945,6 @@
strings.Contains(importPath, "/vendor/")
}
-// firstNonNil returns the first non-nil error, or nil otherwise.
-func firstNonNil(errs ...error) error {
- for _, e := range errs {
- if e != nil {
- return e
- }
- }
- return nil
-}
-
// multiError is a concatentation of multiple errors.
// There must be one or more errors, and all must be non-nil.
type multiError []error
diff --git a/dashboard/builders.go b/dashboard/builders.go
index cecb9a0..78b62a7 100644
--- a/dashboard/builders.go
+++ b/dashboard/builders.go
@@ -939,40 +939,6 @@
return env
}
-// ShouldTestPackageInGOPATHMode is used to control whether the package
-// with the specified import path should be tested in GOPATH mode.
-//
-// When running tests for all golang.org/* repositories in GOPATH mode,
-// this method is called repeatedly with the full import path of each
-// package that is found and is being considered for testing in GOPATH
-// mode. It's not used and has no effect on import paths in the main
-// "go" repository. It has no effect on tests done in module mode.
-//
-// When considering making changes here, keep the release policy in mind:
-//
-// https://golang.org/doc/devel/release.html#policy
-//
-func (*BuildConfig) ShouldTestPackageInGOPATHMode(importPath string) bool {
- if importPath == "golang.org/x/tools/gopls" ||
- strings.HasPrefix(importPath, "golang.org/x/tools/gopls/") {
- // Don't test golang.org/x/tools/gopls/... in GOPATH mode.
- return false
- }
- if importPath == "golang.org/x/net/http2/h2demo" {
- // Don't test golang.org/x/net/http2/h2demo in GOPATH mode.
- //
- // It was never tested before golang.org/issue/34361 because it
- // had a +build h2demo constraint. But that build constraint is
- // being removed, so explicitly skip testing it in GOPATH mode.
- //
- // The package is supported only in module mode now, since
- // it requires third-party dependencies.
- return false
- }
- // Test everything else in GOPATH mode as usual.
- return true
-}
-
func (c *BuildConfig) IsReverse() bool { return c.HostConfig().IsReverse }
func (c *BuildConfig) IsContainer() bool { return c.HostConfig().IsContainer() }
diff --git a/dashboard/builders_test.go b/dashboard/builders_test.go
index 24a21f5..1636ca2 100644
--- a/dashboard/builders_test.go
+++ b/dashboard/builders_test.go
@@ -733,33 +733,6 @@
}
}
-func TestShouldTestPackageInGOPATHMode(t *testing.T) {
- // This function doesn't change behavior depending on the builder
- // at this time, so just use a common one.
- bc, ok := Builders["linux-amd64"]
- if !ok {
- t.Fatal("unknown builder")
- }
-
- tests := []struct {
- importPath string
- want bool
- }{
- {"golang.org/x/image/bmp", true},
- {"golang.org/x/tools/go/ast/astutil", true},
- {"golang.org/x/tools/go/packages", true},
- {"golang.org/x/tools", true}, // Three isn't a package there, but if there was, it should be tested.
- {"golang.org/x/tools/gopls", false},
- {"golang.org/x/tools/gopls/internal/foobar", false},
- }
- for _, tt := range tests {
- got := bc.ShouldTestPackageInGOPATHMode(tt.importPath)
- if got != tt.want {
- t.Errorf("ShouldTestPackageInGOPATHMode(%q) = %v; want %v", tt.importPath, got, tt.want)
- }
- }
-}
-
func TestSlowBotAliases(t *testing.T) {
for term, name := range slowBotAliases {
if name == "" {
