[release-branch.go1.19] cmd/go/internal/work: make formatOutput return an error that includes the import path

This refines the error output that was previously adjusted in CL 437298.

Longer term, we should consider unraveling the call chains involving
formatOutput to avoid passing so many parameters through so many
different formatting functions.

Updates #60710.
Updates #60650.
Updates #25842.

Change-Id: I3b9d03bf5968902d8ccc4841ab4dbe114a2239e4
Reviewed-on: https://go-review.googlesource.com/c/go/+/451218
Reviewed-by: Bryan Mills <[email protected]>
Auto-Submit: Russ Cox <[email protected]>
Reviewed-by: Russ Cox <[email protected]>
Run-TryBot: Russ Cox <[email protected]>
Reviewed-by: Than McIntosh <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-on: https://go-review.googlesource.com/c/go/+/502196
Reviewed-by: Dmitri Shuralyov <[email protected]>
TryBot-Bypass: Bryan Mills <[email protected]>
Run-TryBot: Bryan Mills <[email protected]>
Reviewed-by: Michael Matloob <[email protected]>

Author: Bryan C. Mills

src/cmd/go/internal/work/exec.go

@@ -149,11 +149,15 @@ func (b *Builder) Do(ctx context.Context, root *Action) {
 		defer b.exec.Unlock()
 
 		if err != nil {
-			if b.AllowErrors {
+			if b.AllowErrors && a.Package != nil {
 				if a.Package.Error == nil {
 					a.Package.Error = &load.PackageError{Err: err}
 				}
 			} else {
+				var ipe load.ImportPathError
+				if a.Package != nil && (!errors.As(err, &ipe) || ipe.ImportPath() != a.Package.ImportPath) {
+					err = fmt.Errorf("%s: %v", a.Package.ImportPath, err)
+				}
 				base.Errorf("%s", err)
 			}
 			a.Failed = true
@@ -510,9 +514,6 @@ func (b *Builder) build(ctx context.Context, a *Action) (err error) {
 	}
 
 	defer func() {
-		if err != nil {
-			err = fmt.Errorf("go build %s: %v", p.ImportPath, err)
-		}
 		if err != nil && b.IsCmdList && b.NeedError && p.Error == nil {
 			p.Error = &load.PackageError{Err: err}
 		}
@@ -825,7 +826,7 @@ OverlayLoop:
 		}
 
 		if err != nil {
-			return errors.New(fmt.Sprint(formatOutput(b.WorkDir, p.Dir, p.Desc(), output)))
+			return formatOutput(b.WorkDir, p.Dir, p.ImportPath, p.Desc(), output)
 		} else {
 			b.showOutput(a, p.Dir, p.Desc(), output)
 		}
@@ -1506,8 +1507,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 {
-			prefix, suffix := formatOutput(b.WorkDir, p.Dir, b.PkgconfigCmd()+" --cflags "+strings.Join(pcflags, " ")+" -- "+strings.Join(pkgs, " "), string(out))
-			return nil, nil, errors.New(fmt.Sprint(prefix, suffix+err.Error()))
+			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
 		}
 		if len(out) > 0 {
 			cflags, err = splitPkgConfigOutput(out)
@@ -1520,8 +1521,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 {
-			prefix, suffix := formatOutput(b.WorkDir, p.Dir, b.PkgconfigCmd()+" --libs "+strings.Join(pcflags, " ")+" -- "+strings.Join(pkgs, " "), string(out))
-			return nil, nil, errors.New(fmt.Sprint(prefix, suffix+err.Error()))
+			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
 		}
 		if len(out) > 0 {
 			// NOTE: we don't attempt to parse quotes and unescapes here. pkg-config
@@ -2017,16 +2018,37 @@ func (b *Builder) Showcmd(dir string, format string, args ...any) {
 // If a is not nil and a.output is not nil, showOutput appends to that slice instead of
 // printing to b.Print.
 func (b *Builder) showOutput(a *Action, dir, desc, out string) {
-	prefix, suffix := formatOutput(b.WorkDir, dir, desc, out)
+	importPath := ""
+	if a != nil && a.Package != nil {
+		importPath = a.Package.ImportPath
+	}
+	psErr := formatOutput(b.WorkDir, dir, importPath, desc, out)
 	if a != nil && a.output != nil {
-		a.output = append(a.output, prefix...)
-		a.output = append(a.output, suffix...)
+		a.output = append(a.output, psErr.prefix...)
+		a.output = append(a.output, psErr.suffix...)
 		return
 	}
 
 	b.output.Lock()
 	defer b.output.Unlock()
-	b.Print(prefix, suffix)
+	b.Print(psErr.prefix, psErr.suffix)
+}
+
+// A prefixSuffixError is an error formatted by formatOutput.
+type prefixSuffixError struct {
+	importPath     string
+	prefix, suffix string
+}
+
+func (e *prefixSuffixError) Error() string {
+	if e.importPath != "" && !strings.HasPrefix(strings.TrimPrefix(e.prefix, "# "), e.importPath) {
+		return fmt.Sprintf("go build %s:\n%s%s", e.importPath, e.prefix, e.suffix)
+	}
+	return e.prefix + e.suffix
+}
+
+func (e *prefixSuffixError) ImportPath() string {
+	return e.importPath
 }
 
 // formatOutput prints "# desc" followed by the given output.
@@ -2052,17 +2074,17 @@ func (b *Builder) showOutput(a *Action, dir, desc, out string) {
 // formatOutput also replaces references to the work directory with $WORK.
 // formatOutput returns the output in a prefix with the description and a
 // suffix with the actual output.
-func formatOutput(workDir, dir, desc, out string) (prefix, suffix string) {
-	prefix = "# " + desc
-	suffix = "\n" + out
+func formatOutput(workDir, dir, importPath, desc, out string) *prefixSuffixError {
+	prefix := "# " + desc
+	suffix := "\n" + out
 	if reldir := base.ShortPath(dir); reldir != dir {
 		suffix = strings.ReplaceAll(suffix, " "+dir, " "+reldir)
 		suffix = strings.ReplaceAll(suffix, "\n"+dir, "\n"+reldir)
 		suffix = strings.ReplaceAll(suffix, "\n\t"+dir, "\n\t"+reldir)
 	}
 	suffix = strings.ReplaceAll(suffix, " "+workDir, " $WORK")
 
-	return prefix, suffix
+	return &prefixSuffixError{importPath: importPath, prefix: prefix, suffix: suffix}
 }
 
 var cgoLine = lazyregexp.New(`\[[^\[\]]+\.(cgo1|cover)\.go:[0-9]+(:[0-9]+)?\]`)
@@ -2078,7 +2100,7 @@ func (b *Builder) run(a *Action, dir string, desc string, env []string, cmdargs
 			desc = b.fmtcmd(dir, "%s", strings.Join(str.StringList(cmdargs...), " "))
 		}
 		if err != nil {
-			err = errors.New(fmt.Sprint(formatOutput(b.WorkDir, dir, desc, b.processOutput(out))))
+			err = formatOutput(b.WorkDir, dir, a.Package.ImportPath, desc, b.processOutput(out))
 		} else {
 			b.showOutput(a, dir, desc, b.processOutput(out))
 		}
@@ -2364,7 +2386,6 @@ func (b *Builder) gfortran(a *Action, p *load.Package, workdir, out string, flag
 // ccompile runs the given C or C++ compiler and creates an object from a single source file.
 func (b *Builder) ccompile(a *Action, p *load.Package, outfile string, flags []string, file string, compiler []string) error {
 	file = mkAbs(p.Dir, file)
-	desc := p.ImportPath
 	outfile = mkAbs(p.Dir, outfile)
 
 	// Elide source directory paths if -trimpath or GOROOT_FINAL is set.
@@ -2425,9 +2446,9 @@ func (b *Builder) ccompile(a *Action, p *load.Package, outfile string, flags []s
 		}
 
 		if err != nil || os.Getenv("GO_BUILDER_NAME") != "" {
-			err = errors.New(fmt.Sprintf(formatOutput(b.WorkDir, p.Dir, desc, b.processOutput(output))))
+			err = formatOutput(b.WorkDir, p.Dir, p.ImportPath, p.Desc(), b.processOutput(output))
 		} else {
-			b.showOutput(a, p.Dir, desc, b.processOutput(output))
+			b.showOutput(a, p.Dir, p.Desc(), b.processOutput(output))
 		}
 	}
 	return err
@@ -2890,8 +2911,6 @@ func (b *Builder) cgo(a *Action, cgoExe, objdir string, pcCFLAGS, pcLDFLAGS, cgo
 		cgoflags = append(cgoflags, "-exportheader="+objdir+"_cgo_install.h")
 	}
 
-	execdir := p.Dir
-
 	// Rewrite overlaid paths in cgo files.
 	// cgo adds //line and #line pragmas in generated files with these paths.
 	var trimpath []string
@@ -2906,7 +2925,7 @@ func (b *Builder) cgo(a *Action, cgoExe, objdir string, pcCFLAGS, pcLDFLAGS, cgo
 		cgoflags = append(cgoflags, "-trimpath", strings.Join(trimpath, ";"))
 	}
 
-	if err := b.run(a, execdir, p.ImportPath, cgoenv, cfg.BuildToolexec, cgoExe, "-objdir", objdir, "-importpath", p.ImportPath, cgoflags, "--", cgoCPPFLAGS, cgoCFLAGS, cgofiles); err != nil {
+	if err := b.run(a, p.Dir, p.ImportPath, cgoenv, cfg.BuildToolexec, cgoExe, "-objdir", objdir, "-importpath", p.ImportPath, cgoflags, "--", cgoCPPFLAGS, cgoCFLAGS, cgofiles); err != nil {
 		return nil, nil, err
 	}
 	outGo = append(outGo, gofiles...)
@@ -3382,7 +3401,7 @@ func (b *Builder) swigOne(a *Action, p *load.Package, file, objdir string, pcCFL
 				return "", "", errors.New("must have SWIG version >= 3.0.6")
 			}
 			// swig error
-			return "", "", errors.New(fmt.Sprint(formatOutput(b.WorkDir, p.Dir, p.Desc(), b.processOutput(out))))
+			err = formatOutput(b.WorkDir, p.Dir, p.ImportPath, p.Desc(), b.processOutput(out))
 		}
 		return "", "", err
 	}

src/cmd/go/internal/work/gc.go

@@ -7,7 +7,6 @@ package work
 import (
 	"bufio"
 	"bytes"
-	"errors"
 	"fmt"
 	"io"
 	"log"
@@ -503,7 +502,7 @@ func (gcToolchain) pack(b *Builder, a *Action, afile string, ofiles []string) er
 		return nil
 	}
 	if err := packInternal(absAfile, absOfiles); err != nil {
-		return errors.New(fmt.Sprint(formatOutput(b.WorkDir, p.Dir, p.Desc(), err.Error()+"\n")))
+		return formatOutput(b.WorkDir, p.Dir, p.ImportPath, p.Desc(), err.Error()+"\n")
 	}
 	return nil
 }

src/cmd/go/testdata/script/list_export_e.txt

@@ -1,6 +1,13 @@
-go list -e -export ./...
+! go list -export ./...
+stderr '^# example.com/p2\np2'${/}'main\.go:7:.*'
+! stderr '^go build '
+
+go list -f '{{with .Error}}{{.}}{{end}}' -e -export ./...
 ! stderr '.'
-go list -e -export -json ...
+stdout '^# example.com/p2\np2'${/}'main\.go:7:.*'
+
+go list -e -export -json=Error ./...
+stdout '"Err": "# example.com/p2'
 
 -- go.mod --
 module example.com
@@ -15,5 +22,5 @@ import "fmt"
 import "example.com/p1"
 
 func main() {
-        fmt.Println(p1.Name == 5)
-}
+	fmt.Println(p1.Name == 5)
+}

src/cmd/go/testdata/script/list_pkgconfig_error.txt

@@ -0,0 +1,16 @@
+[!cgo] skip 'test verifies cgo pkg-config errors'
+[!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+$'
+
+-- go.mod --
+module example
+go 1.20
+-- example.go --
+package example
+
+// #cgo pkg-config: libnot-a-valid-cgo-library
+import "C"
+
+package main() {}