Skip to content

Commit

Permalink
Forward -testfilter to nogo and fix failure in case of no srcs (#4075)
Browse files Browse the repository at this point in the history
**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**
  • Loading branch information
fmeum authored Aug 29, 2024
1 parent 8292592 commit 6f5f6f3
Show file tree
Hide file tree
Showing 9 changed files with 212 additions and 137 deletions.
8 changes: 6 additions & 2 deletions go/private/actions/compilepkg.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -211,12 +211,13 @@ def emit_compilepkg(
_run_nogo(
go,
sources = sources,
cgo_go_srcs = cgo_go_srcs_for_nogo,
importpath = importpath,
importmap = importmap,
archives = archives,
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,
Expand All @@ -227,12 +228,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,
Expand Down Expand Up @@ -263,6 +265,8 @@ def _run_nogo(
if importmap:
args.add("-p", importmap)
args.add("-package_list", sdk.package_list)
if testfilter:
args.add("-testfilter", testfilter)

args.add_all(archives, before_each = "-facts", map_each = _facts)
args.add("-out_facts", out_facts)
Expand Down
101 changes: 43 additions & 58 deletions go/tools/builders/compilepkg.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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")
Expand Down
30 changes: 30 additions & 0 deletions go/tools/builders/filter.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
53 changes: 19 additions & 34 deletions go/tools/builders/nogo.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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")
Expand All @@ -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 {
Expand All @@ -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)
Expand Down
5 changes: 0 additions & 5 deletions tests/core/nogo/BUILD.bazel

This file was deleted.

38 changes: 0 additions & 38 deletions tests/core/nogo/common.bzl

This file was deleted.

6 changes: 6 additions & 0 deletions tests/core/nogo/tests/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -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"],
)
13 changes: 13 additions & 0 deletions tests/core/nogo/tests/README.rst
Original file line number Diff line number Diff line change
@@ -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.
Loading

0 comments on commit 6f5f6f3

Please sign in to comment.