Forward -testfilter to nogo and fix failure in case of no srcs (#4075)

**What type of PR is this?**

Bug fix

**What does this PR do? Why is it needed?**

Without this, nogo runs on the external test sources when compiling the
internal test library, which can result in missing deps errors and is
also wasteful. Since filtering out files made certain nogo actions run
on no files, also fix a bug that affects this situation by writing out
an empty log in addition to an empty facts file.

The code that checks imports and builds the importcfg was shared between
`compilepkg` and `nogo` and is now extracted into a common method. Along
the way, have it output the file into the working directory, which
simplifies cleanup, makes the file easier to find and avoids writing
files unknown to Bazel into the output directory.

Also removes some unused test files.

**Which issues(s) does this PR fix?**

Fixes #4062
Fixes #4070
Fixes #4073

**Other notes for review**

go/private/actions/compilepkg.bzl

@@ -207,12 +207,13 @@ def emit_compilepkg(
         _run_nogo(
             go,
             sources = sources,
+            cgo_go_srcs = cgo_go_srcs_for_nogo,
             importpath = importpath,
             importmap = importmap,
             archives = archives + ([cover_archive] if cover_archive else []),
             recompile_internal_deps = recompile_internal_deps,
             cover_mode = cover_mode,
-            cgo_go_srcs = cgo_go_srcs_for_nogo,
+            testfilter = testfilter,
             out_facts = out_facts,
             out_log = out_nogo_log,
             out_validation = out_nogo_validation,
@@ -223,12 +224,13 @@ def _run_nogo(
         go,
         *,
         sources,
+        cgo_go_srcs,
         importpath,
         importmap,
         archives,
         recompile_internal_deps,
         cover_mode,
-        cgo_go_srcs,
+        testfilter,
         out_facts,
         out_log,
         out_validation,
@@ -256,7 +258,10 @@ def _run_nogo(
         args.add("-importpath", go.label.name)
     if importmap:
         args.add("-p", importmap)
+
     args.add("-package_list", go.package_list)
+    if testfilter:
+        args.add("-testfilter", testfilter)
 
     args.add_all(archives, before_each = "-facts", map_each = _facts)
     args.add("-out_facts", out_facts)

go/tools/builders/compilepkg.go

@@ -98,29 +98,9 @@ func compilePkg(args []string) error {
 		return err
 	}
 
-	// TODO(jayconrod): remove -testfilter flag. The test action should compile
-	// the main, internal, and external packages by calling compileArchive
-	// with the correct sources for each.
-	switch testFilter {
-	case "off":
-	case "only":
-		testSrcs := make([]fileInfo, 0, len(srcs.goSrcs))
-		for _, f := range srcs.goSrcs {
-			if strings.HasSuffix(f.pkg, "_test") {
-				testSrcs = append(testSrcs, f)
-			}
-		}
-		srcs.goSrcs = testSrcs
-	case "exclude":
-		libSrcs := make([]fileInfo, 0, len(srcs.goSrcs))
-		for _, f := range srcs.goSrcs {
-			if !strings.HasSuffix(f.pkg, "_test") {
-				libSrcs = append(libSrcs, f)
-			}
-		}
-		srcs.goSrcs = libSrcs
-	default:
-		return fmt.Errorf("invalid test filter %q", testFilter)
+	err = applyTestFilter(testFilter, &srcs)
+	if err != nil {
+		return err
 	}
 
 	return compileArchive(
@@ -351,44 +331,10 @@ func compileArchive(
 		gcFlags = append(gcFlags, createTrimPath(gcFlags, "."))
 	}
 
-	// Check that the filtered sources don't import anything outside of
-	// the standard library and the direct dependencies.
-	imports, err := checkImports(srcs.goSrcs, deps, packageListPath, importPath, recompileInternalDeps)
-	if err != nil {
-		return err
-	}
-	if compilingWithCgo {
-		// cgo generated code imports some extra packages.
-		imports["runtime/cgo"] = nil
-		imports["syscall"] = nil
-		imports["unsafe"] = nil
-	}
-	if coverMode != "" {
-		if coverMode == "atomic" {
-			imports["sync/atomic"] = nil
-		}
-		const coverdataPath = "github.com/bazelbuild/rules_go/go/tools/coverdata"
-		var coverdata *archive
-		for i := range deps {
-			if deps[i].importPath == coverdataPath {
-				coverdata = &deps[i]
-				break
-			}
-		}
-		if coverdata == nil {
-			return errors.New("coverage requested but coverdata dependency not provided")
-		}
-		imports[coverdataPath] = coverdata
-	}
-
-	// Build an importcfg file for the compiler.
-	importcfgPath, err := buildImportcfgFileForCompile(imports, goenv.installSuffix, filepath.Dir(outLinkObj))
+	importcfgPath, err := checkImportsAndBuildCfg(goenv, importPath, srcs, deps, packageListPath, recompileInternalDeps, compilingWithCgo, coverMode, workDir)
 	if err != nil {
 		return err
 	}
-	if !goenv.shouldPreserveWorkDir {
-		defer os.Remove(importcfgPath)
-	}
 
 	// Build an embedcfg file mapping embed patterns to filenames.
 	// Embed patterns are relative to any one of a list of root directories
@@ -490,6 +436,45 @@ func compileArchive(
 	return nil
 }
 
+func checkImportsAndBuildCfg(goenv *env, importPath string, srcs archiveSrcs, deps []archive, packageListPath string, recompileInternalDeps []string, compilingWithCgo bool, coverMode string, workDir string) (string, error) {
+	// Check that the filtered sources don't import anything outside of
+	// the standard library and the direct dependencies.
+	imports, err := checkImports(srcs.goSrcs, deps, packageListPath, importPath, recompileInternalDeps)
+	if err != nil {
+		return "", err
+	}
+	if compilingWithCgo {
+		// cgo generated code imports some extra packages.
+		imports["runtime/cgo"] = nil
+		imports["syscall"] = nil
+		imports["unsafe"] = nil
+	}
+	if coverMode != "" {
+		if coverMode == "atomic" {
+			imports["sync/atomic"] = nil
+		}
+		const coverdataPath = "github.com/bazelbuild/rules_go/go/tools/coverdata"
+		var coverdata *archive
+		for i := range deps {
+			if deps[i].importPath == coverdataPath {
+				coverdata = &deps[i]
+				break
+			}
+		}
+		if coverdata == nil {
+			return "", errors.New("coverage requested but coverdata dependency not provided")
+		}
+		imports[coverdataPath] = coverdata
+	}
+
+	// Build an importcfg file for the compiler.
+	importcfgPath, err := buildImportcfgFileForCompile(imports, goenv.installSuffix, workDir)
+	if err != nil {
+		return "", err
+	}
+	return importcfgPath, nil
+}
+
 func compileGo(goenv *env, srcs []string, packagePath, importcfgPath, embedcfgPath, asmHdrPath, symabisPath string, gcFlags []string, pgoprofile, outLinkobjPath, outInterfacePath string) error {
 	args := goenv.goTool("compile")
 	args = append(args, "-p", packagePath, "-importcfg", importcfgPath, "-pack")

go/tools/builders/filter.go

@@ -112,6 +112,36 @@ func filterAndSplitFiles(fileNames []string) (archiveSrcs, error) {
 	return res, nil
 }
 
+// applyTestFilter filters out test files from the list of sources in place
+// according to the filter.
+func applyTestFilter(testFilter string, srcs *archiveSrcs) error {
+	// TODO(jayconrod): remove -testfilter flag. The test action should compile
+	// the main, internal, and external packages by calling compileArchive
+	// with the correct sources for each.
+	switch testFilter {
+	case "off":
+	case "only":
+		testSrcs := make([]fileInfo, 0, len(srcs.goSrcs))
+		for _, f := range srcs.goSrcs {
+			if strings.HasSuffix(f.pkg, "_test") {
+				testSrcs = append(testSrcs, f)
+			}
+		}
+		srcs.goSrcs = testSrcs
+	case "exclude":
+		libSrcs := make([]fileInfo, 0, len(srcs.goSrcs))
+		for _, f := range srcs.goSrcs {
+			if !strings.HasSuffix(f.pkg, "_test") {
+				libSrcs = append(libSrcs, f)
+			}
+		}
+		srcs.goSrcs = libSrcs
+	default:
+		return fmt.Errorf("invalid test filter %q", testFilter)
+	}
+	return nil
+}
+
 // readFileInfo applies build constraints to an input file and returns whether
 // it should be compiled.
 func readFileInfo(bctx build.Context, input string) (fileInfo, error) {

go/tools/builders/nogo.go

@@ -23,6 +23,7 @@ func nogo(args []string) error {
 	var unfilteredSrcs, recompileInternalDeps multiFlag
 	var deps, facts archiveMultiFlag
 	var importPath, packagePath, nogoPath, packageListPath string
+	var testFilter string
 	var outFactsPath, outLogPath string
 	var coverMode string
 	fs.Var(&unfilteredSrcs, "src", ".go, .c, .cc, .m, .mm, .s, or .S file to be filtered and compiled")
@@ -33,6 +34,7 @@ func nogo(args []string) error {
 	fs.StringVar(&packageListPath, "package_list", "", "The file containing the list of standard library packages")
 	fs.Var(&recompileInternalDeps, "recompile_internal_deps", "The import path of the direct dependencies that needs to be recompiled.")
 	fs.StringVar(&coverMode, "cover_mode", "", "The coverage mode to use. Empty if coverage instrumentation should not be added.")
+	fs.StringVar(&testFilter, "testfilter", "off", "Controls test package filtering")
 	fs.StringVar(&nogoPath, "nogo", "", "The nogo binary")
 	fs.StringVar(&outFactsPath, "out_facts", "", "The file to emit serialized nogo facts to")
 	fs.StringVar(&outLogPath, "out_log", "", "The file to emit nogo logs into")
@@ -52,6 +54,11 @@ func nogo(args []string) error {
 		return err
 	}
 
+	err = applyTestFilter(testFilter, &srcs)
+	if err != nil {
+		return err
+	}
+
 	var goSrcs []string
 	haveCgo := false
 	for _, src := range srcs.goSrcs {
@@ -68,50 +75,28 @@ func nogo(args []string) error {
 	}
 	defer cleanup()
 
-	imports, err := checkImports(srcs.goSrcs, deps, packageListPath, importPath, recompileInternalDeps)
+	compilingWithCgo := os.Getenv("CGO_ENABLED") == "1" && haveCgo
+	importcfgPath, err := checkImportsAndBuildCfg(goenv, importPath, srcs, deps, packageListPath, recompileInternalDeps, compilingWithCgo, coverMode, workDir)
 	if err != nil {
 		return err
 	}
-	cgoEnabled := os.Getenv("CGO_ENABLED") == "1"
-	if haveCgo && cgoEnabled {
-		// cgo generated code imports some extra packages.
-		imports["runtime/cgo"] = nil
-		imports["syscall"] = nil
-		imports["unsafe"] = nil
-	}
-	if coverMode != "" {
-		if coverMode == "atomic" {
-			imports["sync/atomic"] = nil
-		}
-		const coverdataPath = "github.com/bazelbuild/rules_go/go/tools/coverdata"
-		var coverdata *archive
-		for i := range deps {
-			if deps[i].importPath == coverdataPath {
-				coverdata = &deps[i]
-				break
-			}
-		}
-		if coverdata == nil {
-			return errors.New("coverage requested but coverdata dependency not provided")
-		}
-		imports[coverdataPath] = coverdata
-	}
-
-	importcfgPath, err := buildImportcfgFileForCompile(imports, goenv.installSuffix, filepath.Dir(outFactsPath))
-	if err != nil {
-		return err
-	}
-	if !goenv.shouldPreserveWorkDir {
-		defer os.Remove(importcfgPath)
-	}
 
 	return runNogo(workDir, nogoPath, goSrcs, facts, importPath, importcfgPath, outFactsPath, outLogPath)
 }
 
 func runNogo(workDir string, nogoPath string, srcs []string, facts []archive, packagePath, importcfgPath, outFactsPath string, outLogPath string) error {
 	if len(srcs) == 0 {
 		// emit_compilepkg expects a nogo facts file, even if it's empty.
-		return os.WriteFile(outFactsPath, nil, 0o666)
+		// We also need to write the validation output log.
+		err := os.WriteFile(outFactsPath, nil, 0o666)
+		if err != nil {
+			return fmt.Errorf("error writing empty nogo facts file: %v", err)
+		}
+		err = os.WriteFile(outLogPath, nil, 0o666)
+		if err != nil {
+			return fmt.Errorf("error writing empty nogo log file: %v", err)
+		}
+		return nil
 	}
 	args := []string{nogoPath}
 	args = append(args, "-p", packagePath)

tests/core/nogo/tests/BUILD.bazel

@@ -0,0 +1,6 @@
+load("@io_bazel_rules_go//go/tools/bazel_testing:def.bzl", "go_bazel_test")
+
+go_bazel_test(
+    name = "tests_test",
+    srcs = ["tests_test.go"],
+)

tests/core/nogo/tests/README.rst

@@ -0,0 +1,13 @@
+nogo for go_test
+=============================
+
+.. _nogo: /go/nogo.rst
+.. _go_test: /docs/go/core/rules.md#_go_test
+
+Verifies interactions between `nogo`_ and `go_test`_.
+
+
+tests_test
+=============================
+
+Tests that `nogo`_ can handle various edge cases of external tests.