Run nogo in a separate validation action (#3995)

**What type of PR is this?** Feature **What does this PR do? Why is it needed?** `nogo` is run in a separate action that can be toggled with `--run_validations`. This avoids rerunning compilation if the `nogo` tool changes and also allows to collect `nogo` findings of targets that depend on other targets that themselves have `nogo` findings (with `--keep_going`). **Which issues(s) does this PR fix?** Fixes #3619 Fixes #3695 Fixes #3846 Closes #3707 **Other notes for review** --------- Co-authored-by: Zhongpeng Lin <[email protected]>
bazel-contrib · Aug 6, 2024 · 29d4e5d · 29d4e5d
1 parent 5ec14ee
commit 29d4e5d
Show file tree

Hide file tree

Showing 17 changed files with 433 additions and 145 deletions.
diff --git a/go/nogo.rst b/go/nogo.rst
@@ -3,6 +3,7 @@
 
 .. _nogo: nogo.rst#nogo
 .. _configuring-analyzers: nogo.rst#configuring-analyzers
+.. _validation action: https://bazel.build/extending/rules#validation_actions
 .. _Bzlmod: /docs/go/core/bzlmod.md#configuring-nogo
 .. _go_library: /docs/go/core/rules.md#go_library
 .. _analysis: https://godoc.org/golang.org/x/tools/go/analysis
@@ -133,12 +134,22 @@ Usage
 ---------------------------------
 
 ``nogo``, upon configured, will be invoked automatically when building any Go target in your
-workspace.  If any of the analyzers reject the program, the build will fail.
+workspace.  If any of the analyzers reject the program, the build will fail. However, since
+``nogo`` runs in a `validation action`_ that is separate from compilation, you can use
+``--keep_going`` to have compilation continue and see all ``nogo`` findings, not just those
+from the first failing target. You can also specify ``--norun_validations`` to disable all
+validations, including ``nogo``.
+
+Note: Since the action that runs ``nogo`` doesn't fail if ``nogo`` produces findings, it
+is not possible to debug it with ``--sandbox_debug``. If necessary, set the ``debug``
+attribute of the ``nogo`` rule to ``True`` to have ``nogo`` fail in this case.
 
 ``nogo`` will run on all Go targets in your workspace, including tests and binary targets.
-It will also run on targets that are imported from other workspaces by default. You could
-exclude the external repositories from ``nogo`` by using the `exclude_files` regex in
-`configuring-analyzers`_.
+When using WORKSPACE, it will also run on targets that are imported from other workspaces
+by default. You could exclude the external repositories from ``nogo`` by using the
+`exclude_files` regex in `configuring-analyzers`_. With Bzlmod, external repositories are
+not validated with ``nogo`` by default. See the Bzlmod_ guide for more information
+on how to configure the ``nogo`` scope in this case.
 
 Relationship with other linters
 ~~~~~~~~~~~~~~~~~~~~~

diff --git a/go/private/actions/archive.bzl b/go/private/actions/archive.bzl
@@ -62,9 +62,13 @@ def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_d
     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)
     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")
 
     direct = [get_archive(dep) for dep in source.deps]
     runfiles = source.runfiles
@@ -110,6 +114,8 @@ def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_d
             out_lib = out_lib,
             out_export = out_export,
             out_facts = out_facts,
+            out_nogo_log = out_nogo_log,
+            out_nogo_validation = out_nogo_validation,
             nogo = nogo,
             out_cgo_export_h = out_cgo_export_h,
             gc_goopts = source.gc_goopts,
@@ -137,6 +143,8 @@ def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_d
             out_lib = out_lib,
             out_export = out_export,
             out_facts = out_facts,
+            out_nogo_log = out_nogo_log,
+            out_nogo_validation = out_nogo_validation,
             nogo = nogo,
             gc_goopts = source.gc_goopts,
             cgo = False,
@@ -185,6 +193,7 @@ def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_d
         export_file = out_export,
         facts_file = out_facts,
         data_files = as_tuple(data_files),
+        _validation_output = out_nogo_validation,
         _cgo_deps = as_tuple(cgo_deps),
     )
     x_defs = dict(source.x_defs)

diff --git a/go/private/actions/compilepkg.bzl b/go/private/actions/compilepkg.bzl
@@ -68,6 +68,8 @@ def emit_compilepkg(
         out_lib = None,
         out_export = None,
         out_facts = None,
+        out_nogo_log = None,
+        out_nogo_validation = None,
         nogo = None,
         out_cgo_export_h = None,
         gc_goopts = [],
@@ -81,6 +83,10 @@ def emit_compilepkg(
         fail("out_lib is a required parameter")
     if bool(nogo) != bool(out_facts):
         fail("nogo must be specified if and only if out_facts is specified")
+    if bool(nogo) != bool(out_nogo_log):
+        fail("nogo must be specified if and only if out_nogo_log is specified")
+    if bool(nogo) != bool(out_nogo_validation):
+        fail("nogo must be specified if and only if out_nogo_validation is specified")
 
     inputs = (sources + embedsrcs + [go.package_list] +
               [archive.data.export_file for archive in archives] +
@@ -104,13 +110,17 @@ def emit_compilepkg(
         uniquify = True,
         expand_directories = False,
     )
+    cover_mode = None
+    cover_archive = None
     if cover and go.coverdata:
-        inputs.append(go.coverdata.data.export_file)
-        args.add("-arc", _archive(go.coverdata))
+        cover_archive = go.coverdata
+        inputs.append(cover_archive.data.export_file)
+        args.add("-arc", _archive(cover_archive))
         if go.mode.race:
-            args.add("-cover_mode", "atomic")
+            cover_mode = "atomic"
         else:
-            args.add("-cover_mode", "set")
+            cover_mode = "set"
+        args.add("-cover_mode", cover_mode)
         args.add("-cover_format", go.cover_format)
         args.add_all(cover, before_each = "-cover")
     args.add_all(archives, before_each = "-arc", map_each = _archive)
@@ -126,13 +136,6 @@ def emit_compilepkg(
 
     args.add("-lo", out_lib)
     args.add("-o", out_export)
-    if nogo:
-        args.add_all(archives, before_each = "-facts", map_each = _facts)
-        inputs.extend([archive.data.facts_file for archive in archives if archive.data.facts_file])
-        args.add("-out_facts", out_facts)
-        outputs.append(out_facts)
-        args.add("-nogo", nogo)
-        inputs.append(nogo)
     if out_cgo_export_h:
         args.add("-cgoexport", out_cgo_export_h)
         outputs.append(out_cgo_export_h)
@@ -163,7 +166,12 @@ def emit_compilepkg(
     else:
         env = go.env_for_path_mapping
         execution_requirements = SUPPORTS_PATH_MAPPING_REQUIREMENT
+    cgo_go_srcs_for_nogo = None
     if cgo:
+        if nogo:
+            cgo_go_srcs_for_nogo = go.declare_directory(go, path = out_lib.basename + ".cgo")
+            outputs.append(cgo_go_srcs_for_nogo)
+            args.add("-cgo_go_srcs", cgo_go_srcs_for_nogo.path)
         inputs.extend(cgo_inputs.to_list())  # OPT: don't expand depset
         inputs.extend(go.cc_toolchain_files)
         env["CC"] = go.cgo_tools.c_compiler_path
@@ -194,3 +202,99 @@ def emit_compilepkg(
         toolchain = GO_TOOLCHAIN_LABEL,
         execution_requirements = execution_requirements,
     )
+
+    if nogo:
+        _run_nogo(
+            go,
+            sources = sources,
+            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,
+            out_facts = out_facts,
+            out_log = out_nogo_log,
+            out_validation = out_nogo_validation,
+            nogo = nogo,
+        )
+
+def _run_nogo(
+        go,
+        *,
+        sources,
+        importpath,
+        importmap,
+        archives,
+        recompile_internal_deps,
+        cover_mode,
+        cgo_go_srcs,
+        out_facts,
+        out_log,
+        out_validation,
+        nogo):
+    """Runs nogo on Go source files, including those generated by cgo."""
+    inputs = (sources + [nogo, go.package_list] +
+              [archive.data.facts_file for archive in archives if archive.data.facts_file] +
+              [archive.data.export_file for archive in archives] +
+              go.sdk.tools + go.sdk.headers + go.stdlib.libs)
+    outputs = [out_facts, out_log]
+
+    args = go.builder_args(go, "nogo", use_path_mapping = True)
+    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")
+    if cover_mode:
+        args.add("-cover_mode", cover_mode)
+    if recompile_internal_deps:
+        args.add_all(recompile_internal_deps, before_each = "-recompile_internal_deps")
+    args.add_all(archives, before_each = "-arc", map_each = _archive)
+    if importpath:
+        args.add("-importpath", importpath)
+    else:
+        args.add("-importpath", go.label.name)
+    if importmap:
+        args.add("-p", importmap)
+    args.add("-package_list", go.package_list)
+
+    args.add_all(archives, before_each = "-facts", map_each = _facts)
+    args.add("-out_facts", out_facts)
+    args.add("-out_log", out_log)
+    args.add("-nogo", nogo)
+
+    # This action runs nogo and produces the facts files for downstream nogo actions.
+    # It is important that this action doesn't fail if nogo produces findings, which allows users
+    # to get the nogo findings for all targets with --keep_going rather than stopping at the first
+    # target with findings.
+    # If nogo fails for any other reason, the action still fails, which allows users to debug their
+    # analyzers with --sandbox_debug. Users can set debug = True on the nogo target to have it fail
+    # on findings to get the same debugging experience as with other failures.
+    go.actions.run(
+        inputs = inputs,
+        outputs = outputs,
+        mnemonic = "RunNogo",
+        executable = go.toolchain._builder,
+        arguments = [args],
+        env = go.env_for_path_mapping,
+        toolchain = GO_TOOLCHAIN_LABEL,
+        execution_requirements = SUPPORTS_PATH_MAPPING_REQUIREMENT,
+        progress_message = "Running nogo on %{label}",
+    )
+
+    # This is a separate action that produces the validation output registered with Bazel. It
+    # prints any nogo findings and, crucially, fails if there are any findings. This is necessary
+    # to actually fail the build on nogo findings, which RunNogo doesn't do.
+    validation_args = go.actions.args()
+    validation_args.add("nogovalidation")
+    validation_args.add(out_validation)
+    validation_args.add(out_log)
+    go.actions.run(
+        inputs = [out_log],
+        outputs = [out_validation],
+        mnemonic = "ValidateNogo",
+        executable = go.toolchain._builder,
+        arguments = [validation_args],
+        execution_requirements = SUPPORTS_PATH_MAPPING_REQUIREMENT,
+        progress_message = "Validating nogo output for %{label}",
+    )
diff --git a/go/private/rules/binary.bzl b/go/private/rules/binary.bzl
@@ -119,6 +119,7 @@ def _go_binary_impl(ctx):
         info_file = ctx.info_file,
         executable = executable,
     )
+    validation_output = archive.data._validation_output
 
     providers = [
         library,
@@ -127,6 +128,7 @@ def _go_binary_impl(ctx):
         OutputGroupInfo(
             cgo_exports = archive.cgo_exports,
             compilation_outputs = [archive.data.file],
+            _validation = [validation_output] if validation_output else [],
         ),
     ]
 

diff --git a/go/private/rules/library.bzl b/go/private/rules/library.bzl
@@ -38,6 +38,7 @@ def _go_library_impl(ctx):
     library = go.new_library(go)
     source = go.library_to_source(go, ctx.attr, library, ctx.coverage_instrumented())
     archive = go.archive(go, source)
+    validation_output = archive.data._validation_output
 
     return [
         library,
@@ -55,6 +56,7 @@ def _go_library_impl(ctx):
         OutputGroupInfo(
             cgo_exports = archive.cgo_exports,
             compilation_outputs = [archive.data.file],
+            _validation = [validation_output] if validation_output else [],
         ),
     ]
 

diff --git a/go/private/rules/nogo.bzl b/go/private/rules/nogo.bzl
@@ -44,6 +44,8 @@ def _nogo_impl(ctx):
     nogo_args = ctx.actions.args()
     nogo_args.add("gennogomain")
     nogo_args.add("-output", nogo_main)
+    if ctx.attr.debug:
+        nogo_args.add("-debug")
     nogo_inputs = []
     analyzer_archives = [get_archive(dep) for dep in ctx.attr.deps]
     analyzer_importpaths = [archive.data.importpath for archive in analyzer_archives]
@@ -96,6 +98,9 @@ _nogo = rule(
         "config": attr.label(
             allow_single_file = True,
         ),
+        "debug": attr.bool(
+            default = False,
+        ),
         "_nogo_srcs": attr.label(
             default = "//go/tools/builders:nogo_srcs",
         ),

diff --git a/go/private/rules/test.bzl b/go/private/rules/test.bzl
@@ -165,6 +165,7 @@ 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():
@@ -186,6 +187,7 @@ def _go_test_impl(ctx):
         ),
         OutputGroupInfo(
             compilation_outputs = [internal_archive.data.file],
+            _validation = [validation_output] if validation_output else [],
         ),
         coverage_common.instrumented_files_info(
             ctx,

diff --git a/go/tools/builders/BUILD.bazel b/go/tools/builders/BUILD.bazel
@@ -65,6 +65,7 @@ filegroup(
         "builder.go",
         "cgo2.go",
         "compilepkg.go",
+        "constants.go",
         "cover.go",
         "edit.go",
         "embedcfg.go",
@@ -76,6 +77,8 @@ filegroup(
         "generate_test_main.go",
         "importcfg.go",
         "link.go",
+        "nogo.go",
+        "nogo_validation.go",
         "read.go",
         "replicate.go",
         "stdlib.go",
@@ -90,6 +93,7 @@ filegroup(
 go_source(
     name = "nogo_srcs",
     srcs = [
+        "constants.go",
         "env.go",
         "flags.go",
         "nogo_main.go",

diff --git a/go/tools/builders/builder.go b/go/tools/builders/builder.go
@@ -39,8 +39,9 @@ func main() {
 
 	var action func(args []string) error
 	switch verb {
-	case "compilepkg":
-		action = compilePkg
+	case "compilepkg": action = compilePkg
+	case "nogo": action = nogo
+	case "nogovalidation": action = nogoValidation
 	case "filterbuildid":
 		action = filterBuildID
 	case "gentestmain":

diff --git a/go/tools/builders/cgo2.go b/go/tools/builders/cgo2.go
@@ -32,7 +32,7 @@ import (
 )
 
 // cgo2 processes a set of mixed source files with cgo.
-func cgo2(goenv *env, goSrcs, cgoSrcs, cSrcs, cxxSrcs, objcSrcs, objcxxSrcs, sSrcs, hSrcs []string, packagePath, packageName string, cc string, cppFlags, cFlags, cxxFlags, objcFlags, objcxxFlags, ldFlags []string, cgoExportHPath string) (srcDir string, allGoSrcs, cObjs []string, err error) {
+func cgo2(goenv *env, goSrcs, cgoSrcs, cSrcs, cxxSrcs, objcSrcs, objcxxSrcs, sSrcs, hSrcs []string, packagePath, packageName string, cc string, cppFlags, cFlags, cxxFlags, objcFlags, objcxxFlags, ldFlags []string, cgoExportHPath string, cgoGoSrcsPath string) (srcDir string, allGoSrcs, cObjs []string, err error) {
 	// Report an error if the C/C++ toolchain wasn't configured.
 	if cc == "" {
 		err := cgoError(cgoSrcs[:])
@@ -240,6 +240,13 @@ func cgo2(goenv *env, goSrcs, cgoSrcs, cSrcs, cxxSrcs, objcSrcs, objcxxSrcs, sSr
 		return "", nil, nil, err
 	}
 	genGoSrcs = append(genGoSrcs, cgoImportsGo)
+	if cgoGoSrcsPath != "" {
+		for _, src := range genGoSrcs {
+			if err := copyFile(src, filepath.Join(cgoGoSrcsPath, filepath.Base(src))); err != nil {
+				return "", nil, nil, err
+			}
+		}
+	}
 
 	// Copy regular Go source files into the work directory so that we can
 	// use -trimpath=workDir.