Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nogo: use bazel validations and don't fail compile #3707

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions go/private/actions/archive.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,17 @@ 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

# capture the nogo_log output but don't fail the compile, later we assert that the log is empty
out_nogo_log = None
out_nogo_validation = None
if go.nogo:
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
data_files = runfiles.files
validations = []

files = []
for a in direct:
Expand Down Expand Up @@ -106,6 +114,7 @@ def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_d
out_lib = out_lib,
out_export = out_export,
out_cgo_export_h = out_cgo_export_h,
out_nogo_log = out_nogo_log,
gc_goopts = source.gc_goopts,
cgo = True,
cgo_inputs = cgo.inputs,
Expand All @@ -129,12 +138,28 @@ def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_d
archives = direct,
out_lib = out_lib,
out_export = out_export,
out_nogo_log = out_nogo_log,
gc_goopts = source.gc_goopts,
cgo = False,
testfilter = testfilter,
recompile_internal_deps = recompile_internal_deps,
)

if go.nogo:
validations.append(out_nogo_validation)
go.actions.run_shell(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We try to avoid relying on run_shell on Windows. Can you think of a way to replace this with a run?

For example, could this logic be moved into https://github.com/bazelbuild/rules_go/blob/master/go/tools/builders/generate_nogo_main.go?

inputs = [out_nogo_log],
outputs = [out_nogo_validation],
# Create the expected output file if there are no errors, otherwise print the errors to stderr.
command = '[ -s "$1" ] && cat <(echo) "$1" <(echo) 1>&2 || touch $2',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this approach. The actual nogo is still running as part of GoCompilePkg action, producing an output that is consumed by this no-ops command.

I would prefer if you start with separating nogo part of the go code into a separate builder sub-command, or ever a separate binary. Then in the validation action, we instrument that new subcommand/binary instead.

Not quite sure if it's a thing, but ideally user should be able to request for a --output_group=_validation and only trigger the validation actions to run linter, without much dependency on the compilation action.

arguments = [
out_nogo_log.path,
out_nogo_validation.path,
],
progress_message = "Nogo Static Analysis",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slightly misleading as the action just processes the results, it doesn't analyze anything.

Also, purely from a stylistic point of view, progress messages typically start with a verb in progressive form, e.g. Post-processing nogo findings.

mnemonic = "GoNogo",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future in which we have moved out the nogo analysis into its own action, this is exactly the mnemonic we would like to use for that. Since mnemonics are public API, please use something else here.

)

data = GoArchiveData(
# TODO(#2578): reconsider the provider API. There's a lot of redundant
# information here. Some fields are tuples instead of lists or dicts
Expand Down Expand Up @@ -198,4 +223,5 @@ def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_d
cgo_exports = cgo_exports,
runfiles = runfiles,
mode = go.mode,
validations = validations,
)
4 changes: 4 additions & 0 deletions go/private/actions/compilepkg.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ def emit_compilepkg(
clinkopts = [],
out_lib = None,
out_export = None,
out_nogo_log = None,
out_cgo_export_h = None,
gc_goopts = [],
testfilter = None, # TODO: remove when test action compiles packages
Expand Down Expand Up @@ -112,6 +113,9 @@ def emit_compilepkg(
if go.nogo:
args.add("-nogo", go.nogo)
inputs.append(go.nogo)
if out_nogo_log:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this ever validly be None? If not, let's fail if it is.

args.add("-nogo_log", out_nogo_log.path)
outputs.append(out_nogo_log)
if out_cgo_export_h:
args.add("-cgoexport", out_cgo_export_h)
outputs.append(out_cgo_export_h)
Expand Down
1 change: 1 addition & 0 deletions go/private/rules/binary.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ def _go_binary_impl(ctx):
OutputGroupInfo(
cgo_exports = archive.cgo_exports,
compilation_outputs = [archive.data.file],
_validation = depset(archive.validations),
),
]

Expand Down
1 change: 1 addition & 0 deletions go/private/rules/library.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ def _go_library_impl(ctx):
OutputGroupInfo(
cgo_exports = archive.cgo_exports,
compilation_outputs = [archive.data.file],
_validation = depset(archive.validations),
),
]

Expand Down
1 change: 1 addition & 0 deletions go/private/rules/test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ def _go_test_impl(ctx):
),
OutputGroupInfo(
compilation_outputs = [internal_archive.data.file],
_validation = test_archive.validations,
),
coverage_common.instrumented_files_info(
ctx,
Expand Down
43 changes: 28 additions & 15 deletions go/tools/builders/compilepkg.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func compilePkg(args []string) error {
goenv := envFlags(fs)
var unfilteredSrcs, coverSrcs, embedSrcs, embedLookupDirs, embedRoots, recompileInternalDeps multiFlag
var deps archiveMultiFlag
var importPath, packagePath, nogoPath, packageListPath, coverMode string
var importPath, packagePath, nogoPath, nogoLogPath, packageListPath, coverMode string
var outPath, outFactsPath, cgoExportHPath string
var testFilter string
var gcFlags, asmFlags, cppFlags, cFlags, cxxFlags, objcFlags, objcxxFlags, ldFlags quoteMultiFlag
Expand All @@ -74,6 +74,7 @@ func compilePkg(args []string) error {
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(&nogoLogPath, "nogo_log", "", "The path of the nogo log to write to. Required if -nogo is set.")
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(&outPath, "o", "", "The output archive file to write compiled code")
Expand Down Expand Up @@ -158,6 +159,7 @@ func compilePkg(args []string) error {
objcxxFlags,
ldFlags,
nogoPath,
nogoLogPath,
packageListPath,
outPath,
outFactsPath,
Expand Down Expand Up @@ -189,6 +191,7 @@ func compileArchive(
objcxxFlags []string,
ldFlags []string,
nogoPath string,
nogoLogPath string,
packageListPath string,
outPath string,
outXPath string,
Expand Down Expand Up @@ -449,18 +452,25 @@ func compileArchive(
// Add unknown origin source files into the mix.
nogoSrcs = append(nogoSrcs, goSrc)
}
if nogoPath != "" && len(nogoSrcs) > 0 {
ctx, cancel := context.WithCancel(context.Background())
nogoChan = make(chan error)
go func() {
nogoChan <- runNogo(ctx, workDir, nogoPath, nogoSrcs, deps, packagePath, importcfgPath, outFactsPath)
}()
defer func() {
if nogoChan != nil {
cancel()
<-nogoChan
}
}()
if nogoPath != "" {
nogoLogFile, err := os.OpenFile(nogoLogPath, os.O_CREATE|os.O_RDWR, 0644)
if err != nil {
return fmt.Errorf("error opening nogo log file: %v", err)
}
defer nogoLogFile.Close()
if len(nogoSrcs) > 0 {
ctx, cancel := context.WithCancel(context.Background())
nogoChan = make(chan error)
go func() {
nogoChan <- runNogo(ctx, workDir, nogoPath, nogoSrcs, deps, packagePath, importcfgPath, outFactsPath, nogoLogFile)
fmeum marked this conversation as resolved.
Show resolved Hide resolved
}()
defer func() {
if nogoChan != nil {
cancel()
<-nogoChan
}
}()
}
}

// If there are Go assembly files and this is go1.12+: generate symbol ABIs.
Expand Down Expand Up @@ -577,7 +587,7 @@ func compileGo(goenv *env, srcs []string, packagePath, importcfgPath, embedcfgPa
return goenv.runCommand(args)
}

func runNogo(ctx context.Context, workDir string, nogoPath string, srcs []string, deps []archive, packagePath, importcfgPath, outFactsPath string) error {
func runNogo(ctx context.Context, workDir string, nogoPath string, srcs []string, deps []archive, packagePath, importcfgPath, outFactsPath string, logFile *os.File) error {
args := []string{nogoPath}
args = append(args, "-p", packagePath)
args = append(args, "-importcfg", importcfgPath)
Expand All @@ -601,7 +611,10 @@ func runNogo(ctx context.Context, workDir string, nogoPath string, srcs []string
cmdLine := strings.Join(args, " ")
return fmt.Errorf("nogo command '%s' exited unexpectedly: %s", cmdLine, exitErr.String())
}
return errors.New(string(relativizePaths(out.Bytes())))
_, err := logFile.Write(relativizePaths(out.Bytes()))
if err != nil {
return fmt.Errorf("error writing nogo log: %v", err)
}
} else {
if out.Len() != 0 {
fmt.Fprintln(os.Stderr, out.String())
Expand Down
6 changes: 3 additions & 3 deletions go/tools/builders/nogo_main.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,14 @@ func run(args []string) error {
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)
}
if *xPath != "" {
if err := ioutil.WriteFile(abs(*xPath), facts, 0o666); err != nil {
return fmt.Errorf("error writing facts: %v", err)
}
}
if diagnostics != "" {
return fmt.Errorf("errors found by nogo during build-time code analysis:\n%s\n", diagnostics)
}

return nil
}
Expand Down