From 53ff4b9bd0e2dac9c924f748493c0066a997948c Mon Sep 17 00:00:00 2001 From: Alessandro Patti Date: Wed, 7 Aug 2024 13:34:42 +0200 Subject: [PATCH 01/15] Lazy expand absolute path to avoid including them in the output (#4009) **What type of PR is this?** Bug fix **What does this PR do? Why is it needed?** It fixes reproducibility issues reported in https://github.com/bazelbuild/rules_go/issues/3994 by lazy-expanding paths to absolute paths **Which issues(s) does this PR fix?** Fixes #3994 --- go/tools/builders/BUILD.bazel | 1 + go/tools/builders/builder.go | 2 + go/tools/builders/cc.go | 47 +++++++ go/tools/builders/env.go | 39 +++++- go/tools/builders/stdlib.go | 4 +- go/tools/builders/stdliblist.go | 4 +- .../reproducibility/reproducibility_test.go | 131 ++++++++++++------ 7 files changed, 179 insertions(+), 49 deletions(-) create mode 100644 go/tools/builders/cc.go diff --git a/go/tools/builders/BUILD.bazel b/go/tools/builders/BUILD.bazel index 244513000a..631dac1590 100644 --- a/go/tools/builders/BUILD.bazel +++ b/go/tools/builders/BUILD.bazel @@ -63,6 +63,7 @@ filegroup( "ar.go", "asm.go", "builder.go", + "cc.go", "cgo2.go", "compilepkg.go", "constants.go", diff --git a/go/tools/builders/builder.go b/go/tools/builders/builder.go index f9b96b8b8e..fd0a5ba6af 100644 --- a/go/tools/builders/builder.go +++ b/go/tools/builders/builder.go @@ -54,6 +54,8 @@ func main() { action = stdlib case "stdliblist": action = stdliblist + case "cc": + action = cc default: log.Fatalf("unknown action: %s", verb) } diff --git a/go/tools/builders/cc.go b/go/tools/builders/cc.go new file mode 100644 index 0000000000..90f9258b9c --- /dev/null +++ b/go/tools/builders/cc.go @@ -0,0 +1,47 @@ +package main + +import ( + "errors" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "syscall" +) + +func cc(args []string) error { + cc := os.Getenv("GO_CC") + if cc == "" { + errors.New("GO_CC environment variable not set") + } + ccroot := os.Getenv("GO_CC_ROOT") + if ccroot == "" { + errors.New("GO_CC_ROOT environment variable not set") + } + normalized := []string{cc} + normalized = append(normalized, args...) + transformArgs(normalized, cgoAbsEnvFlags, func(s string) string { + if strings.HasPrefix(s, cgoAbsPlaceholder) { + trimmed := strings.TrimPrefix(s, cgoAbsPlaceholder) + abspath := filepath.Join(ccroot, trimmed) + if _, err := os.Stat(abspath); err == nil { + // Only return the abspath if it exists, otherwise it + // means that either it won't have any effect or the original + // value was not a relpath (e.g. a path with a XCODE placehold from + // macos cc_wrapper) + return abspath + } + return trimmed + } + return s + }) + if runtime.GOOS == "windows" { + cmd := exec.Command(normalized[0], normalized[1:]...) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + return cmd.Run() + } else { + return syscall.Exec(normalized[0], normalized, os.Environ()) + } +} diff --git a/go/tools/builders/env.go b/go/tools/builders/env.go index cc8b37d721..bb01ac205f 100644 --- a/go/tools/builders/env.go +++ b/go/tools/builders/env.go @@ -35,6 +35,8 @@ var ( cgoEnvVars = []string{"CGO_CFLAGS", "CGO_CXXFLAGS", "CGO_CPPFLAGS", "CGO_LDFLAGS"} // cgoAbsEnvFlags are all the flags that need absolute path in cgoEnvVars cgoAbsEnvFlags = []string{"-I", "-L", "-isysroot", "-isystem", "-iquote", "-include", "-gcc-toolchain", "--sysroot", "-resource-dir", "-fsanitize-blacklist", "-fsanitize-ignorelist"} + // cgoAbsPlaceholder is placed in front of flag values that must be absolutized + cgoAbsPlaceholder = "__GO_BAZEL_CC_PLACEHOLDER__" ) // env holds a small amount of Go environment and toolchain information @@ -160,10 +162,33 @@ func (e *env) runCommandToFile(out, err io.Writer, args []string) error { return runAndLogCommand(cmd, e.verbose) } -func absEnv(envNameList []string, argList []string) error { +// absCCCompiler modifies CGO flags to workaround relative paths. +// Because go is having its own sandbox, all CGO flags should use +// absolute paths. However, CGO flags are embedded in the output +// so we cannot use absolute paths directly. Instead, use a placeholder +// for the absolute path and we replace CC with this builder so that +// we can expand the placeholder later. +func absCCCompiler(envNameList []string, argList []string) error { + err := os.Setenv("GO_CC", os.Getenv("CC")) + if err != nil { + return err + } + err = os.Setenv("GO_CC_ROOT", abs(".")) + if err != nil { + return err + } + err = os.Setenv("CC", abs(os.Args[0])+" cc") + if err != nil { + return err + } for _, envName := range envNameList { splitedEnv := strings.Fields(os.Getenv(envName)) - absArgs(splitedEnv, argList) + transformArgs(splitedEnv, argList, func(s string) string { + if filepath.IsAbs(s) { + return s + } + return cgoAbsPlaceholder + s + }) if err := os.Setenv(envName, strings.Join(splitedEnv, " ")); err != nil { return err } @@ -320,10 +345,16 @@ func abs(path string) string { // absArgs applies abs to strings that appear in args. Only paths that are // part of options named by flags are modified. func absArgs(args []string, flags []string) { + transformArgs(args, flags, abs) +} + +// transformArgs applies fn to strings that appear in args. Only paths that are +// part of options named by flags are modified. +func transformArgs(args []string, flags []string, fn func(string) string) { absNext := false for i := range args { if absNext { - args[i] = abs(args[i]) + args[i] = fn(args[i]) absNext = false continue } @@ -341,7 +372,7 @@ func absArgs(args []string, flags []string) { possibleValue = possibleValue[1:] separator = "=" } - args[i] = fmt.Sprintf("%s%s%s", f, separator, abs(possibleValue)) + args[i] = fmt.Sprintf("%s%s%s", f, separator, fn(possibleValue)) break } } diff --git a/go/tools/builders/stdlib.go b/go/tools/builders/stdlib.go index 15d52bb012..105ca5c635 100644 --- a/go/tools/builders/stdlib.go +++ b/go/tools/builders/stdlib.go @@ -158,9 +158,7 @@ You may need to use the flags --cpu=x64_windows --compiler=mingw-gcc.`) installArgs = append(installArgs, "-ldflags="+allSlug+strings.Join(ldflags, " ")) installArgs = append(installArgs, "-asmflags="+allSlug+strings.Join(asmflags, " ")) - // Modify CGO flags to use only absolute path - // because go is having its own sandbox, all CGO flags must use absolute path - if err := absEnv(cgoEnvVars, cgoAbsEnvFlags); err != nil { + if err := absCCCompiler(cgoEnvVars, cgoAbsEnvFlags); err != nil { return fmt.Errorf("error modifying cgo environment to absolute path: %v", err) } diff --git a/go/tools/builders/stdliblist.go b/go/tools/builders/stdliblist.go index 098be0408c..2648d2486d 100644 --- a/go/tools/builders/stdliblist.go +++ b/go/tools/builders/stdliblist.go @@ -257,9 +257,7 @@ func stdliblist(args []string) error { } os.Setenv("CC", quotePathIfNeeded(abs(ccEnv))) - // Modify CGO flags to use only absolute path - // because go is having its own sandbox, all CGO flags must use absolute path - if err := absEnv(cgoEnvVars, cgoAbsEnvFlags); err != nil { + if err := absCCCompiler(cgoEnvVars, cgoAbsEnvFlags); err != nil { return fmt.Errorf("error modifying cgo environment to absolute path: %v", err) } diff --git a/tests/integration/reproducibility/reproducibility_test.go b/tests/integration/reproducibility/reproducibility_test.go index 2e9bc59144..0feeee4714 100644 --- a/tests/integration/reproducibility/reproducibility_test.go +++ b/tests/integration/reproducibility/reproducibility_test.go @@ -189,6 +189,8 @@ func Test(t *testing.T) { defer wg.Done() cmd := bazel_testing.BazelCmd("build", "//:all", + "--copt=-Irelative/path/does/not/exist", + "--linkopt=-Lrelative/path/does/not/exist", "@io_bazel_rules_go//go/tools/builders:go_path", "@go_sdk//:builder", ) @@ -200,49 +202,92 @@ func Test(t *testing.T) { } wg.Wait() - // Hash files in each bazel-bin directory. - dirHashes := make([][]fileHash, len(dirs)) - errs := make([]error, len(dirs)) - wg.Add(len(dirs)) - for i := range dirs { - go func(i int) { - defer wg.Done() - dirHashes[i], errs[i] = hashFiles(filepath.Join(dirs[i], "bazel-bin")) - }(i) - } - wg.Wait() - for _, err := range errs { - if err != nil { + t.Run("Check Hashes", func(t *testing.T) { + // Hash files in each bazel-bin directory. + dirHashes := make([][]fileHash, len(dirs)) + errs := make([]error, len(dirs)) + wg.Add(len(dirs)) + for i := range dirs { + go func(i int) { + defer wg.Done() + dirHashes[i], errs[i] = hashFiles(filepath.Join(dirs[i], "bazel-out/"), func(root, path string) bool { + // Hash everything but actions stdout/stderr and the volatile-status.txt file + // as they are expected to change + return strings.HasPrefix(path, filepath.Join(root, "_tmp")) || + path == filepath.Join(root, "volatile-status.txt") + }) + }(i) + } + wg.Wait() + for _, err := range errs { + if err != nil { + t.Fatal(err) + } + } + + // Compare dir0 and dir1. They should be identical. + if err := compareHashes(dirHashes[0], dirHashes[1]); err != nil { t.Fatal(err) } - } - // Compare dir0 and dir1. They should be identical. - if err := compareHashes(dirHashes[0], dirHashes[1]); err != nil { - t.Fatal(err) - } + // Compare dir0 and dir2. They should be different. + if err := compareHashes(dirHashes[0], dirHashes[2]); err == nil { + t.Fatalf("dir0 and dir2 are the same)", len(dirHashes[0])) + } + }) - // Compare dir0 and dir2. They should be different. - if err := compareHashes(dirHashes[0], dirHashes[2]); err == nil { - t.Fatalf("dir0 and dir2 are the same)", len(dirHashes[0])) - } + t.Run("Check builder", func(t *testing.T) { + // Check that the go_sdk path doesn't appear in the builder binary. This path is different + // nominally different per workspace (but in these tests, the go_sdk paths are all set to the same + // path in WORKSPACE) -- so if this path is in the builder binary, then builds between workspaces + // would be partially non cacheable. + builder_file, err := os.Open(filepath.Join(dirs[0], "bazel-bin", "external", "go_sdk", "builder")) + if err != nil { + t.Fatal(err) + } + defer builder_file.Close() + builder_data, err := ioutil.ReadAll(builder_file) + if err != nil { + t.Fatal(err) + } + if bytes.Index(builder_data, []byte("go_sdk")) != -1 { + t.Fatalf("Found go_sdk path in builder binary, builder tool won't be reproducible") + } + }) - // Check that the go_sdk path doesn't appear in the builder binary. This path is different - // nominally different per workspace (but in these tests, the go_sdk paths are all set to the same - // path in WORKSPACE) -- so if this path is in the builder binary, then builds between workspaces - // would be partially non cacheable. - builder_file, err := os.Open(filepath.Join(dirs[0], "bazel-bin", "external", "go_sdk", "builder")) - if err != nil { - t.Fatal(err) - } - defer builder_file.Close() - builder_data, err := ioutil.ReadAll(builder_file) - if err != nil { - t.Fatal(err) - } - if bytes.Index(builder_data, []byte("go_sdk")) != -1 { - t.Fatalf("Found go_sdk path in builder binary, builder tool won't be reproducible") - } + t.Run("Check stdlib", func(t *testing.T) { + for _, dir := range dirs { + found := false + root, err := os.Readlink(filepath.Join(dir, "bazel-out/")) + if err != nil { + t.Fatal(err) + } + + filepath.Walk(root, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if !strings.Contains(path, "stdlib_/pkg") { + return nil + } + if !strings.HasSuffix(path, ".a") { + return nil + } + data, err := ioutil.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if bytes.Index(data, []byte("__GO_BAZEL_CC_PLACEHOLDER__")) == -1 { + found = true + return filepath.SkipAll + } + return nil + }) + if !found { + t.Fatal("Placeholder not found in stdlib of " + dir) + } + } + }) } func copyTree(dstRoot, srcRoot string) error { @@ -311,7 +356,7 @@ type fileHash struct { rel, hash string } -func hashFiles(dir string) ([]fileHash, error) { +func hashFiles(dir string, exclude func(root, path string) bool) ([]fileHash, error) { // Follow top-level symbolic link root := dir for { @@ -348,7 +393,15 @@ func hashFiles(dir string) ([]fileHash, error) { return err } } + if info.IsDir() { + if exclude(root, path) { + return filepath.SkipDir + } + return nil + } + + if exclude(root, path) { return nil } From 0dd58b3c1d480c8e26b4d905261f361b3f97f8b7 Mon Sep 17 00:00:00 2001 From: David Zbarsky Date: Sun, 11 Aug 2024 16:33:33 -0400 Subject: [PATCH 02/15] Remove get_nogo from context (#4029) **What type of PR is this?** Cleanup **What does this PR do? Why is it needed?** It seems odd to hang helpers off the `go` context because they still need to take `go` as a param. It's clearer to just import them. **Which issues(s) does this PR fix?** Fixes # **Other notes for review** --- go/private/actions/archive.bzl | 14 ++++++++++---- go/private/context.bzl | 5 ++--- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/go/private/actions/archive.bzl b/go/private/actions/archive.bzl index e2da3fbe2d..1bbd135d73 100644 --- a/go/private/actions/archive.bzl +++ b/go/private/actions/archive.bzl @@ -17,6 +17,10 @@ load( "as_tuple", "split_srcs", ) +load( + "//go/private:context.bzl", + "get_nogo", +) load( "//go/private:mode.bzl", "LINKMODE_C_ARCHIVE", @@ -61,14 +65,16 @@ def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_d # store export information for compiling dependent packages separately out_export = go.declare_file(go, name = source.library.name, ext = pre_ext + ".x") out_cgo_export_h = None # set if cgo used in c-shared or c-archive mode - out_facts = None - out_nogo_log = None - out_nogo_validation = None - nogo = go.get_nogo(go) + + nogo = get_nogo(go) if nogo: out_facts = go.declare_file(go, name = source.library.name, ext = pre_ext + ".facts") out_nogo_log = go.declare_file(go, name = source.library.name, ext = pre_ext + ".nogo.log") out_nogo_validation = go.declare_file(go, name = source.library.name, ext = pre_ext + ".nogo") + else: + out_facts = None + out_nogo_log = None + out_nogo_validation = None direct = [get_archive(dep) for dep in source.deps] runfiles = source.runfiles diff --git a/go/private/context.bzl b/go/private/context.bzl index c2c02effff..247c4a3443 100644 --- a/go/private/context.bzl +++ b/go/private/context.bzl @@ -435,9 +435,9 @@ def _matches_scopes(label, scopes): return True return False -def _get_nogo(go): +def get_nogo(go): """Returns the nogo file for this target, if enabled and in scope.""" - label = go._ctx.label + label = go.label if _matches_scopes(label, NOGO_INCLUDES) and not _matches_scopes(label, NOGO_EXCLUDES): return go.nogo else: @@ -615,7 +615,6 @@ def go_context(ctx, attr = None): library_to_source = _library_to_source, declare_file = _declare_file, declare_directory = _declare_directory, - get_nogo = _get_nogo, # Private # TODO: All uses of this should be removed From c5299a8c248453fc3ffab0430c21a616bbdab74f Mon Sep 17 00:00:00 2001 From: David Zbarsky Date: Mon, 12 Aug 2024 23:57:10 -0400 Subject: [PATCH 03/15] Tweak nogo archive handling (#4023) This is one of the sources of feeding structs into `deps`. **What type of PR is this?** Starlark cleanup **What does this PR do? Why is it needed?** **Which issues(s) does this PR fix?** Fixes # **Other notes for review** --- go/private/rules/nogo.bzl | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/go/private/rules/nogo.bzl b/go/private/rules/nogo.bzl index 9d6b1f9806..7aa63cfa53 100644 --- a/go/private/rules/nogo.bzl +++ b/go/private/rules/nogo.bzl @@ -25,7 +25,6 @@ load( "EXPORT_PATH", "GoArchive", "GoLibrary", - "get_archive", ) load( "//go/private/rules:transition.bzl", @@ -47,7 +46,7 @@ def _nogo_impl(ctx): if ctx.attr.debug: nogo_args.add("-debug") nogo_inputs = [] - analyzer_archives = [get_archive(dep) for dep in ctx.attr.deps] + analyzer_archives = [dep[GoArchive] for dep in ctx.attr.deps] analyzer_importpaths = [archive.data.importpath for archive in analyzer_archives] nogo_args.add_all(analyzer_importpaths, before_each = "-analyzer_importpath") if ctx.file.config: From eb9a7af825d95b0dd2fc2b07ce91665a8882c797 Mon Sep 17 00:00:00 2001 From: Tyler French Date: Tue, 13 Aug 2024 14:06:22 +0200 Subject: [PATCH 04/15] Add qnx and osx platforms (#4036) **What type of PR is this?** Bug fix **What does this PR do? Why is it needed?** **Which issues(s) does this PR fix?** **Other notes for review** This change was requested in https://github.com/bazelbuild/bazel-gazelle/issues/1735#issuecomment-2285850060 --- go/private/platforms.bzl | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/go/private/platforms.bzl b/go/private/platforms.bzl index ee6a1c767e..761969ef8a 100644 --- a/go/private/platforms.bzl +++ b/go/private/platforms.bzl @@ -22,6 +22,7 @@ BAZEL_GOOS_CONSTRAINTS = { "freebsd": "@platforms//os:freebsd", "ios": "@platforms//os:ios", "linux": "@platforms//os:linux", + "qnx": "@platforms//os:qnx", "windows": "@platforms//os:windows", } @@ -77,6 +78,14 @@ GOOS_GOARCH = ( ("openbsd", "amd64"), ("openbsd", "arm"), ("openbsd", "arm64"), + ("osx", "386"), + ("osx", "amd64"), + ("osx", "arm"), + ("osx", "arm64"), + ("qnx", "386"), + ("qnx", "amd64"), + ("qnx", "arm"), + ("qnx", "arm64"), ("plan9", "386"), ("plan9", "amd64"), ("plan9", "arm"), From 9599402c9ce92696998e87ec9d98320ad0d2a4d6 Mon Sep 17 00:00:00 2001 From: Walter Cacau Date: Sun, 18 Aug 2024 08:23:07 -0700 Subject: [PATCH 05/15] Making gopackagesdriver correctly handle relative queries made from a subdirectory. (#4002) **What type of PR is this?** Bug fix **What does this PR do? Why is it needed?** It allows gopackagesdriver to be called within a subdirectory of a workspace and properly resolve queries from gopls from IDEs like VSCode when they include relative references. For example, if you have the following repo/workspace structure: ``` WORKSPACE project1/BUILD.bazel project1/main.go project2/BUILD.bazel project2/main.go ... ``` If you run gopackagesdriver using bazel run from project1 directory and receive the following queries: ``` file=./main.go ./... ``` The existing code will try to resolve those queries against the root of the workspace, when instead we would expect it to resolve relative to the project1 directory. **Which issues(s) does this PR fix?** Fixes #4001 **Other notes for review** --- go/tools/gopackagesdriver/bazel.go | 24 ++-- .../gopackagesdriver/bazel_json_builder.go | 37 +++--- .../gopackagesdriver/gopackagesdriver_test.go | 107 +++++++++++++++++- go/tools/gopackagesdriver/main.go | 3 +- 4 files changed, 140 insertions(+), 31 deletions(-) diff --git a/go/tools/gopackagesdriver/bazel.go b/go/tools/gopackagesdriver/bazel.go index 554c2af596..a88a60dcea 100644 --- a/go/tools/gopackagesdriver/bazel.go +++ b/go/tools/gopackagesdriver/bazel.go @@ -35,11 +35,12 @@ const ( ) type Bazel struct { - bazelBin string - workspaceRoot string - bazelStartupFlags []string - info map[string]string - version bazelVersion + bazelBin string + workspaceRoot string + buildWorkingDirectory string + bazelStartupFlags []string + info map[string]string + version bazelVersion } // Minimal BEP structs to access the build outputs @@ -52,11 +53,12 @@ type BEPNamedSet struct { } `json:"namedSetOfFiles"` } -func NewBazel(ctx context.Context, bazelBin, workspaceRoot string, bazelStartupFlags []string) (*Bazel, error) { +func NewBazel(ctx context.Context, bazelBin, workspaceRoot string, buildWorkingDirectory string, bazelStartupFlags []string) (*Bazel, error) { b := &Bazel{ - bazelBin: bazelBin, - workspaceRoot: workspaceRoot, - bazelStartupFlags: bazelStartupFlags, + bazelBin: bazelBin, + workspaceRoot: workspaceRoot, + buildWorkingDirectory: buildWorkingDirectory, + bazelStartupFlags: bazelStartupFlags, } if err := b.fillInfo(ctx); err != nil { return nil, fmt.Errorf("unable to query bazel info: %w", err) @@ -163,6 +165,10 @@ func (b *Bazel) WorkspaceRoot() string { return b.workspaceRoot } +func (b *Bazel) BuildWorkingDirectory() string { + return b.buildWorkingDirectory +} + func (b *Bazel) ExecutionRoot() string { return b.info["execution_root"] } diff --git a/go/tools/gopackagesdriver/bazel_json_builder.go b/go/tools/gopackagesdriver/bazel_json_builder.go index f36108ce4b..a86c396011 100644 --- a/go/tools/gopackagesdriver/bazel_json_builder.go +++ b/go/tools/gopackagesdriver/bazel_json_builder.go @@ -21,7 +21,6 @@ import ( "fmt" "io/ioutil" "os" - "path" "path/filepath" "regexp" "runtime" @@ -39,14 +38,9 @@ var _defaultKinds = []string{"go_library", "go_test", "go_binary"} var externalRe = regexp.MustCompile(".*\\/external\\/([^\\/]+)(\\/(.*))?\\/([^\\/]+.go)") -func (b *BazelJSONBuilder) fileQuery(filename string) string { - label := filename - - if filepath.IsAbs(filename) { - label, _ = filepath.Rel(b.bazel.WorkspaceRoot(), filename) - } else if strings.HasPrefix(filename, "./") { - label = strings.TrimPrefix(filename, "./") - } +func (b *BazelJSONBuilder) fileQuery(label string) string { + label = b.adjustToRelativePathIfPossible(label) + filename := filepath.FromSlash(label) if matches := externalRe.FindStringSubmatch(filename); len(matches) == 5 { // if filepath is for a third party lib, we need to know, what external @@ -56,7 +50,7 @@ func (b *BazelJSONBuilder) fileQuery(filename string) string { } relToBin, err := filepath.Rel(b.bazel.info["output_path"], filename) - if err == nil && !strings.HasPrefix(relToBin, "../") { + if err == nil && !strings.HasPrefix(relToBin, filepath.FromSlash("../")) { parts := strings.SplitN(relToBin, string(filepath.Separator), 3) relToBin = parts[2] // We've effectively converted filename from bazel-bin/some/path.go to some/path.go; @@ -93,12 +87,7 @@ func (b *BazelJSONBuilder) getKind() string { } func (b *BazelJSONBuilder) localQuery(request string) string { - request = path.Clean(request) - if filepath.IsAbs(request) { - if relPath, err := filepath.Rel(workspaceRoot, request); err == nil { - request = relPath - } - } + request = b.adjustToRelativePathIfPossible(request) if !strings.HasSuffix(request, "...") { request = fmt.Sprintf("%s:*", request) @@ -107,6 +96,22 @@ func (b *BazelJSONBuilder) localQuery(request string) string { return fmt.Sprintf(`kind("^(%s) rule$", %s)`, b.getKind(), request) } +func (b *BazelJSONBuilder) adjustToRelativePathIfPossible(request string) string { + // If request is a relative path and gopackagesdriver is ran within a subdirectory of the + // workspace, we must first resolve the absolute path for it. + // Note: Using FromSlash/ToSlash for handling windows + absRequest := filepath.FromSlash(request) + if !filepath.IsAbs(absRequest) { + absRequest = filepath.Join(b.bazel.BuildWorkingDirectory(), absRequest) + } + if relPath, err := filepath.Rel(workspaceRoot, absRequest); err == nil { + request = filepath.ToSlash(relPath) + } else { + fmt.Fprintf(os.Stderr, "error adjusting path to be relative to the workspace root from request %s: %v\n", request, err) + } + return request +} + func (b *BazelJSONBuilder) packageQuery(importPath string) string { if strings.HasSuffix(importPath, "/...") { importPath = fmt.Sprintf(`^%s(/.+)?$`, strings.TrimSuffix(importPath, "/...")) diff --git a/go/tools/gopackagesdriver/gopackagesdriver_test.go b/go/tools/gopackagesdriver/gopackagesdriver_test.go index 3d61ab96ae..47d2ce6ce7 100644 --- a/go/tools/gopackagesdriver/gopackagesdriver_test.go +++ b/go/tools/gopackagesdriver/gopackagesdriver_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "os" "path" + "path/filepath" "strings" "testing" @@ -61,6 +62,25 @@ package hello_test import "testing" func TestHelloExternal(t *testing.T) {} + +-- subhello/BUILD.bazel -- +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "subhello", + srcs = ["subhello.go"], + importpath = "example.com/hello/subhello", + visibility = ["//visibility:public"], +) + +-- subhello/subhello.go -- +package subhello + +import "os" + +func main() { + fmt.Fprintln(os.Stderr, "Subdirectory Hello World!") +} `, }) } @@ -71,7 +91,7 @@ const ( ) func TestBaseFileLookup(t *testing.T) { - resp := runForTest(t, "file=hello.go") + resp := runForTest(t, ".", "file=hello.go") t.Run("roots", func(t *testing.T) { if len(resp.Roots) != 1 { @@ -140,8 +160,80 @@ func TestBaseFileLookup(t *testing.T) { }) } +func TestRelativeFileLookup(t *testing.T) { + resp := runForTest(t, "subhello", "file=./subhello.go") + + t.Run("roots", func(t *testing.T) { + if len(resp.Roots) != 1 { + t.Errorf("Expected 1 package root: %+v", resp.Roots) + return + } + + if !strings.HasSuffix(resp.Roots[0], "//subhello:subhello") { + t.Errorf("Unexpected package id: %q", resp.Roots[0]) + return + } + }) + + t.Run("package", func(t *testing.T) { + var pkg *FlatPackage + for _, p := range resp.Packages { + if p.ID == resp.Roots[0] { + pkg = p + } + } + + if pkg == nil { + t.Errorf("Expected to find %q in resp.Packages", resp.Roots[0]) + return + } + + if len(pkg.CompiledGoFiles) != 1 || len(pkg.GoFiles) != 1 || + path.Base(pkg.GoFiles[0]) != "subhello.go" || path.Base(pkg.CompiledGoFiles[0]) != "subhello.go" { + t.Errorf("Expected to find 1 file (subhello.go) in (Compiled)GoFiles:\n%+v", pkg) + return + } + }) +} + +func TestRelativePatternWildcardLookup(t *testing.T) { + resp := runForTest(t, "subhello", "./...") + + t.Run("roots", func(t *testing.T) { + if len(resp.Roots) != 1 { + t.Errorf("Expected 1 package root: %+v", resp.Roots) + return + } + + if !strings.HasSuffix(resp.Roots[0], "//subhello:subhello") { + t.Errorf("Unexpected package id: %q", resp.Roots[0]) + return + } + }) + + t.Run("package", func(t *testing.T) { + var pkg *FlatPackage + for _, p := range resp.Packages { + if p.ID == resp.Roots[0] { + pkg = p + } + } + + if pkg == nil { + t.Errorf("Expected to find %q in resp.Packages", resp.Roots[0]) + return + } + + if len(pkg.CompiledGoFiles) != 1 || len(pkg.GoFiles) != 1 || + path.Base(pkg.GoFiles[0]) != "subhello.go" || path.Base(pkg.CompiledGoFiles[0]) != "subhello.go" { + t.Errorf("Expected to find 1 file (subhello.go) in (Compiled)GoFiles:\n%+v", pkg) + return + } + }) +} + func TestExternalTests(t *testing.T) { - resp := runForTest(t, "file=hello_external_test.go") + resp := runForTest(t, ".", "file=hello_external_test.go") if len(resp.Roots) != 2 { t.Errorf("Expected exactly two roots for package: %+v", resp.Roots) } @@ -167,7 +259,7 @@ func TestExternalTests(t *testing.T) { } } -func runForTest(t *testing.T, args ...string) driverResponse { +func runForTest(t *testing.T, relativeWorkingDir string, args ...string) driverResponse { t.Helper() // Remove most environment variables, other than those on an allowlist. @@ -210,7 +302,7 @@ func runForTest(t *testing.T, args ...string) driverResponse { } }() - // Set workspaceRoot global variable. + // Set workspaceRoot and buildWorkingDirectory global variable. // It's initialized to the BUILD_WORKSPACE_DIRECTORY environment variable // before this point. wd, err := os.Getwd() @@ -218,8 +310,13 @@ func runForTest(t *testing.T, args ...string) driverResponse { t.Fatal(err) } oldWorkspaceRoot := workspaceRoot + oldBuildWorkingDirectory := buildWorkingDirectory workspaceRoot = wd - defer func() { workspaceRoot = oldWorkspaceRoot }() + buildWorkingDirectory = filepath.Join(wd, relativeWorkingDir) + defer func() { + workspaceRoot = oldWorkspaceRoot + buildWorkingDirectory = oldBuildWorkingDirectory + }() in := strings.NewReader("{}") out := &bytes.Buffer{} diff --git a/go/tools/gopackagesdriver/main.go b/go/tools/gopackagesdriver/main.go index 0613a3be79..ea591f6f7c 100644 --- a/go/tools/gopackagesdriver/main.go +++ b/go/tools/gopackagesdriver/main.go @@ -62,6 +62,7 @@ var ( bazelQueryScope = getenvDefault("GOPACKAGESDRIVER_BAZEL_QUERY_SCOPE", "") bazelBuildFlags = strings.Fields(os.Getenv("GOPACKAGESDRIVER_BAZEL_BUILD_FLAGS")) workspaceRoot = os.Getenv("BUILD_WORKSPACE_DIRECTORY") + buildWorkingDirectory = os.Getenv("BUILD_WORKING_DIRECTORY") additionalAspects = strings.Fields(os.Getenv("GOPACKAGESDRIVER_BAZEL_ADDTL_ASPECTS")) additionalKinds = strings.Fields(os.Getenv("GOPACKAGESDRIVER_BAZEL_KINDS")) emptyResponse = &driverResponse{ @@ -81,7 +82,7 @@ func run(ctx context.Context, in io.Reader, out io.Writer, args []string) error return fmt.Errorf("unable to read request: %w", err) } - bazel, err := NewBazel(ctx, bazelBin, workspaceRoot, bazelStartupFlags) + bazel, err := NewBazel(ctx, bazelBin, workspaceRoot, buildWorkingDirectory, bazelStartupFlags) if err != nil { return fmt.Errorf("unable to create bazel instance: %w", err) } From 6583a73fdc7089f42e6d49007cd3dc7861fb205f Mon Sep 17 00:00:00 2001 From: Tyler Rockwood Date: Tue, 20 Aug 2024 09:08:33 -0500 Subject: [PATCH 06/15] Support GOOS=wasip1 (#4045) **What type of PR is this?** Feature **What does this PR do? Why is it needed?** Adds support for GOOS=wasip1 so that users can build wasi binaries using rules_go **Which issues(s) does this PR fix?** Fixes https://github.com/bazelbuild/rules_go/issues/4046 **Other notes for review** None --- go/private/platforms.bzl | 1 + go/tools/internal/stdlib_tags/stdlib_tags.go | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/go/private/platforms.bzl b/go/private/platforms.bzl index 761969ef8a..0a1e343f1d 100644 --- a/go/private/platforms.bzl +++ b/go/private/platforms.bzl @@ -90,6 +90,7 @@ GOOS_GOARCH = ( ("plan9", "amd64"), ("plan9", "arm"), ("solaris", "amd64"), + ("wasip1", "wasm"), ("windows", "386"), ("windows", "amd64"), ("windows", "arm"), diff --git a/go/tools/internal/stdlib_tags/stdlib_tags.go b/go/tools/internal/stdlib_tags/stdlib_tags.go index 8bd507af3a..dfb9745b75 100644 --- a/go/tools/internal/stdlib_tags/stdlib_tags.go +++ b/go/tools/internal/stdlib_tags/stdlib_tags.go @@ -125,7 +125,7 @@ func isConstraint(line string) bool { } // Taken from -// https://github.com/golang/go/blob/3d5391ed87d813110e10b954c62bf7ed578b591f/src/go/build/syslist.go +// https://github.com/golang/go/blob/2693f77b3583585172810427e12a634b28d34493/src/internal/syslist/syslist.go var knownOS = map[string]bool{ "aix": true, "android": true, @@ -142,6 +142,7 @@ var knownOS = map[string]bool{ "openbsd": true, "plan9": true, "solaris": true, + "wasip1": true, "windows": true, "zos": true, } From c8fca5c0ab42b21aeca74c98084b1aeb62e9ea9d Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Thu, 22 Aug 2024 01:23:32 -0700 Subject: [PATCH 07/15] builder: work around change in filepath.EvalSymlinks behavior (#4050) **What type of PR is this?** > Bug fix **What does this PR do? Why is it needed?** filepath.EvalSymlinks works differently on Windows in Go 1.23.0: mount points and reparse points are no longer treated as symbolic links by os.Stat. Bazel uses junctions (implemented as reparse points) for inputs, so this means EvalSymlinks can fail with some kinds of inputs where it didn't before. replicateTree calls EvalSymlinks before constructing an input tree for `GoStdlib`. This seems to be necessary in some cases, though I'm not altogether sure why. EvalSymlinks fails due to the new behavior though. This change sets `GODEBUG=winsymlink=0` when invoking the builder binary to revert to the old behavior. This should not affect the behavior of user code compiled using the builder. This may not be a permanent solution, but it should work at least through Go 1.25. **Which issues(s) does this PR fix?** Fixes #4049 **Other notes for review** No test, but verified locally. Simply running `GoStdlib` with Go 1.23.0 will cover this. rules_go currently builds with Go 1.21.8 (no longer a version supported by Google!). --- go/private/context.bzl | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/go/private/context.bzl b/go/private/context.bzl index 247c4a3443..1eb69a5ea6 100644 --- a/go/private/context.bzl +++ b/go/private/context.bzl @@ -503,6 +503,14 @@ def go_context(ctx, attr = None): # # See https://go.dev/doc/toolchain for more info. "GOTOOLCHAIN": "local", + + # NOTE(#4049): Since Go 1.23.0, os.Readlink (and consequently + # filepath.EvalSymlinks) stopped treating Windows mount points and + # reparse points (junctions) as symbolic links. Bazel uses junctions + # when constructing exec roots, and we use filepath.EvalSymlinks in + # GoStdlib, so this broke us. Setting GODEBUG=winsymlink=0 restores + # the old behavior. + "GODEBUG": "winsymlink=0", } # Path mapping can't map the values of environment variables, so we pass GOROOT to the action From 7666720a3e074767481d3109476a10adbd68073b Mon Sep 17 00:00:00 2001 From: Tom van der Woerdt Date: Wed, 28 Aug 2024 14:54:49 +0200 Subject: [PATCH 08/15] gomock: allow passing extra flags to mockgen_tool (#4066) **What type of PR is this?** > Feature **What does this PR do? Why is it needed?** Adds the ability to pass extra arguments to gomock **Which issues(s) does this PR fix?** Fixes #4065 Closes #4067 **Other notes for review** cleaner --- docs/go/extras/extras.md | 3 ++- extras/gomock.bzl | 16 +++++++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/docs/go/extras/extras.md b/docs/go/extras/extras.md index b3eb5be1df..1c3a2007b7 100644 --- a/docs/go/extras/extras.md +++ b/docs/go/extras/extras.md @@ -34,7 +34,7 @@ This rule has moved. See [gazelle rule] in the Gazelle repository.
 gomock(name, out, library, source_importpath, source, interfaces, package, self_package, aux_files,
-       mockgen_tool, imports, copyright_file, mock_names, kwargs)
+       mockgen_tool, mockgen_args, imports, copyright_file, mock_names, kwargs)
 
Calls [mockgen](https://github.com/golang/mock) to generates a Go file containing mocks from the given library. @@ -57,6 +57,7 @@ If `source` is given, the mocks are generated in source mode; otherwise in refle | self_package | the full package import path for the generated code. The purpose of this flag is to prevent import cycles in the generated code by trying to include its own package. See [mockgen's -self_package](https://github.com/golang/mock#flags) for more information. | "" | | aux_files | a map from source files to their package path. This only needed when source is provided. See [mockgen's -aux_files](https://github.com/golang/mock#flags) for more information. | {} | | mockgen_tool | the mockgen tool to run. | Label("//extras/gomock:mockgen") | +| mockgen_args | additional arguments to pass to the mockgen tool. | [] | | imports | dictionary of name-path pairs of explicit imports to use. See [mockgen's -imports](https://github.com/golang/mock#flags) for more information. | {} | | copyright_file | optional file containing copyright to prepend to the generated contents. See [mockgen's -copyright_file](https://github.com/golang/mock#flags) for more information. | None | | mock_names | dictionary of interface name to mock name pairs to change the output names of the mock objects. Mock names default to 'Mock' prepended to the name of the interface. See [mockgen's -mock_names](https://github.com/golang/mock#flags) for more information. | {} | diff --git a/extras/gomock.bzl b/extras/gomock.bzl index 20e06d2a4a..c6d034f225 100644 --- a/extras/gomock.bzl +++ b/extras/gomock.bzl @@ -160,6 +160,10 @@ _gomock_source = rule( cfg = "exec", mandatory = False, ), + "mockgen_args": attr.string_list( + doc = "Additional arguments to pass to the mockgen tool", + mandatory = False, + ), "_go_context_data": attr.label( default = "//:go_context_data", ), @@ -167,7 +171,7 @@ _gomock_source = rule( toolchains = [GO_TOOLCHAIN], ) -def gomock(name, out, library = None, source_importpath = "", source = None, interfaces = [], package = "", self_package = "", aux_files = {}, mockgen_tool = _MOCKGEN_TOOL, imports = {}, copyright_file = None, mock_names = {}, **kwargs): +def gomock(name, out, library = None, source_importpath = "", source = None, interfaces = [], package = "", self_package = "", aux_files = {}, mockgen_tool = _MOCKGEN_TOOL, mockgen_args = [], imports = {}, copyright_file = None, mock_names = {}, **kwargs): """Calls [mockgen](https://github.com/golang/mock) to generates a Go file containing mocks from the given library. If `source` is given, the mocks are generated in source mode; otherwise in reflective mode. @@ -183,6 +187,7 @@ def gomock(name, out, library = None, source_importpath = "", source = None, int self_package: the full package import path for the generated code. The purpose of this flag is to prevent import cycles in the generated code by trying to include its own package. See [mockgen's -self_package](https://github.com/golang/mock#flags) for more information. aux_files: a map from source files to their package path. This only needed when `source` is provided. See [mockgen's -aux_files](https://github.com/golang/mock#flags) for more information. mockgen_tool: the mockgen tool to run. + mockgen_args: additional arguments to pass to the mockgen tool. imports: dictionary of name-path pairs of explicit imports to use. See [mockgen's -imports](https://github.com/golang/mock#flags) for more information. copyright_file: optional file containing copyright to prepend to the generated contents. See [mockgen's -copyright_file](https://github.com/golang/mock#flags) for more information. mock_names: dictionary of interface name to mock name pairs to change the output names of the mock objects. Mock names default to 'Mock' prepended to the name of the interface. See [mockgen's -mock_names](https://github.com/golang/mock#flags) for more information. @@ -199,6 +204,7 @@ def gomock(name, out, library = None, source_importpath = "", source = None, int self_package = self_package, aux_files = aux_files, mockgen_tool = mockgen_tool, + mockgen_args = mockgen_args, imports = imports, copyright_file = copyright_file, mock_names = mock_names, @@ -213,6 +219,7 @@ def gomock(name, out, library = None, source_importpath = "", source = None, int package = package, self_package = self_package, mockgen_tool = mockgen_tool, + mockgen_args = mockgen_args, imports = imports, copyright_file = copyright_file, mock_names = mock_names, @@ -375,6 +382,11 @@ _gomock_prog_exec = rule( cfg = "exec", mandatory = False, ), + "mockgen_args": attr.string_list( + doc = "Additional arguments to pass to the mockgen tool", + mandatory = False, + default = [], + ), "_go_context_data": attr.label( default = "//:go_context_data", ), @@ -398,5 +410,7 @@ def _handle_shared_args(ctx, args): if len(ctx.attr.mock_names) > 0: mock_names = ",".join(["{0}={1}".format(name, pkg) for name, pkg in ctx.attr.mock_names.items()]) args += ["-mock_names", mock_names] + if ctx.attr.mockgen_args: + args += ctx.attr.mockgen_args return args, needed_files From e470f85bc91cfcc6f9e6823f45c353480a82636f Mon Sep 17 00:00:00 2001 From: Tyler French Date: Wed, 28 Aug 2024 17:06:14 -0500 Subject: [PATCH 09/15] Update nogo.rst (#4071) Resolves #3996 --------- Co-authored-by: Fabian Meumertzheim --- go/nogo.rst | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/go/nogo.rst b/go/nogo.rst index c1c9b58e1e..c1a815ecd8 100644 --- a/go/nogo.rst +++ b/go/nogo.rst @@ -24,14 +24,9 @@ .. footer:: The ``nogo`` logo was derived from the Go gopher, which was designed by Renee French. (http://reneefrench.blogspot.com/) The design is licensed under the Creative Commons 3.0 Attributions license. Read this article for more details: http://blog.golang.org/gopher -**WARNING**: This functionality is experimental, so its API might change. -Please do not rely on it for production use, but feel free to use it and file -issues. - ``nogo`` is a tool that analyzes the source code of Go programs. It runs -alongside the Go compiler in the Bazel Go rules and rejects programs that -contain disallowed coding patterns. In addition, ``nogo`` may report -compiler-like errors. +in an action after the Go compiler in the Bazel Go rules and rejects sources that +contain disallowed coding patterns from the configured analyzers. ``nogo`` is a powerful tool for preventing bugs and code anti-patterns early in the development process. It may be used to run the same analyses as `vet`_, From dfaceea811b9e44881d18e2ff2b362ba631279a1 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 30 Aug 2024 00:24:49 +0200 Subject: [PATCH 10/15] 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) + } +} From 6df867ce7684881ccb32719e5c3b62b2f3b04529 Mon Sep 17 00:00:00 2001 From: Tyler French Date: Fri, 30 Aug 2024 10:58:11 -0500 Subject: [PATCH 11/15] prepare rules_go release 0.50.0 (#4064) This will be merged, but we will end up cherry picking certain commits onto an `release-0.50` branch, which will not include all of the commits on master. --- MODULE.bazel | 2 +- go/def.bzl | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/MODULE.bazel b/MODULE.bazel index c1af0602d3..e40bba25c2 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -1,6 +1,6 @@ module( name = "rules_go", - version = "0.49.0", + version = "0.50.0", compatibility_level = 0, repo_name = "io_bazel_rules_go", ) diff --git a/go/def.bzl b/go/def.bzl index b32c7e2248..f7a6f75d84 100644 --- a/go/def.bzl +++ b/go/def.bzl @@ -121,7 +121,7 @@ TOOLS_NOGO = [str(Label(l)) for l in _TOOLS_NOGO] # Current version or next version to be tagged. Gazelle and other tools may # check this to determine compatibility. -RULES_GO_VERSION = "0.49.0" +RULES_GO_VERSION = "0.50.0" go_context = _go_context gomock = _gomock From c93053c99d757c340c6a506bcbc74385d6068653 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 4 Sep 2024 16:47:45 +0200 Subject: [PATCH 12/15] Do not report nogo diagnostics for cgo generated files (#4081) **What type of PR is this?** Bug fix **What does this PR do? Why is it needed?** **Which issues(s) does this PR fix?** Fixes #4079 **Other notes for review** --- go/private/actions/compilepkg.bzl | 2 +- go/tools/builders/nogo.go | 14 ++++--- go/tools/builders/nogo_main.go | 20 ++++++++- tests/core/nogo/custom/custom_test.go | 60 +++++++++++++++++++++++++-- 4 files changed, 85 insertions(+), 11 deletions(-) diff --git a/go/private/actions/compilepkg.bzl b/go/private/actions/compilepkg.bzl index 43872cf3c6..a7d26b616e 100644 --- a/go/private/actions/compilepkg.bzl +++ b/go/private/actions/compilepkg.bzl @@ -246,7 +246,7 @@ def _run_nogo( args.add_all(sources, before_each = "-src") if cgo_go_srcs: inputs.append(cgo_go_srcs) - args.add_all([cgo_go_srcs], before_each = "-src") + args.add_all([cgo_go_srcs], before_each = "-ignore_src") if cover_mode: args.add("-cover_mode", cover_mode) if recompile_internal_deps: diff --git a/go/tools/builders/nogo.go b/go/tools/builders/nogo.go index 9747942f89..44222013a5 100644 --- a/go/tools/builders/nogo.go +++ b/go/tools/builders/nogo.go @@ -20,13 +20,14 @@ func nogo(args []string) error { fs := flag.NewFlagSet("GoNogo", flag.ExitOnError) goenv := envFlags(fs) - var unfilteredSrcs, recompileInternalDeps multiFlag + var unfilteredSrcs, ignoreSrcs, 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") + fs.Var(&unfilteredSrcs, "src", ".go, .c, .cc, .m, .mm, .s, or .S file to be filtered and checked") + fs.Var(&ignoreSrcs, "ignore_src", ".go, .c, .cc, .m, .mm, .s, or .S file to be filtered and checked, but with its diagnostics ignored") fs.Var(&deps, "arc", "Import path, package path, and file name of a direct dependency, separated by '='") fs.Var(&facts, "facts", "Import path, package path, and file name of a direct dependency's nogo facts file, separated by '='") fs.StringVar(&importPath, "importpath", "", "The import path of the package being compiled. Not passed to the compiler, but may be displayed in debug data.") @@ -49,7 +50,7 @@ func nogo(args []string) error { } // Filter sources. - srcs, err := filterAndSplitFiles(unfilteredSrcs) + srcs, err := filterAndSplitFiles(append(unfilteredSrcs, ignoreSrcs...)) if err != nil { return err } @@ -81,10 +82,10 @@ func nogo(args []string) error { return err } - return runNogo(workDir, nogoPath, goSrcs, facts, importPath, importcfgPath, outFactsPath, outLogPath) + return runNogo(workDir, nogoPath, goSrcs, ignoreSrcs, facts, importPath, importcfgPath, outFactsPath, outLogPath) } -func runNogo(workDir string, nogoPath string, srcs []string, facts []archive, packagePath, importcfgPath, outFactsPath string, outLogPath string) error { +func runNogo(workDir string, nogoPath string, srcs, ignores []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. // We also need to write the validation output log. @@ -105,6 +106,9 @@ func runNogo(workDir string, nogoPath string, srcs []string, facts []archive, pa args = append(args, "-fact", fmt.Sprintf("%s=%s", fact.importPath, fact.file)) } args = append(args, "-x", outFactsPath) + for _, ignore := range ignores { + args = append(args, "-ignore", ignore) + } args = append(args, srcs...) paramsFile := filepath.Join(workDir, "nogo.param") diff --git a/go/tools/builders/nogo_main.go b/go/tools/builders/nogo_main.go index 9cf558ba1d..e9aff5ef5b 100644 --- a/go/tools/builders/nogo_main.go +++ b/go/tools/builders/nogo_main.go @@ -77,6 +77,8 @@ func run(args []string) (error, int) { importcfg := flags.String("importcfg", "", "The import configuration file") packagePath := flags.String("p", "", "The package path (importmap) of the package being compiled") xPath := flags.String("x", "", "The archive file where serialized facts should be written") + var ignores multiFlag + flags.Var(&ignores, "ignore", "Names of files to ignore") flags.Parse(args) srcs := flags.Args() @@ -85,7 +87,7 @@ func run(args []string) (error, int) { return fmt.Errorf("error parsing importcfg: %v", err), nogoError } - diagnostics, facts, err := checkPackage(analyzers, *packagePath, packageFile, importMap, factMap, srcs) + diagnostics, facts, err := checkPackage(analyzers, *packagePath, packageFile, importMap, factMap, srcs, ignores) if err != nil { return fmt.Errorf("error running analyzers: %v", err), nogoError } @@ -156,7 +158,7 @@ func readImportCfg(file string) (packageFile map[string]string, importMap map[st // It returns an empty string if no source code diagnostics need to be printed. // // This implementation was adapted from that of golang.org/x/tools/go/checker/internal/checker. -func checkPackage(analyzers []*analysis.Analyzer, packagePath string, packageFile, importMap map[string]string, factMap map[string]string, filenames []string) (string, []byte, error) { +func checkPackage(analyzers []*analysis.Analyzer, packagePath string, packageFile, importMap map[string]string, factMap map[string]string, filenames, ignoreFiles []string) (string, []byte, error) { // Register fact types and establish dependencies between analyzers. actions := make(map[*analysis.Analyzer]*action) var visit func(a *analysis.Analyzer) *action @@ -211,8 +213,22 @@ func checkPackage(analyzers []*analysis.Analyzer, packagePath string, packageFil act.pkg = pkg } + ignoreFilesSet := map[string]struct{}{} + for _, ignore := range ignoreFiles { + ignoreFilesSet[ignore] = struct{}{} + } // Process nolint directives similar to golangci-lint. + // Also skip over fully ignored files. for _, f := range pkg.syntax { + if _, ok := ignoreFilesSet[pkg.fset.Position(f.Pos()).Filename]; ok { + for _, act := range actions { + act.nolint = append(act.nolint, &Range{ + from: pkg.fset.Position(f.FileStart), + to: pkg.fset.Position(f.End()).Line, + }) + } + continue + } // CommentMap will correctly associate comments to the largest node group // applicable. This handles inline comments that might trail a large // assignment and will apply the comment to the entire assignment. diff --git a/tests/core/nogo/custom/custom_test.go b/tests/core/nogo/custom/custom_test.go index a7a6786b36..b23a20f00f 100644 --- a/tests/core/nogo/custom/custom_test.go +++ b/tests/core/nogo/custom/custom_test.go @@ -39,6 +39,7 @@ nogo( ":foofuncname", ":importfmt", ":visibility", + ":cgogen", ], # config = "", visibility = ["//visibility:public"], @@ -71,6 +72,14 @@ go_library( visibility = ["//visibility:public"], ) +go_library( + name = "cgogen", + srcs = ["cgogen.go"], + importpath = "cgogenanalyzer", + deps = ["@org_golang_x_tools//go/analysis"], + visibility = ["//visibility:public"], +) + go_library( name = "has_errors", srcs = ["has_errors.go"], @@ -85,6 +94,13 @@ go_library( deps = [":dep"], ) +go_library( + name = "uses_cgo_clean", + srcs = ["examplepkg/uses_cgo_clean.go"], + importpath = "uses_cgo_clean", + cgo = True, +) + go_library( name = "uses_cgo_with_errors", srcs = [ @@ -109,7 +125,7 @@ go_library( ) -- foofuncname.go -- -// importfmt checks for functions named "Foo". +// foofuncname checks for functions named "Foo". // It has the same package name as another check to test the checks with // the same package name do not conflict. package importfmt @@ -279,6 +295,40 @@ func run(pass *analysis.Pass) (interface{}, error) { return nil, nil } +-- cgogen.go -- +// cgogen reports diagnostics on files generated by cgo. +package cgogen + +import ( + "go/ast" + "strings" + + "golang.org/x/tools/go/analysis" +) + +const doc = "report synthetic diagnostics on files generated by cgo" + +var Analyzer = &analysis.Analyzer{ + Name: "cgogen", + Run: run, + Doc: doc, +} + +func run(pass *analysis.Pass) (interface{}, error) { + for _, f := range pass.Files { + ast.Inspect(f, func(n ast.Node) bool { + switch n := n.(type) { + case *ast.Comment: + if strings.HasPrefix(n.Text, "//go:linkname") || strings.HasPrefix(n.Text, "//go:cgo_import") { + pass.Reportf(n.Pos(), "nogo must not run on code generated by cgo") + } + return true + } + return true + }) + } + return nil, nil +} -- config.json -- { @@ -462,6 +512,10 @@ func Test(t *testing.T) { target: "//:no_errors", wantSuccess: true, excludes: []string{"no_errors.go"}, + }, { + desc: "uses_cgo_clean", + target: "//:uses_cgo_clean", + wantSuccess: true, }, } { t.Run(test.desc, func(t *testing.T) { @@ -477,9 +531,9 @@ func Test(t *testing.T) { stderr := &bytes.Buffer{} cmd.Stderr = stderr if err := cmd.Run(); err == nil && !test.wantSuccess { - t.Fatal("unexpected success") + t.Fatalf("unexpected success\n%s", stderr) } else if err != nil && test.wantSuccess { - t.Fatalf("unexpected error: %v", err) + t.Fatalf("unexpected error: %v\n%s", err, stderr) } for _, pattern := range test.includes { From 046d5bc3336010e094e282712c29405f5762d307 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 4 Sep 2024 17:09:35 +0200 Subject: [PATCH 13/15] Run nogo on internal and external tests libs, not testmain (#4082) **What type of PR is this?** Bug fix **What does this PR do? Why is it needed?** nogo should run on both the internal and external libraries compiled for a `go_test`, but didn't. It also shouldn't run on the generated `testmain.go` file, but did. --- go/private/rules/test.bzl | 9 ++++- tests/core/nogo/tests/tests_test.go | 59 ++++++++++++++++++++++++++++- 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/go/private/rules/test.bzl b/go/private/rules/test.bzl index b5db446928..5e0fad37c3 100644 --- a/go/private/rules/test.bzl +++ b/go/private/rules/test.bzl @@ -60,10 +60,14 @@ def _go_test_impl(ctx): go = go_context(ctx) + validation_outputs = [] + # Compile the library to test with internal white box tests internal_library = go.new_library(go, testfilter = "exclude") internal_source = go.library_to_source(go, ctx.attr, internal_library, ctx.coverage_instrumented()) internal_archive = go.archive(go, internal_source) + if internal_archive.data._validation_output: + validation_outputs.append(internal_archive.data._validation_output) go_srcs = split_srcs(internal_source.srcs).go # Compile the library with the external black box tests @@ -82,6 +86,8 @@ def _go_test_impl(ctx): ), external_library, ctx.coverage_instrumented()) external_source, internal_archive = _recompile_external_deps(go, external_source, internal_archive, [t.label for t in ctx.attr.embed]) external_archive = go.archive(go, external_source, is_external_pkg = True) + if external_archive.data._validation_output: + validation_outputs.append(external_archive.data._validation_output) # now generate the main function repo_relative_rundir = ctx.attr.rundir or ctx.label.package or "." @@ -165,7 +171,6 @@ def _go_test_impl(ctx): version_file = ctx.version_file, info_file = ctx.info_file, ) - validation_output = test_archive.data._validation_output env = {} for k, v in ctx.attr.env.items(): @@ -187,7 +192,7 @@ def _go_test_impl(ctx): ), OutputGroupInfo( compilation_outputs = [internal_archive.data.file], - _validation = [validation_output] if validation_output else [], + _validation = validation_outputs, ), coverage_common.instrumented_files_info( ctx, diff --git a/tests/core/nogo/tests/tests_test.go b/tests/core/nogo/tests/tests_test.go index ed19dd41e2..801388b022 100644 --- a/tests/core/nogo/tests/tests_test.go +++ b/tests/core/nogo/tests/tests_test.go @@ -15,6 +15,7 @@ package importpath_test import ( + "strings" "testing" "github.com/bazelbuild/rules_go/go/tools/bazel_testing" @@ -45,6 +46,18 @@ go_test( srcs = ["super_simple_test.go"], ) +go_test( + name = "diagnostic_external_test", + size = "small", + srcs = ["diagnostic_external_test.go"], +) + +go_test( + name = "diagnostic_internal_test", + size = "small", + srcs = ["diagnostic_internal_test.go"], +) + nogo( name = "nogo", vet = True, @@ -75,21 +88,63 @@ import ( func TestFoo(t *testing.T) { } +-- diagnostic_external_test.go -- +package diagnostic_test + +import ( + "testing" +) + +func TestFoo(t *testing.T) { + if TestFoo == nil { + t.Fatal("TestFoo is nil") + } +} +-- diagnostic_internal_test.go -- +package diagnostic + +import ( + "testing" +) + +func TestFoo(t *testing.T) { + if TestFoo == nil { + t.Fatal("TestFoo is nil") + } +} `, Nogo: `@//:nogo`, }) } func TestExternalTestWithFullImportpath(t *testing.T) { - if out, err := bazel_testing.BazelOutput("test", "//:all"); err != nil { + if out, err := bazel_testing.BazelOutput("test", "//:simple_test"); err != nil { println(string(out)) t.Fatal(err) } } func TestEmptyExternalTest(t *testing.T) { - if out, err := bazel_testing.BazelOutput("test", "//:all"); err != nil { + if out, err := bazel_testing.BazelOutput("test", "//:super_simple_test"); err != nil { println(string(out)) t.Fatal(err) } } + +func TestDiagnosticInExternalTest(t *testing.T) { + if _, err := bazel_testing.BazelOutput("test", "//:diagnostic_external_test"); err == nil { + t.Fatal("unexpected success") + } else if !strings.Contains(err.Error(), "diagnostic_external_test.go:8:8: comparison of function TestFoo == nil is always false (nilfunc)") { + println(err.Error()) + t.Fatal("unexpected output") + } +} + +func TestDiagnosticInInternalTest(t *testing.T) { + if _, err := bazel_testing.BazelOutput("test", "//:diagnostic_internal_test"); err == nil { + t.Fatal("unexpected success") + } else if !strings.Contains(err.Error(), "diagnostic_internal_test.go:8:8: comparison of function TestFoo == nil is always false (nilfunc)") { + println(err.Error()) + t.Fatal("unexpected output") + } +} From 5e2e09d7033ea83325238becf47818c7e54dd013 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 4 Sep 2024 19:48:12 +0200 Subject: [PATCH 14/15] Only print type-checking error once in nogo (#4083) **What type of PR is this?** Bug fix **What does this PR do? Why is it needed?** Previously, it was printed once by analyzer. **Which issues(s) does this PR fix?** Fixes #4080 **Other notes for review** --- go/tools/builders/nogo_main.go | 13 ++++++++--- tests/core/nogo/custom/custom_test.go | 31 +++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/go/tools/builders/nogo_main.go b/go/tools/builders/nogo_main.go index e9aff5ef5b..8a3871d3f7 100644 --- a/go/tools/builders/nogo_main.go +++ b/go/tools/builders/nogo_main.go @@ -372,9 +372,7 @@ func (act *action) execOnce() { act.pass = pass var err error - if act.pkg.illTyped && !pass.Analyzer.RunDespiteErrors { - err = fmt.Errorf("analysis skipped due to type-checking error: %v", act.pkg.typeCheckError) - } else { + if !act.pkg.illTyped || pass.Analyzer.RunDespiteErrors { act.result, err = pass.Analyzer.Run(pass) if err == nil { if got, want := reflect.TypeOf(act.result), pass.Analyzer.ResultType; got != want { @@ -471,7 +469,13 @@ func checkAnalysisResults(actions []*action, pkg *goPackage) string { if cwd == "" || err != nil { errs = append(errs, fmt.Errorf("nogo failed to get CWD: %w", err)) } + numSkipped := 0 for _, act := range actions { + if act.pkg.illTyped && !act.a.RunDespiteErrors { + // Don't report type-checking errors once per analyzer. + numSkipped++ + continue + } if act.err != nil { // Analyzer failed. errs = append(errs, fmt.Errorf("analyzer %q failed: %v", act.a.Name, act.err)) @@ -543,6 +547,9 @@ func checkAnalysisResults(actions []*action, pkg *goPackage) string { } } } + if numSkipped > 0 { + errs = append(errs, fmt.Errorf("%d analyzers skipped due to type-checking error: %v", numSkipped, pkg.typeCheckError)) + } if len(diagnostics) == 0 && len(errs) == 0 { return "" } diff --git a/tests/core/nogo/custom/custom_test.go b/tests/core/nogo/custom/custom_test.go index b23a20f00f..3eb220d1aa 100644 --- a/tests/core/nogo/custom/custom_test.go +++ b/tests/core/nogo/custom/custom_test.go @@ -31,7 +31,7 @@ func TestMain(m *testing.M) { Nogo: "@//:nogo", Main: ` -- BUILD.bazel -- -load("@io_bazel_rules_go//go:def.bzl", "go_library", "nogo") +load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library", "nogo") nogo( name = "nogo", @@ -124,6 +124,12 @@ go_library( importpath = "dep", ) +go_binary( + name = "type_check_fail", + srcs = ["type_check_fail.go"], + pure = "on", +) + -- foofuncname.go -- // foofuncname checks for functions named "Foo". // It has the same package name as another check to test the checks with @@ -434,6 +440,16 @@ func Foo() bool { // This should fail foofuncname return Bar() } +-- type_check_fail.go -- +package type_check_fail + +import ( + "strings" +) + +func Foo() bool { + return strings.Split("a,b,c", ",") // This fails type-checking +} `, }) } @@ -443,6 +459,7 @@ func Test(t *testing.T) { desc, config, target string wantSuccess bool includes, excludes []string + bazelArgs []string }{ { desc: "default_config", @@ -507,6 +524,16 @@ func Test(t *testing.T) { // note the cross platform regex :) `examplepkg[\\/]pure_src_with_err_calling_native.go:.*function must not be named Foo \(foofuncname\)`, }, + }, { + desc: "type_check_fail", + config: "config.json", + target: "//:type_check_fail", + wantSuccess: false, + includes: []string{ + "4 analyzers skipped due to type-checking error: type_check_fail.go:8:10:", + }, + // Ensure that nogo runs even though compilation fails + bazelArgs: []string{"--keep_going"}, }, { desc: "no_errors", target: "//:no_errors", @@ -527,7 +554,7 @@ func Test(t *testing.T) { defer replaceInFile("BUILD.bazel", customConfig, origConfig) } - cmd := bazel_testing.BazelCmd("build", test.target) + cmd := bazel_testing.BazelCmd(append([]string{"build", test.target}, test.bazelArgs...)...) stderr := &bytes.Buffer{} cmd.Stderr = stderr if err := cmd.Run(); err == nil && !test.wantSuccess { From 1e855e21ad8273e470d0b417235304cd0fbd55b0 Mon Sep 17 00:00:00 2001 From: Tyler French Date: Wed, 4 Sep 2024 16:37:16 -0500 Subject: [PATCH 15/15] prepare patch release 0.50.1 (#4087) This PR prepares for the cherry-picked `0.50.1` patch release, which will include: #4081 #4082 #4083 These commits will be cherry-picked to the branch `release-0.50` --- MODULE.bazel | 2 +- go/def.bzl | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/MODULE.bazel b/MODULE.bazel index e40bba25c2..bc7eb86bdd 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -1,6 +1,6 @@ module( name = "rules_go", - version = "0.50.0", + version = "0.50.1", compatibility_level = 0, repo_name = "io_bazel_rules_go", ) diff --git a/go/def.bzl b/go/def.bzl index f7a6f75d84..a8908ec98d 100644 --- a/go/def.bzl +++ b/go/def.bzl @@ -121,7 +121,7 @@ TOOLS_NOGO = [str(Label(l)) for l in _TOOLS_NOGO] # Current version or next version to be tagged. Gazelle and other tools may # check this to determine compatibility. -RULES_GO_VERSION = "0.50.0" +RULES_GO_VERSION = "0.50.1" go_context = _go_context gomock = _gomock