[release-branch.go1.19] cmd/go: do not exit with non-zero code from go list -e -export

go list -e -export puts errors running build actions on the load.Package
corresponding to the failed action rather than exiting with a non zero
exit code.

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

Change-Id: I1fea85cc5a0557f514fe9d4ed3b6a858376fdcde
Reviewed-on: https://go-review.googlesource.com/c/go/+/437298
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/502195
TryBot-Bypass: Bryan Mills <bcmills@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>

6 files changed, 87 insertions, 44 deletions

diff --git a/src/cmd/go/internal/list/list.go b/src/cmd/go/internal/list/list.go
index dfb58c28eb..1c61e6ec70 100644
--- a/src/cmd/go/internal/list/list.go
+++ b/src/cmd/go/internal/list/list.go
@@ -690,6 +690,9 @@ func runList(ctx context.Context, cmd *base.Command, args []string) {
 	needStale := (listJson && listJsonFields.needAny("Stale", "StaleReason")) || strings.Contains(*listFmt, ".Stale")
 	if needStale || *listExport || *listCompiled {
 		b := work.NewBuilder("")
+		if *listE {
+			b.AllowErrors = true
+		}
 		b.IsCmdList = true
 		b.NeedExport = *listExport
 		b.NeedCompiledGoFiles = *listCompiled
diff --git a/src/cmd/go/internal/work/action.go b/src/cmd/go/internal/work/action.go
index 4bbd23ab8e..422cb3bd93 100644
--- a/src/cmd/go/internal/work/action.go
+++ b/src/cmd/go/internal/work/action.go
@@ -43,6 +43,7 @@ type Builder struct {
 	NeedError           bool // list needs p.Error
 	NeedExport          bool // list needs p.Export
 	NeedCompiledGoFiles bool // list needs p.CompiledGoFiles
+	AllowErrors         bool // errors don't immediately exit the program
 
 	objdirSeq int // counter for NewObjdir
 	pkgSeq    int
diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go
index 567f045922..ac3815511c 100644
--- a/src/cmd/go/internal/work/exec.go
+++ b/src/cmd/go/internal/work/exec.go
@@ -149,8 +149,10 @@ func (b *Builder) Do(ctx context.Context, root *Action) {
 		defer b.exec.Unlock()
 
 		if err != nil {
-			if err == errPrintedOutput {
-				base.SetExitStatus(2)
+			if b.AllowErrors {
+				if a.Package.Error == nil {
+					a.Package.Error = &load.PackageError{Err: err}
+				}
 			} else {
 				base.Errorf("%s", err)
 			}
@@ -508,8 +510,8 @@ func (b *Builder) build(ctx context.Context, a *Action) (err error) {
 	}
 
 	defer func() {
-		if err != nil && err != errPrintedOutput {
-			err = fmt.Errorf("go build %s: %v", a.Package.ImportPath, err)
+		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}
@@ -821,9 +823,11 @@ OverlayLoop:
 		if p.Module != nil && !allowedVersion(p.Module.GoVersion) {
 			output += "note: module requires Go " + p.Module.GoVersion + "\n"
 		}
-		b.showOutput(a, a.Package.Dir, a.Package.Desc(), output)
+
 		if err != nil {
-			return errPrintedOutput
+			return errors.New(fmt.Sprint(formatOutput(b.WorkDir, p.Dir, p.Desc(), output)))
+		} else {
+			b.showOutput(a, p.Dir, p.Desc(), output)
 		}
 	}
 	if err != nil {
@@ -1502,9 +1506,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 {
-			b.showOutput(nil, p.Dir, b.PkgconfigCmd()+" --cflags "+strings.Join(pcflags, " ")+" -- "+strings.Join(pkgs, " "), string(out))
-			b.Print(err.Error() + "\n")
-			return nil, nil, errPrintedOutput
+			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()))
 		}
 		if len(out) > 0 {
 			cflags, err = splitPkgConfigOutput(out)
@@ -1517,9 +1520,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 {
-			b.showOutput(nil, p.Dir, b.PkgconfigCmd()+" --libs "+strings.Join(pcflags, " ")+" -- "+strings.Join(pkgs, " "), string(out))
-			b.Print(err.Error() + "\n")
-			return nil, nil, errPrintedOutput
+			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()))
 		}
 		if len(out) > 0 {
 			// NOTE: we don't attempt to parse quotes and unescapes here. pkg-config
@@ -1611,7 +1613,7 @@ func (b *Builder) linkShared(ctx context.Context, a *Action) (err error) {
 // BuildInstallFunc is the action for installing a single package or executable.
 func BuildInstallFunc(b *Builder, ctx context.Context, a *Action) (err error) {
 	defer func() {
-		if err != nil && err != errPrintedOutput {
+		if err != nil {
 			// a.Package == nil is possible for the go install -buildmode=shared
 			// action that installs libmangledname.so, which corresponds to
 			// a list of packages, not just one.
@@ -2015,15 +2017,7 @@ 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 := "# " + 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, " "+b.WorkDir, " $WORK")
-
+	prefix, suffix := formatOutput(b.WorkDir, dir, desc, out)
 	if a != nil && a.output != nil {
 		a.output = append(a.output, prefix...)
 		a.output = append(a.output, suffix...)
@@ -2035,12 +2029,41 @@ func (b *Builder) showOutput(a *Action, dir, desc, out string) {
 	b.Print(prefix, suffix)
 }
 
-// errPrintedOutput is a special error indicating that a command failed
-// but that it generated output as well, and that output has already
-// been printed, so there's no point showing 'exit status 1' or whatever
-// the wait status was. The main executor, builder.do, knows not to
-// print this error.
-var errPrintedOutput = errors.New("already printed output - no need to show error")
+// formatOutput prints "# desc" followed by the given output.
+// The output is expected to contain references to 'dir', usually
+// the source directory for the package that has failed to build.
+// formatOutput rewrites mentions of dir with a relative path to dir
+// when the relative path is shorter. This is usually more pleasant.
+// For example, if fmt doesn't compile and we are in src/html,
+// the output is
+//
+//	$ go build
+//	# fmt
+//	../fmt/print.go:1090: undefined: asdf
+//	$
+//
+// instead of
+//
+//	$ go build
+//	# fmt
+//	/usr/gopher/go/src/fmt/print.go:1090: undefined: asdf
+//	$
+//
+// 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
+	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
+}
 
 var cgoLine = lazyregexp.New(`\[[^\[\]]+\.(cgo1|cover)\.go:[0-9]+(:[0-9]+)?\]`)
 var cgoTypeSigRe = lazyregexp.New(`\b_C2?(type|func|var|macro)_\B`)
@@ -2054,9 +2077,10 @@ func (b *Builder) run(a *Action, dir string, desc string, env []string, cmdargs
 		if desc == "" {
 			desc = b.fmtcmd(dir, "%s", strings.Join(str.StringList(cmdargs...), " "))
 		}
-		b.showOutput(a, dir, desc, b.processOutput(out))
 		if err != nil {
-			err = errPrintedOutput
+			err = errors.New(fmt.Sprint(formatOutput(b.WorkDir, dir, desc, b.processOutput(out))))
+		} else {
+			b.showOutput(a, dir, desc, b.processOutput(out))
 		}
 	}
 	return err
@@ -2400,11 +2424,10 @@ func (b *Builder) ccompile(a *Action, p *load.Package, outfile string, flags []s
 			}
 		}
 
-		b.showOutput(a, p.Dir, desc, b.processOutput(output))
-		if err != nil {
-			err = errPrintedOutput
-		} else if os.Getenv("GO_BUILDER_NAME") != "" {
-			return errors.New("C compiler warning promoted to error on Go builders")
+		if err != nil || os.Getenv("GO_BUILDER_NAME") != "" {
+			err = errors.New(fmt.Sprintf(formatOutput(b.WorkDir, p.Dir, desc, b.processOutput(output))))
+		} else {
+			b.showOutput(a, p.Dir, desc, b.processOutput(output))
 		}
 	}
 	return err
@@ -3358,8 +3381,8 @@ func (b *Builder) swigOne(a *Action, p *load.Package, file, objdir string, pcCFL
 			if bytes.Contains(out, []byte("-intgosize")) || bytes.Contains(out, []byte("-cgo")) {
 				return "", "", errors.New("must have SWIG version >= 3.0.6")
 			}
-			b.showOutput(a, p.Dir, p.Desc(), b.processOutput(out)) // swig error
-			return "", "", errPrintedOutput
+			// swig error
+			return "", "", errors.New(fmt.Sprint(formatOutput(b.WorkDir, p.Dir, p.Desc(), b.processOutput(out))))
 		}
 		return "", "", err
 	}
diff --git a/src/cmd/go/internal/work/gc.go b/src/cmd/go/internal/work/gc.go
index 8429529115..3e405d448d 100644
--- a/src/cmd/go/internal/work/gc.go
+++ b/src/cmd/go/internal/work/gc.go
@@ -7,6 +7,7 @@ package work
 import (
 	"bufio"
 	"bytes"
+	"errors"
 	"fmt"
 	"io"
 	"log"
@@ -502,8 +503,7 @@ func (gcToolchain) pack(b *Builder, a *Action, afile string, ofiles []string) er
 		return nil
 	}
 	if err := packInternal(absAfile, absOfiles); err != nil {
-		b.showOutput(a, p.Dir, p.Desc(), err.Error()+"\n")
-		return errPrintedOutput
+		return errors.New(fmt.Sprint(formatOutput(b.WorkDir, p.Dir, p.Desc(), err.Error()+"\n")))
 	}
 	return nil
 }
diff --git a/src/cmd/go/testdata/script/build_cwd_newline.txt b/src/cmd/go/testdata/script/build_cwd_newline.txt
index e80235698a..87d8b344e2 100644
--- a/src/cmd/go/testdata/script/build_cwd_newline.txt
+++ b/src/cmd/go/testdata/script/build_cwd_newline.txt
@@ -35,12 +35,9 @@ stderr 'package example: invalid package directory .*uh-oh'
 [!cgo] ! go test -v main_nocgo.go main_test.go
 stderr 'package command-line-arguments: invalid package directory .*uh-oh'
 
-	# Note: this command fails on release-branch.go1.19 but succeeds
-	# on release-branch.go1.20 and later. I (bcmills) suspect it may
-	# have been fixed by CL 437298.
-! go list -compiled -e -f '{{with .CompiledGoFiles}}{{.}}{{end}}' .
-stderr 'package example: invalid package directory .*uh-oh'
+go list -compiled -e -f '{{with .CompiledGoFiles}}{{.}}{{end}}' .
 ! stdout .
+! stderr .
 ! exists obj_
 
 # The cgo tool should only accept the source file if the working directory
diff --git a/src/cmd/go/testdata/script/list_export_e.txt b/src/cmd/go/testdata/script/list_export_e.txt
new file mode 100644
index 0000000000..f6992e221d
--- /dev/null
+++ b/src/cmd/go/testdata/script/list_export_e.txt
@@ -0,0 +1,19 @@
+go list -e -export ./...
+! stderr '.'
+go list -e -export -json ...
+
+-- go.mod --
+module example.com
+-- p1/p1.go --
+package p1
+
+const Name = "p1"
+-- p2/main.go --
+package main
+
+import "fmt"
+import "example.com/p1"
+
+func main() {
+        fmt.Println(p1.Name == 5)
+}
\ No newline at end of file