From 7ee9980105f607641fb2d594f01a253e48abb1f4 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Fri, 15 Sep 2023 12:45:17 -0400 Subject: [PATCH] cmd/go: replace formatOutput/showOutput with reportCmd The general pattern is to replace if len(cmdOut) > 0 { output := b.processOutput(cmdOut) if err != nil { err = formatOutput(b.WorkDir, dir, p.ImportPath, desc, output) } else { b.showOutput(a, dir, desc, output) } } if err != nil { return err } with if err := b.reportCmd(a, p, desc, dir, cmdOut, err); err != nil { return err } However, there is a fair amount of variation between call sites. The most common non-trivial variation is sites where errors are an expected outcome. In this case, often we simply pass "nil" for the error to trigger only the printing behavior of reportCmd. For #62067, but also a nice cleanup on its own. Change-Id: Ie5f918017c02d8558f23ad4c38261077c0fa4ea3 Reviewed-on: https://go-review.googlesource.com/c/go/+/529219 LUCI-TryBot-Result: Go LUCI Reviewed-by: Bryan Mills --- src/cmd/go/internal/work/cover.go | 8 +- src/cmd/go/internal/work/exec.go | 122 +++++++----------- src/cmd/go/internal/work/gc.go | 2 +- src/cmd/go/internal/work/gccgo.go | 25 ++-- .../testdata/script/list_pkgconfig_error.txt | 2 +- 5 files changed, 68 insertions(+), 91 deletions(-) diff --git a/src/cmd/go/internal/work/cover.go b/src/cmd/go/internal/work/cover.go index b1de4b0cb4..ebffe52412 100644 --- a/src/cmd/go/internal/work/cover.go +++ b/src/cmd/go/internal/work/cover.go @@ -72,9 +72,7 @@ func WriteCoveragePercent(b *Builder, runAct *Action, mf string, w io.Writer) er dir := filepath.Dir(mf) output, cerr := b.CovData(runAct, "percent", "-i", dir) if cerr != nil { - p := runAct.Package - return formatOutput(b.WorkDir, p.Dir, p.ImportPath, - p.Desc(), string(output)) + return b.reportCmd(runAct, nil, "", "", output, cerr) } _, werr := w.Write(output) return werr @@ -89,9 +87,7 @@ func WriteCoverageProfile(b *Builder, runAct *Action, mf, outf string, w io.Writ dir := filepath.Dir(mf) output, err := b.CovData(runAct, "textfmt", "-i", dir, "-o", outf) if err != nil { - p := runAct.Package - return formatOutput(b.WorkDir, p.Dir, p.ImportPath, - p.Desc(), string(output)) + return b.reportCmd(runAct, nil, "", "", output, err) } _, werr := w.Write(output) return werr diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go index eecdecaa05..7b5232f1b0 100644 --- a/src/cmd/go/internal/work/exec.go +++ b/src/cmd/go/internal/work/exec.go @@ -594,7 +594,7 @@ func (b *Builder) build(ctx context.Context, a *Action) (err error) { var objects, cgoObjects, pcCFLAGS, pcLDFLAGS []string if p.UsesCgo() || p.UsesSwig() { - if pcCFLAGS, pcLDFLAGS, err = b.getPkgConfigFlags(p); err != nil { + if pcCFLAGS, pcLDFLAGS, err = b.getPkgConfigFlags(a); err != nil { return } } @@ -868,15 +868,7 @@ OverlayLoop: // Compile Go. objpkg := objdir + "_pkg_.a" ofile, out, err := BuildToolchain.gc(b, a, objpkg, icfg.Bytes(), embedcfg, symabis, len(sfiles) > 0, gofiles) - if len(out) > 0 { - output := b.processOutput(out) - if err != nil { - return formatOutput(b.WorkDir, p.Dir, p.ImportPath, p.Desc(), output) - } else { - b.showOutput(a, p.Dir, p.Desc(), output) - } - } - if err != nil { + if err := b.reportCmd(a, nil, "", "", out, err); err != nil { return err } if ofile != objpkg { @@ -1000,8 +992,11 @@ func (b *Builder) checkDirectives(a *Action) error { } } if msg != nil { - return formatOutput(b.WorkDir, p.Dir, p.ImportPath, p.Desc(), b.processOutput(msg.Bytes())) - + // We pass a non-nil error to reportCmd to trigger the failure reporting + // path, but the content of the error doesn't matter because msg is + // non-empty. + err := errors.New("invalid directive") + return b.reportCmd(a, nil, "", "", msg.Bytes(), err) } return nil } @@ -1616,8 +1611,9 @@ func splitPkgConfigOutput(out []byte) ([]string, error) { return flags, nil } -// Calls pkg-config if needed and returns the cflags/ldflags needed to build the package. -func (b *Builder) getPkgConfigFlags(p *load.Package) (cflags, ldflags []string, err error) { +// Calls pkg-config if needed and returns the cflags/ldflags needed to build a's package. +func (b *Builder) getPkgConfigFlags(a *Action) (cflags, ldflags []string, err error) { + p := a.Package if pcargs := p.CgoPkgConfig; len(pcargs) > 0 { // pkg-config permits arguments to appear anywhere in // the command line. Move them all to the front, before --. @@ -1640,8 +1636,8 @@ func (b *Builder) getPkgConfigFlags(p *load.Package) (cflags, ldflags []string, var out []byte out, err = b.runOut(nil, p.Dir, nil, b.PkgconfigCmd(), "--cflags", pcflags, "--", pkgs) if err != nil { - err = formatOutput(b.WorkDir, p.Dir, p.ImportPath, b.PkgconfigCmd()+" --cflags "+strings.Join(pcflags, " ")+" -- "+strings.Join(pkgs, " "), string(out)+err.Error()) - return nil, nil, err + desc := b.PkgconfigCmd() + " --cflags " + strings.Join(pcflags, " ") + " -- " + strings.Join(pkgs, " ") + return nil, nil, b.reportCmd(a, nil, desc, "", out, err) } if len(out) > 0 { cflags, err = splitPkgConfigOutput(bytes.TrimSpace(out)) @@ -1654,8 +1650,8 @@ func (b *Builder) getPkgConfigFlags(p *load.Package) (cflags, ldflags []string, } out, err = b.runOut(nil, p.Dir, nil, b.PkgconfigCmd(), "--libs", pcflags, "--", pkgs) if err != nil { - err = formatOutput(b.WorkDir, p.Dir, p.ImportPath, b.PkgconfigCmd()+" --libs "+strings.Join(pcflags, " ")+" -- "+strings.Join(pkgs, " "), string(out)+err.Error()) - return nil, nil, err + desc := b.PkgconfigCmd() + " --libs " + strings.Join(pcflags, " ") + " -- " + strings.Join(pkgs, " ") + return nil, nil, b.reportCmd(a, nil, desc, "", out, err) } if len(out) > 0 { // We need to handle path with spaces so that C:/Program\ Files can pass @@ -2511,17 +2507,10 @@ var cgoTypeSigRe = lazyregexp.New(`\b_C2?(type|func|var|macro)_\B`) // and returns a non-nil error. func (b *Builder) run(a *Action, dir string, desc string, env []string, cmdargs ...any) error { out, err := b.runOut(a, dir, env, cmdargs...) - if len(out) > 0 { - if desc == "" { - desc = b.fmtcmd(dir, "%s", strings.Join(str.StringList(cmdargs...), " ")) - } - if err != nil { - err = formatOutput(b.WorkDir, dir, a.Package.ImportPath, desc, b.processOutput(out)) - } else { - b.showOutput(a, dir, desc, b.processOutput(out)) - } + if desc == "" { + desc = b.fmtcmd(dir, "%s", strings.Join(str.StringList(cmdargs...), " ")) } - return err + return b.reportCmd(a, nil, desc, dir, out, err) } // processOutput prepares the output of runOut to be output to the console. @@ -2870,38 +2859,33 @@ func (b *Builder) ccompile(a *Action, p *load.Package, outfile string, flags []s overlayPath = p } output, err := b.runOut(a, filepath.Dir(overlayPath), b.cCompilerEnv(), compiler, flags, "-o", outfile, "-c", filepath.Base(overlayPath)) - if len(output) > 0 { - // On FreeBSD 11, when we pass -g to clang 3.8 it - // invokes its internal assembler with -dwarf-version=2. - // When it sees .section .note.GNU-stack, it warns - // "DWARF2 only supports one section per compilation unit". - // This warning makes no sense, since the section is empty, - // but it confuses people. - // We work around the problem by detecting the warning - // and dropping -g and trying again. - if bytes.Contains(output, []byte("DWARF2 only supports one section per compilation unit")) { - newFlags := make([]string, 0, len(flags)) - for _, f := range flags { - if !strings.HasPrefix(f, "-g") { - newFlags = append(newFlags, f) - } - } - if len(newFlags) < len(flags) { - return b.ccompile(a, p, outfile, newFlags, file, compiler) - } - } - if err == nil && os.Getenv("GO_BUILDER_NAME") != "" { - output = append(output, "C compiler warning promoted to error on Go builders\n"...) - err = errors.New("warning promoted to error") + // On FreeBSD 11, when we pass -g to clang 3.8 it + // invokes its internal assembler with -dwarf-version=2. + // When it sees .section .note.GNU-stack, it warns + // "DWARF2 only supports one section per compilation unit". + // This warning makes no sense, since the section is empty, + // but it confuses people. + // We work around the problem by detecting the warning + // and dropping -g and trying again. + if bytes.Contains(output, []byte("DWARF2 only supports one section per compilation unit")) { + newFlags := make([]string, 0, len(flags)) + for _, f := range flags { + if !strings.HasPrefix(f, "-g") { + newFlags = append(newFlags, f) + } } - if err != nil { - err = formatOutput(b.WorkDir, p.Dir, p.ImportPath, p.Desc(), b.processOutput(output)) - } else { - b.showOutput(a, p.Dir, p.Desc(), b.processOutput(output)) + if len(newFlags) < len(flags) { + return b.ccompile(a, p, outfile, newFlags, file, compiler) } } - return err + + if len(output) > 0 && err == nil && os.Getenv("GO_BUILDER_NAME") != "" { + output = append(output, "C compiler warning promoted to error on Go builders\n"...) + err = errors.New("warning promoted to error") + } + + return b.reportCmd(a, p, "", "", output, err) } // gccld runs the gcc linker to create an executable from a set of object files. @@ -2915,7 +2899,6 @@ func (b *Builder) gccld(a *Action, p *load.Package, objdir, outfile string, flag } cmdargs := []any{cmd, "-o", outfile, objs, flags} - dir := p.Dir out, err := b.runOut(a, base.Cwd(), b.cCompilerEnv(), cmdargs...) if len(out) > 0 { @@ -2951,9 +2934,11 @@ func (b *Builder) gccld(a *Action, p *load.Package, objdir, outfile string, flag save = append(save, line) } out = bytes.Join(save, nil) - if len(out) > 0 && (cfg.BuildN || cfg.BuildX) { - b.showOutput(nil, dir, p.ImportPath, b.processOutput(out)) - } + } + // Note that failure is an expected outcome here, so we report output only + // in debug mode and don't report the error. + if cfg.BuildN || cfg.BuildX { + b.reportCmd(a, p, "", "", out, nil) } return err } @@ -3493,7 +3478,7 @@ func (b *Builder) cgo(a *Action, cgoExe, objdir string, pcCFLAGS, pcLDFLAGS, cgo if pkgpath := gccgoPkgpath(p); pkgpath != "" { cgoflags = append(cgoflags, "-gccgopkgpath="+pkgpath) } - if !BuildToolchain.(gccgoToolchain).supportsCgoIncomplete(b) { + if !BuildToolchain.(gccgoToolchain).supportsCgoIncomplete(b, a) { cgoflags = append(cgoflags, "-gccgo_define_cgoincomplete") } } @@ -3990,18 +3975,11 @@ func (b *Builder) swigOne(a *Action, p *load.Package, file, objdir string, pcCFL } out, err := b.runOut(a, p.Dir, nil, "swig", args, file) - if err != nil { - if len(out) > 0 { - if bytes.Contains(out, []byte("-intgosize")) || bytes.Contains(out, []byte("-cgo")) { - return "", "", errors.New("must have SWIG version >= 3.0.6") - } - // swig error - err = formatOutput(b.WorkDir, p.Dir, p.ImportPath, p.Desc(), b.processOutput(out)) - } - return "", "", err + if err != nil && (bytes.Contains(out, []byte("-intgosize")) || bytes.Contains(out, []byte("-cgo"))) { + return "", "", errors.New("must have SWIG version >= 3.0.6") } - if len(out) > 0 { - b.showOutput(a, p.Dir, p.Desc(), b.processOutput(out)) // swig warning + if err := b.reportCmd(a, p, "", "", out, err); err != nil { + return "", "", err } // If the input was x.swig, the output is x.go in the objdir. diff --git a/src/cmd/go/internal/work/gc.go b/src/cmd/go/internal/work/gc.go index 3ea9f714d2..6054e8d81c 100644 --- a/src/cmd/go/internal/work/gc.go +++ b/src/cmd/go/internal/work/gc.go @@ -464,7 +464,7 @@ func (gcToolchain) pack(b *Builder, a *Action, afile string, ofiles []string) er return nil } if err := packInternal(absAfile, absOfiles); err != nil { - return formatOutput(b.WorkDir, p.Dir, p.ImportPath, p.Desc(), err.Error()+"\n") + return b.reportCmd(a, nil, "", "", nil, err) } return nil } diff --git a/src/cmd/go/internal/work/gccgo.go b/src/cmd/go/internal/work/gccgo.go index fa566bfd05..f38d57a43f 100644 --- a/src/cmd/go/internal/work/gccgo.go +++ b/src/cmd/go/internal/work/gccgo.go @@ -5,6 +5,7 @@ package work import ( + "bytes" "fmt" "os" "os/exec" @@ -243,12 +244,8 @@ func (tools gccgoToolchain) pack(b *Builder, a *Action, afile string, ofiles []s return b.run(a, p.Dir, p.ImportPath, nil, tools.ar(), arArgs, "rc", absAfile, absOfiles) } - if len(output) > 0 { - // Show the output if there is any even without errors. - b.showOutput(a, p.Dir, p.ImportPath, b.processOutput(output)) - } - - return nil + // Show the output if there is any even without errors. + return b.reportCmd(a, nil, "", "", output, nil) } func (tools gccgoToolchain) link(b *Builder, root *Action, out, importcfg string, allactions []*Action, buildmode, desc string) error { @@ -617,7 +614,10 @@ type I cgo.Incomplete // supportsCgoIncomplete reports whether the gccgo/GoLLVM compiler // being used supports cgo.Incomplete, which was added in GCC 13. -func (tools gccgoToolchain) supportsCgoIncomplete(b *Builder) bool { +// +// This takes an Action only for output reporting purposes. +// The result value is unrelated to the Action. +func (tools gccgoToolchain) supportsCgoIncomplete(b *Builder, a *Action) bool { gccgoSupportsCgoIncompleteOnce.Do(func() { fail := func(err error) { fmt.Fprintf(os.Stderr, "cmd/go: %v\n", err) @@ -650,14 +650,17 @@ func (tools gccgoToolchain) supportsCgoIncomplete(b *Builder) bool { } cmd := exec.Command(tools.compiler(), "-c", "-o", on, fn) cmd.Dir = tmpdir - var buf strings.Builder + var buf bytes.Buffer cmd.Stdout = &buf cmd.Stderr = &buf err = cmd.Run() - if out := buf.String(); len(out) > 0 && cfg.BuildX { - b.showOutput(nil, tmpdir, b.fmtcmd(tmpdir, "%s -c -o %s %s", tools.compiler(), on, fn), out) - } gccgoSupportsCgoIncomplete = err == nil + if cfg.BuildN || cfg.BuildX { + // Show output. We always pass a nil err because errors are an + // expected outcome in this case. + desc := b.fmtcmd(tmpdir, "%s -c -o %s %s", tools.compiler(), on, fn) + b.reportCmd(a, nil, desc, tmpdir, buf.Bytes(), nil) + } }) return gccgoSupportsCgoIncomplete } diff --git a/src/cmd/go/testdata/script/list_pkgconfig_error.txt b/src/cmd/go/testdata/script/list_pkgconfig_error.txt index 7d671a6438..8e5a278dd0 100644 --- a/src/cmd/go/testdata/script/list_pkgconfig_error.txt +++ b/src/cmd/go/testdata/script/list_pkgconfig_error.txt @@ -2,7 +2,7 @@ [!exec:pkg-config] skip 'test requires pkg-config tool' ! go list -export . -stderr '^go build example:\n# pkg-config (.*\n)+pkg-config: exit status \d+$' +stderr '^go build example:\n# pkg-config (.*\n)+Package .* not found$' -- go.mod -- module example -- 2.48.1