Skip to content

Commit

Permalink
cmd/go: replace formatOutput/showOutput with reportCmd
Browse files Browse the repository at this point in the history 
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 <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
  • Loading branch information
@aclements
aclements committed Oct 18, 2023
1 parent 5ae9724 commit 7ee9980
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 91 deletions.
8 changes: 2 additions & 6 deletions src/cmd/go/internal/work/cover.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
122 changes: 50 additions & 72 deletions src/cmd/go/internal/work/exec.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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 --.
Expand All @@ -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))
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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")
}
}
Expand Down Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/go/internal/work/gc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
25 changes: 14 additions & 11 deletions src/cmd/go/internal/work/gccgo.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package work

import (
"bytes"
"fmt"
"os"
"os/exec"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
}
2 changes: 1 addition & 1 deletion src/cmd/go/testdata/script/list_pkgconfig_error.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 7ee9980

Please sign in to comment.