From 7d6517cf420d131773852f2c5cdd6d07f6edf2ad Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Wed, 16 Nov 2022 15:36:30 -0500 Subject: [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 Auto-Submit: Russ Cox Reviewed-by: Russ Cox Run-TryBot: Russ Cox Reviewed-by: Than McIntosh TryBot-Result: Gopher Robot Reviewed-on: https://go-review.googlesource.com/c/go/+/502196 Reviewed-by: Dmitri Shuralyov TryBot-Bypass: Bryan Mills Run-TryBot: Bryan Mills Reviewed-by: Michael Matloob --- src/cmd/go/internal/work/exec.go | 69 ++++++++++++++-------- src/cmd/go/internal/work/gc.go | 3 +- src/cmd/go/testdata/script/list_export_e.txt | 15 +++-- .../go/testdata/script/list_pkgconfig_error.txt | 16 +++++ 4 files changed, 72 insertions(+), 31 deletions(-) create mode 100644 src/cmd/go/testdata/script/list_pkgconfig_error.txt diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go index ac3815511c..4840568d67 100644 --- a/src/cmd/go/internal/work/exec.go +++ b/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,9 +2074,9 @@ 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) @@ -2062,7 +2084,7 @@ func formatOutput(workDir, dir, desc, out string) (prefix, suffix string) { } 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 } diff --git a/src/cmd/go/internal/work/gc.go b/src/cmd/go/internal/work/gc.go index 3e405d448d..680ca94974 100644 --- a/src/cmd/go/internal/work/gc.go +++ b/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 } diff --git a/src/cmd/go/testdata/script/list_export_e.txt b/src/cmd/go/testdata/script/list_export_e.txt index f6992e221d..8e4c361fc4 100644 --- a/src/cmd/go/testdata/script/list_export_e.txt +++ b/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) -} \ No newline at end of file + fmt.Println(p1.Name == 5) +} diff --git a/src/cmd/go/testdata/script/list_pkgconfig_error.txt b/src/cmd/go/testdata/script/list_pkgconfig_error.txt new file mode 100644 index 0000000000..7d671a6438 --- /dev/null +++ b/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() {} -- cgit v1.2.3-54-g00ecf