From 29d4e5dbcfa702c479311f0bb4a929a236666a46 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 6 Aug 2024 22:12:05 +0200 Subject: [PATCH] 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 --- go/nogo.rst | 19 ++- go/private/actions/archive.bzl | 9 ++ go/private/actions/compilepkg.bzl | 126 ++++++++++++++++-- go/private/rules/binary.bzl | 2 + go/private/rules/library.bzl | 2 + go/private/rules/nogo.bzl | 5 + go/private/rules/test.bzl | 2 + go/tools/builders/BUILD.bazel | 4 + go/tools/builders/builder.go | 5 +- go/tools/builders/cgo2.go | 9 +- go/tools/builders/compilepkg.go | 141 ++++----------------- go/tools/builders/constants.go | 26 ++++ go/tools/builders/generate_nogo_main.go | 5 + go/tools/builders/nogo.go | 162 ++++++++++++++++++++++++ go/tools/builders/nogo_main.go | 30 +++-- go/tools/builders/nogo_validation.go | 29 +++++ tests/core/nogo/custom/custom_test.go | 2 +- 17 files changed, 433 insertions(+), 145 deletions(-) create mode 100644 go/tools/builders/constants.go create mode 100644 go/tools/builders/nogo.go create mode 100644 go/tools/builders/nogo_validation.go diff --git a/go/nogo.rst b/go/nogo.rst index 3a6f3a2a92..c1c9b58e1e 100644 --- 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 index 023546f56a..e2da3fbe2d 100644 --- 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 index 8866b6aa36..cfb7aee697 100644 --- 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 index 40a17f4d52..8bbe462268 100644 --- 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 index ac56354456..933d5f7438 100644 --- 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 index a4bc085b5e..9d6b1f9806 100644 --- 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 index ecd37f078a..b5db446928 100644 --- 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 index 2533b76d7d..244513000a 100644 --- 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 index 5d691839cb..f9b96b8b8e 100644 --- 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 index 80043e467b..63a109f4e6 100644 --- 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. diff --git a/go/tools/builders/compilepkg.go b/go/tools/builders/compilepkg.go index 52086acec6..9e941c77b8 100644 --- a/go/tools/builders/compilepkg.go +++ b/go/tools/builders/compilepkg.go @@ -17,29 +17,16 @@ package main import ( - "bytes" - "context" "errors" "flag" "fmt" - "io/ioutil" "os" - "os/exec" "path" "path/filepath" "sort" "strings" ) -type nogoResult int - -const ( - nogoNotRun nogoResult = iota - nogoError - nogoFailed - nogoSucceeded -) - func compilePkg(args []string) error { // Parse arguments. args, _, err := expandParamsFiles(args) @@ -50,9 +37,9 @@ func compilePkg(args []string) error { fs := flag.NewFlagSet("GoCompilePkg", flag.ExitOnError) goenv := envFlags(fs) var unfilteredSrcs, coverSrcs, embedSrcs, embedLookupDirs, embedRoots, recompileInternalDeps multiFlag - var deps, facts archiveMultiFlag - var importPath, packagePath, nogoPath, packageListPath, coverMode string - var outLinkobjPath, outInterfacePath, outFactsPath, cgoExportHPath string + var deps archiveMultiFlag + var importPath, packagePath, packageListPath, coverMode string + var outLinkobjPath, outInterfacePath, cgoExportHPath, cgoGoSrcsPath string var testFilter string var gcFlags, asmFlags, cppFlags, cFlags, cxxFlags, objcFlags, objcxxFlags, ldFlags quoteMultiFlag var coverFormat string @@ -63,7 +50,6 @@ func compilePkg(args []string) error { fs.Var(&embedLookupDirs, "embedlookupdir", "Root-relative paths to directories relative to which //go:embed directives are resolved") fs.Var(&embedRoots, "embedroot", "Bazel output root under which a file passed via -embedsrc resides") 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.") fs.StringVar(&packagePath, "p", "", "The package path (importmap) of the package being compiled") fs.Var(&gcFlags, "gcflags", "Go compiler flags") @@ -74,13 +60,12 @@ func compilePkg(args []string) error { fs.Var(&objcFlags, "objcflags", "Objective-C compiler flags") fs.Var(&objcxxFlags, "objcxxflags", "Objective-C++ compiler flags") fs.Var(&ldFlags, "ldflags", "C linker flags") - fs.StringVar(&nogoPath, "nogo", "", "The nogo binary. If unset, nogo will not be run.") fs.StringVar(&packageListPath, "package_list", "", "The file containing the list of standard library packages") fs.StringVar(&coverMode, "cover_mode", "", "The coverage mode to use. Empty if coverage instrumentation should not be added.") fs.StringVar(&outLinkobjPath, "lo", "", "The full output archive file required by the linker") fs.StringVar(&outInterfacePath, "o", "", "The export-only output archive required to compile dependent packages") - fs.StringVar(&outFactsPath, "out_facts", "", "The file to emit serialized nogo facts to (must be set if -nogo is set") fs.StringVar(&cgoExportHPath, "cgoexport", "", "The _cgo_exports.h file to write") + fs.StringVar(&cgoGoSrcsPath, "cgo_go_srcs", "", "The directory to emit cgo-generated Go sources for nogo consumption to") fs.StringVar(&testFilter, "testfilter", "off", "Controls test package filtering") fs.StringVar(&coverFormat, "cover_format", "", "Emit source file paths in coverage instrumentation suitable for the specified coverage format") fs.Var(&recompileInternalDeps, "recompile_internal_deps", "The import path of the direct dependencies that needs to be recompiled.") @@ -144,7 +129,6 @@ func compilePkg(args []string) error { packagePath, srcs, deps, - facts, coverMode, coverSrcs, embedSrcs, @@ -160,12 +144,11 @@ func compilePkg(args []string) error { objcFlags, objcxxFlags, ldFlags, - nogoPath, packageListPath, outLinkobjPath, outInterfacePath, - outFactsPath, cgoExportHPath, + cgoGoSrcsPath, coverFormat, recompileInternalDeps, pgoprofile) @@ -177,7 +160,6 @@ func compileArchive( packagePath string, srcs archiveSrcs, deps []archive, - facts []archive, coverMode string, coverSrcs []string, embedSrcs []string, @@ -193,12 +175,11 @@ func compileArchive( objcFlags []string, objcxxFlags []string, ldFlags []string, - nogoPath string, packageListPath string, outLinkObj string, outInterfacePath string, - outFactsPath string, cgoExportHPath string, + cgoGoSrcsForNogoPath string, coverFormat string, recompileInternalDeps []string, pgoprofile string, @@ -209,7 +190,6 @@ func compileArchive( } defer cleanup() - emptyGoFilePath := "" if len(srcs.goSrcs) == 0 { // We need to run the compiler to create a valid archive, even if there's nothing in it. // Otherwise, GoPack will complain if we try to add assembly or cgo objects. @@ -234,7 +214,6 @@ func compileArchive( matched: true, pkg: "empty", }) - emptyGoFilePath = emptyGoFile.Name() } packageName := srcs.goSrcs[0].pkg var goSrcs, cgoSrcs []string @@ -277,20 +256,11 @@ func compileArchive( // constraints. compilingWithCgo := haveCgo && cgoEnabled - filterForNogo := func(slice []string) []string { - filtered := make([]string, 0, len(slice)) - for _, s := range slice { - // Do not subject the generated empty .go file to nogo checks. - if s != emptyGoFilePath { - filtered = append(filtered, s) - } - } - return filtered - } // When coverage is set, source files will be modified during instrumentation. We should only run static analysis // over original source files and not the modified ones. // goSrcsNogo and cgoSrcsNogo are copies of the original source files for nogo to run static analysis. - goSrcsNogo := filterForNogo(goSrcs) + // TODO: Use slices.Clone when 1.21 is the minimal supported version. + goSrcsNogo := append([]string{}, goSrcs...) cgoSrcsNogo := append([]string{}, cgoSrcs...) // Instrument source files for coverage. @@ -348,27 +318,33 @@ func compileArchive( // C files. var objFiles []string if compilingWithCgo { - // If the package uses Cgo, compile .s and .S files with cgo2, not the Go assembler. - // Otherwise: the .s/.S files will be compiled with the Go assembler later var srcDir string - srcDir, goSrcs, objFiles, err = cgo2(goenv, goSrcs, cgoSrcs, cSrcs, cxxSrcs, objcSrcs, objcxxSrcs, sSrcs, hSrcs, packagePath, packageName, cc, cppFlags, cFlags, cxxFlags, objcFlags, objcxxFlags, ldFlags, cgoExportHPath) - if err != nil { - return err - } - if coverMode != "" && nogoPath != "" { - // Compile original source files, not coverage instrumented, for nogo - _, goSrcsNogo, _, err = cgo2(goenv, goSrcsNogo, cgoSrcsNogo, cSrcs, cxxSrcs, objcSrcs, objcxxSrcs, sSrcs, hSrcs, packagePath, packageName, cc, cppFlags, cFlags, cxxFlags, objcFlags, objcxxFlags, ldFlags, cgoExportHPath) + if coverMode != "" && cgoGoSrcsForNogoPath != "" { + // If the package uses Cgo, compile .s and .S files with cgo2, not the Go assembler. + // Otherwise: the .s/.S files will be compiled with the Go assembler later + srcDir, goSrcs, objFiles, err = cgo2(goenv, goSrcs, cgoSrcs, cSrcs, cxxSrcs, objcSrcs, objcxxSrcs, sSrcs, hSrcs, packagePath, packageName, cc, cppFlags, cFlags, cxxFlags, objcFlags, objcxxFlags, ldFlags, cgoExportHPath, "") + if err != nil { + return err + } + // Also run cgo on original source files, not coverage instrumented, if using nogo. + // The compilation outputs are only used to run cgo, but the generated sources are + // passed to the separate nogo action via cgoGoSrcsForNogoPath. + _, _, _, err = cgo2(goenv, goSrcsNogo, cgoSrcsNogo, cSrcs, cxxSrcs, objcSrcs, objcxxSrcs, sSrcs, hSrcs, packagePath, packageName, cc, cppFlags, cFlags, cxxFlags, objcFlags, objcxxFlags, ldFlags, "", cgoGoSrcsForNogoPath) if err != nil { return err } } else { - goSrcsNogo = goSrcs + // If the package uses Cgo, compile .s and .S files with cgo2, not the Go assembler. + // Otherwise: the .s/.S files will be compiled with the Go assembler later + srcDir, goSrcs, objFiles, err = cgo2(goenv, goSrcs, cgoSrcs, cSrcs, cxxSrcs, objcSrcs, objcxxSrcs, sSrcs, hSrcs, packagePath, packageName, cc, cppFlags, cFlags, cxxFlags, objcFlags, objcxxFlags, ldFlags, cgoExportHPath, cgoGoSrcsForNogoPath) + if err != nil { + return err + } } - gcFlags = append(gcFlags, createTrimPath(gcFlags, srcDir)) } else { if cgoExportHPath != "" { - if err := ioutil.WriteFile(cgoExportHPath, nil, 0o666); err != nil { + if err := os.WriteFile(cgoExportHPath, nil, 0o666); err != nil { return err } } @@ -447,22 +423,6 @@ func compileArchive( } } - // Run nogo concurrently. - var nogoChan chan error - if nogoPath != "" { - ctx, cancel := context.WithCancel(context.Background()) - nogoChan = make(chan error) - go func() { - nogoChan <- runNogo(ctx, workDir, nogoPath, goSrcsNogo, facts, importPath, importcfgPath, outFactsPath) - }() - defer func() { - if nogoChan != nil { - cancel() - <-nogoChan - } - }() - } - // If there are Go assembly files and this is go1.12+: generate symbol ABIs. // This excludes Cgo packages: they use the C compiler for assembly. asmHdrPath := "" @@ -527,16 +487,6 @@ func compileArchive( } } - // Check results from nogo. - if nogoChan != nil { - err := <-nogoChan - nogoChan = nil // no cancellation needed - if err != nil { - // TODO: Move nogo into a separate action so we don't fail the compilation here. - return err - } - } - return nil } @@ -564,45 +514,6 @@ func compileGo(goenv *env, srcs []string, packagePath, importcfgPath, embedcfgPa return goenv.runCommand(args) } -func runNogo(ctx context.Context, workDir string, nogoPath string, srcs []string, facts []archive, packagePath, importcfgPath, outFactsPath string) error { - if len(srcs) == 0 { - // emit_compilepkg expects a nogo facts file, even if it's empty. - return os.WriteFile(outFactsPath, nil, 0o666) - } - args := []string{nogoPath} - args = append(args, "-p", packagePath) - args = append(args, "-importcfg", importcfgPath) - for _, fact := range facts { - args = append(args, "-fact", fmt.Sprintf("%s=%s", fact.importPath, fact.file)) - } - args = append(args, "-x", outFactsPath) - args = append(args, srcs...) - - paramsFile := filepath.Join(workDir, "nogo.param") - if err := writeParamsFile(paramsFile, args[1:]); err != nil { - return fmt.Errorf("error writing nogo params file: %v", err) - } - - cmd := exec.CommandContext(ctx, args[0], "-param="+paramsFile) - out := &bytes.Buffer{} - cmd.Stdout, cmd.Stderr = out, out - if err := cmd.Run(); err != nil { - if exitErr, ok := err.(*exec.ExitError); ok { - if !exitErr.Exited() { - cmdLine := strings.Join(args, " ") - return fmt.Errorf("nogo command '%s' exited unexpectedly: %s", cmdLine, exitErr.String()) - } - return errors.New(string(relativizePaths(out.Bytes()))) - } else { - if out.Len() != 0 { - fmt.Fprintln(os.Stderr, out.String()) - } - return fmt.Errorf("error running nogo: %v", err) - } - } - return nil -} - func appendToArchive(goenv *env, outPath string, objFiles []string) error { // Use abs to work around long path issues on Windows. args := goenv.goTool("pack", "r", abs(outPath)) diff --git a/go/tools/builders/constants.go b/go/tools/builders/constants.go new file mode 100644 index 0000000000..6aa1040ee6 --- /dev/null +++ b/go/tools/builders/constants.go @@ -0,0 +1,26 @@ +// Copyright 2024 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. + +// This file contains constants used by nogo binaries. +// Note that this file is shared between the nogo binary and the builder. +// Sharing it as a library isn't possible as libraries depend on nogo, creating +// a circular dependency. +package main + +// The exit codes for nogo binaries. +const ( + nogoSuccess int = iota + nogoError + nogoViolation +) diff --git a/go/tools/builders/generate_nogo_main.go b/go/tools/builders/generate_nogo_main.go index 872b9b0a60..0ebf90cde8 100644 --- a/go/tools/builders/generate_nogo_main.go +++ b/go/tools/builders/generate_nogo_main.go @@ -84,6 +84,8 @@ var configs = map[string]config{ }, {{- end}} } + +const debugMode = {{ .Debug }} ` func genNogoMain(args []string) error { @@ -92,6 +94,7 @@ func genNogoMain(args []string) error { out := flags.String("output", "", "output file to write (defaults to stdout)") flags.Var(&analyzerImportPaths, "analyzer_importpath", "import path of an analyzer library") configFile := flags.String("config", "", "nogo config file") + debug := flags.Bool("debug", false, "enable debug mode") if err := flags.Parse(args); err != nil { return err } @@ -135,9 +138,11 @@ func genNogoMain(args []string) error { Imports []Import Configs Configs NeedRegexp bool + Debug bool }{ Imports: imports, Configs: config, + Debug: *debug, } for _, c := range config { if len(c.OnlyFiles) > 0 || len(c.ExcludeFiles) > 0 { diff --git a/go/tools/builders/nogo.go b/go/tools/builders/nogo.go new file mode 100644 index 0000000000..c49e8bb125 --- /dev/null +++ b/go/tools/builders/nogo.go @@ -0,0 +1,162 @@ +package main + +import ( + "bytes" + "errors" + "flag" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" +) + +func nogo(args []string) error { + // Parse arguments. + args, _, err := expandParamsFiles(args) + if err != nil { + return err + } + + fs := flag.NewFlagSet("GoNogo", flag.ExitOnError) + goenv := envFlags(fs) + var unfilteredSrcs, recompileInternalDeps multiFlag + var deps, facts archiveMultiFlag + var importPath, packagePath, nogoPath, packageListPath 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(&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.") + fs.StringVar(&packagePath, "p", "", "The package path (importmap) of the package being compiled") + 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(&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") + if err := fs.Parse(args); err != nil { + return err + } + if err := goenv.checkFlagsAndSetGoroot(); err != nil { + return err + } + if importPath == "" { + importPath = packagePath + } + + // Filter sources. + srcs, err := filterAndSplitFiles(unfilteredSrcs) + if err != nil { + return err + } + + var goSrcs []string + haveCgo := false + for _, src := range srcs.goSrcs { + if src.isCgo { + haveCgo = true + } else { + goSrcs = append(goSrcs, src.filename) + } + } + + workDir, cleanup, err := goenv.workDir() + if err != nil { + return err + } + defer cleanup() + + imports, err := checkImports(srcs.goSrcs, deps, packageListPath, importPath, recompileInternalDeps) + if err != nil { + return err + } + cgoEnabled := os.Getenv("CGO_ENABLED") == "1" + if haveCgo && cgoEnabled { + // cgo generated code imports some extra packages. + imports["runtime/cgo"] = nil + imports["syscall"] = nil + imports["unsafe"] = nil + } + if coverMode != "" { + if coverMode == "atomic" { + imports["sync/atomic"] = nil + } + const coverdataPath = "github.com/bazelbuild/rules_go/go/tools/coverdata" + var coverdata *archive + for i := range deps { + if deps[i].importPath == coverdataPath { + coverdata = &deps[i] + break + } + } + if coverdata == nil { + return errors.New("coverage requested but coverdata dependency not provided") + } + imports[coverdataPath] = coverdata + } + + importcfgPath, err := buildImportcfgFileForCompile(imports, goenv.installSuffix, filepath.Dir(outFactsPath)) + if err != nil { + return err + } + if !goenv.shouldPreserveWorkDir { + defer os.Remove(importcfgPath) + } + + return runNogo(workDir, nogoPath, goSrcs, facts, importPath, importcfgPath, outFactsPath, outLogPath) +} + +func runNogo(workDir string, nogoPath string, srcs []string, facts []archive, packagePath, importcfgPath, outFactsPath string, outLogPath string) error { + if len(srcs) == 0 { + // emit_compilepkg expects a nogo facts file, even if it's empty. + return os.WriteFile(outFactsPath, nil, 0o666) + } + args := []string{nogoPath} + args = append(args, "-p", packagePath) + args = append(args, "-importcfg", importcfgPath) + for _, fact := range facts { + args = append(args, "-fact", fmt.Sprintf("%s=%s", fact.importPath, fact.file)) + } + args = append(args, "-x", outFactsPath) + args = append(args, srcs...) + + paramsFile := filepath.Join(workDir, "nogo.param") + if err := writeParamsFile(paramsFile, args[1:]); err != nil { + return fmt.Errorf("error writing nogo params file: %v", err) + } + + cmd := exec.Command(args[0], "-param="+paramsFile) + out := &bytes.Buffer{} + cmd.Stdout, cmd.Stderr = out, out + // Always create this file as a Bazel-declared output, but keep it empty + // if nogo finds no issues. + outLog, err := os.Create(outLogPath) + if err != nil { + return fmt.Errorf("error creating nogo log file: %v", err) + } + defer outLog.Close() + err = cmd.Run() + if err == nil { + return nil + } + if exitErr, ok := err.(*exec.ExitError); ok { + if !exitErr.Exited() { + cmdLine := strings.Join(args, " ") + return fmt.Errorf("nogo command '%s' exited unexpectedly: %s", cmdLine, exitErr.String()) + } + prettyOut := relativizePaths(out.Bytes()) + if exitErr.ExitCode() != nogoViolation { + return errors.New(string(prettyOut)) + } + // Do not fail the action if nogo has findings so that facts are + // still available for downstream targets. + _, err := outLog.Write(prettyOut) + if err != nil { + return fmt.Errorf("error writing nogo log file: %v", err) + } + } + return nil +} + diff --git a/go/tools/builders/nogo_main.go b/go/tools/builders/nogo_main.go index d282177477..9cf558ba1d 100644 --- a/go/tools/builders/nogo_main.go +++ b/go/tools/builders/nogo_main.go @@ -57,17 +57,18 @@ var typesSizes = types.SizesFor("gc", os.Getenv("GOARCH")) func main() { log.SetFlags(0) // no timestamp log.SetPrefix("nogo: ") - if err := run(os.Args[1:]); err != nil { - log.Fatal(err) + if err, exitCode := run(os.Args[1:]); err != nil { + log.Print(err) + os.Exit(exitCode) } } // run returns an error if there is a problem loading the package or if any // analysis fails. -func run(args []string) error { +func run(args []string) (error, int) { args, _, err := expandParamsFiles(args) if err != nil { - return fmt.Errorf("error reading paramfiles: %v", err) + return fmt.Errorf("error reading paramfiles: %v", err), nogoError } factMap := factMultiFlag{} @@ -81,23 +82,30 @@ func run(args []string) error { packageFile, importMap, err := readImportCfg(*importcfg) if err != nil { - return fmt.Errorf("error parsing importcfg: %v", err) + return fmt.Errorf("error parsing importcfg: %v", err), nogoError } diagnostics, facts, err := checkPackage(analyzers, *packagePath, packageFile, importMap, factMap, srcs) if err != nil { - return fmt.Errorf("error running analyzers: %v", err) - } - if diagnostics != "" { - return fmt.Errorf("errors found by nogo during build-time code analysis:\n%s\n", diagnostics) + return fmt.Errorf("error running analyzers: %v", err), nogoError } + // Write the facts file for downstream consumers before failing due to diagnostics. if *xPath != "" { if err := ioutil.WriteFile(abs(*xPath), facts, 0o666); err != nil { - return fmt.Errorf("error writing facts: %v", err) + return fmt.Errorf("error writing facts: %v", err), nogoError } } + if diagnostics != "" { + // debugMode is defined by the template in generate_nogo_main.go. + exitCode := nogoViolation + if debugMode { + // Force actions running nogo to fail to help debug issues. + exitCode = nogoError + } + return fmt.Errorf("errors found by nogo during build-time code analysis:\n%s\n", diagnostics), exitCode + } - return nil + return nil, nogoSuccess } // Adapted from go/src/cmd/compile/internal/gc/main.go. Keep in sync. diff --git a/go/tools/builders/nogo_validation.go b/go/tools/builders/nogo_validation.go new file mode 100644 index 0000000000..3d164a9209 --- /dev/null +++ b/go/tools/builders/nogo_validation.go @@ -0,0 +1,29 @@ +package main + +import ( + "fmt" + "os" +) + +func nogoValidation(args []string) error { + validationOutput := args[0] + logFile := args[1] + // Always create the output file and only fail if the log file is non-empty to + // avoid an "action failed to create outputs" error. + logContent, err := os.ReadFile(logFile); + if err != nil { + return err + } + err = os.WriteFile(validationOutput, logContent, 0755) + if err != nil { + return err + } + if len(logContent) > 0 { + // Separate nogo output from Bazel's --sandbox_debug message via an + // empty line. + // Don't return to avoid printing the "nogovalidation:" prefix. + _, _ = fmt.Fprintf(os.Stderr, "\n%s\n", logContent) + os.Exit(1) + } + return nil +} diff --git a/tests/core/nogo/custom/custom_test.go b/tests/core/nogo/custom/custom_test.go index 2762443114..a7a6786b36 100644 --- a/tests/core/nogo/custom/custom_test.go +++ b/tests/core/nogo/custom/custom_test.go @@ -455,7 +455,7 @@ func Test(t *testing.T) { wantSuccess: false, includes: []string{ // note the cross platform regex :) - `.*[\\/]cgo[\\/]examplepkg[\\/]pure_src_with_err_calling_native.go:.*function must not be named Foo \(foofuncname\)`, + `examplepkg[\\/]pure_src_with_err_calling_native.go:.*function must not be named Foo \(foofuncname\)`, }, }, { desc: "no_errors",