From 110d25c1fcaa2649a4fd29f8dd1a7fd198e067c7 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 30 Aug 2024 00:24:49 +0200 Subject: [PATCH] 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 | 9 ++- go/tools/builders/compilepkg.go | 101 ++++++++++++---------------- go/tools/builders/filter.go | 30 +++++++++ go/tools/builders/nogo.go | 53 ++++++--------- tests/core/nogo/BUILD.bazel | 5 -- tests/core/nogo/common.bzl | 38 ----------- tests/core/nogo/tests/BUILD.bazel | 6 ++ tests/core/nogo/tests/README.rst | 13 ++++ tests/core/nogo/tests/tests_test.go | 95 ++++++++++++++++++++++++++ 9 files changed, 213 insertions(+), 137 deletions(-) delete mode 100644 tests/core/nogo/BUILD.bazel delete mode 100644 tests/core/nogo/common.bzl create mode 100644 tests/core/nogo/tests/BUILD.bazel create mode 100644 tests/core/nogo/tests/README.rst create mode 100644 tests/core/nogo/tests/tests_test.go diff --git a/go/private/actions/compilepkg.bzl b/go/private/actions/compilepkg.bzl index cfb7aee697..43872cf3c6 100644 --- a/go/private/actions/compilepkg.bzl +++ b/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) diff --git a/go/tools/builders/compilepkg.go b/go/tools/builders/compilepkg.go index 9e941c77b8..354f86a5d6 100644 --- a/go/tools/builders/compilepkg.go +++ b/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") diff --git a/go/tools/builders/filter.go b/go/tools/builders/filter.go index be4c8accb3..36cff41428 100644 --- a/go/tools/builders/filter.go +++ b/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) { diff --git a/go/tools/builders/nogo.go b/go/tools/builders/nogo.go index c49e8bb125..9747942f89 100644 --- a/go/tools/builders/nogo.go +++ b/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,42 +75,11 @@ 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) } @@ -111,7 +87,16 @@ func nogo(args []string) error { 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) diff --git a/tests/core/nogo/BUILD.bazel b/tests/core/nogo/BUILD.bazel deleted file mode 100644 index 46c7ee89e2..0000000000 --- a/tests/core/nogo/BUILD.bazel +++ /dev/null @@ -1,5 +0,0 @@ -filegroup( - name = "rules_go_deps", - srcs = [":common.bzl"], - visibility = ["//visibility:public"], -) diff --git a/tests/core/nogo/common.bzl b/tests/core/nogo/common.bzl deleted file mode 100644 index 14089ce2fe..0000000000 --- a/tests/core/nogo/common.bzl +++ /dev/null @@ -1,38 +0,0 @@ -# Macros used by all nogo integration tests. - -BUILD_FAILED_TMPL = """ -if [[ result -eq 0 ]]; then - echo "TEST FAILED: expected build error" >&2 - result=1 -else - result=0 - {check_err} -fi -""" - -BUILD_PASSED_TMPL = """ -if [[ result -ne 0 ]]; then - echo "TEST FAILED: unexpected build error" >&2 - result=1 -else - {check_err} -fi -""" - -CONTAINS_ERR_TMPL = """ - lines=$(grep '{err}' bazel-output.txt | wc -l) - if [ $lines -eq 0 ]; then - echo "TEST FAILED: expected error message containing: '{err}'" >&2 - result=1 - elif [ $lines -ne 1 ]; then - echo "TEST FAILED: expected error message '{err}' appears more than once" >&2 - result=1 - fi -""" - -DOES_NOT_CONTAIN_ERR_TMPL = """ - if grep -q '{err}' bazel-output.txt; then - echo "TEST FAILED: received error message containing: '{err}'" >&2 - result=1 - fi -""" diff --git a/tests/core/nogo/tests/BUILD.bazel b/tests/core/nogo/tests/BUILD.bazel new file mode 100644 index 0000000000..b96835c488 --- /dev/null +++ b/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"], +) diff --git a/tests/core/nogo/tests/README.rst b/tests/core/nogo/tests/README.rst new file mode 100644 index 0000000000..c2e43550fb --- /dev/null +++ b/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. diff --git a/tests/core/nogo/tests/tests_test.go b/tests/core/nogo/tests/tests_test.go new file mode 100644 index 0000000000..ed19dd41e2 --- /dev/null +++ b/tests/core/nogo/tests/tests_test.go @@ -0,0 +1,95 @@ +// Copyright 2019 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package importpath_test + +import ( + "testing" + + "github.com/bazelbuild/rules_go/go/tools/bazel_testing" +) + +func TestMain(m *testing.M) { + bazel_testing.TestMain(m, bazel_testing.Args{ + Main: ` +-- BUILD.bazel -- +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test", "nogo") + +go_library( + name = "simple_lib", + srcs = ["simple_lib.go"], + importpath = "example.com/simple", +) + +go_test( + name = "simple_test", + size = "small", + srcs = ["simple_test.go"], + embed = [":simple_lib"], +) + +go_test( + name = "super_simple_test", + size = "small", + srcs = ["super_simple_test.go"], +) + +nogo( + name = "nogo", + vet = True, + visibility = ["//visibility:public"], +) +-- simple_lib.go -- +package simple + +func Foo() {} +-- simple_test.go -- +package simple_test + +import ( + "testing" + + "example.com/simple" +) + +func TestFoo(t *testing.T) { + simple.Foo() +} +-- super_simple_test.go -- +package super_simple_test + +import ( + "testing" +) + +func TestFoo(t *testing.T) { +} +`, + Nogo: `@//:nogo`, + }) +} + +func TestExternalTestWithFullImportpath(t *testing.T) { + if out, err := bazel_testing.BazelOutput("test", "//:all"); err != nil { + println(string(out)) + t.Fatal(err) + } +} + +func TestEmptyExternalTest(t *testing.T) { + if out, err := bazel_testing.BazelOutput("test", "//:all"); err != nil { + println(string(out)) + t.Fatal(err) + } +}