]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go: consolidate showOutput, formatOutput, and processOutput into reportCmd
authorAustin Clements <austin@google.com>
Tue, 5 Sep 2023 15:07:29 +0000 (11:07 -0400)
committerAustin Clements <austin@google.com>
Wed, 18 Oct 2023 14:06:30 +0000 (14:06 +0000)
Many uses of showOutput, formatOutput, and processOutput follow a very
similar (somewhat complex) pattern. Places that diverge from this
pattern are often minor bugs. Furthermore, the roles of formatOutput
and processOutput have somewhat blurred over time; e.g., formatOutput
performs directory shortening, while processOutput performs cgo
demangling.

This CL consolidates all of this logic into a single, new function:
Builder.reportCmd.

In the following CL, we'll replace all calls of the three original
functions with reportCmd.

In addition to being a nice cleanup, this puts us in a much better
position to change how build output is formatted in order to support
`go build -json`.

For #62067.

Change-Id: I733162825377d82d0015c8aae2820e56a1b32958
Reviewed-on: https://go-review.googlesource.com/c/go/+/529218
Reviewed-by: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

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

index 89e67314dfc6d491d368d16130ac294d34e16fa6..eecdecaa053635fb6ee698b1b0f14e98366a1790 100644 (file)
@@ -2198,6 +2198,191 @@ func (b *Builder) Showcmd(dir string, format string, args ...any) {
        b.Print(b.fmtcmd(dir, format, args...) + "\n")
 }
 
+// reportCmd reports the output and exit status of a command. The cmdOut and
+// cmdErr arguments are the output and exit error of the command, respectively.
+//
+// The exact reporting behavior is as follows:
+//
+//     cmdOut  cmdErr  Result
+//     ""      nil     print nothing, return nil
+//     !=""    nil     print output, return nil
+//     ""      !=nil   print nothing, return cmdErr (later printed)
+//     !=""    !=nil   print nothing, ignore err, return output as error (later printed)
+//
+// reportCmd returns a non-nil error if and only if cmdErr != nil. It assumes
+// that the command output, if non-empty, is more detailed than the command
+// error (which is usually just an exit status), so prefers using the output as
+// the ultimate error. Typically, the caller should return this error from an
+// Action, which it will be printed by the Builder.
+//
+// reportCmd formats the output as "# 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. reportCmd 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
+//     $
+//
+// reportCmd also replaces references to the work directory with $WORK, replaces
+// cgo file paths with the original file path, and replaces cgo-mangled names
+// with "C.name".
+//
+// p is optional. If nil, a.Package is used.
+//
+// desc is optional. If "", p.Desc() is used.
+//
+// dir is optional. If "", p.Dir is used.
+func (b *Builder) reportCmd(a *Action, p *load.Package, desc, dir string, cmdOut []byte, cmdErr error) error {
+       // TODO: It seems we can always get p from a.Package, so it should be
+       // possible to drop the "p" argument. However, a lot of callers take both
+       // Action and Package, so we'd want to drop the Package argument from those,
+       // too.
+       if len(cmdOut) == 0 && cmdErr == nil {
+               // Common case
+               return nil
+       }
+       if len(cmdOut) == 0 && cmdErr != nil {
+               // Just return the error.
+               //
+               // TODO: This is what we've done for a long time, but it may be a
+               // mistake because it loses all of the extra context and results in
+               // ultimately less descriptive output. We should probably just take the
+               // text of cmdErr as the output in this case and do everything we
+               // otherwise would. We could chain the errors if we feel like it.
+               return cmdErr
+       }
+
+       // Fetch defaults from the package.
+       if a != nil && p == nil {
+               p = a.Package
+       }
+       var importPath string
+       if p != nil {
+               importPath = p.ImportPath
+               if desc == "" {
+                       desc = p.Desc()
+               }
+               if dir == "" {
+                       dir = p.Dir
+               }
+       }
+
+       out := string(cmdOut)
+
+       if !strings.HasSuffix(out, "\n") {
+               out = out + "\n"
+       }
+
+       // Replace workDir with $WORK
+       out = replacePrefix(out, b.WorkDir, "$WORK")
+
+       // Rewrite mentions of dir with a relative path to dir
+       // when the relative path is shorter.
+       for {
+               // Note that dir starts out long, something like
+               // /foo/bar/baz/root/a
+               // The target string to be reduced is something like
+               // (blah-blah-blah) /foo/bar/baz/root/sibling/whatever.go:blah:blah
+               // /foo/bar/baz/root/a doesn't match /foo/bar/baz/root/sibling, but the prefix
+               // /foo/bar/baz/root does.  And there may be other niblings sharing shorter
+               // prefixes, the only way to find them is to look.
+               // This doesn't always produce a relative path --
+               // /foo is shorter than ../../.., for example.
+               if reldir := base.ShortPath(dir); reldir != dir {
+                       out = replacePrefix(out, dir, reldir)
+                       if filepath.Separator == '\\' {
+                               // Don't know why, sometimes this comes out with slashes, not backslashes.
+                               wdir := strings.ReplaceAll(dir, "\\", "/")
+                               out = replacePrefix(out, wdir, reldir)
+                       }
+               }
+               dirP := filepath.Dir(dir)
+               if dir == dirP {
+                       break
+               }
+               dir = dirP
+       }
+
+       // Fix up output referring to cgo-generated code to be more readable.
+       // Replace x.go:19[/tmp/.../x.cgo1.go:18] with x.go:19.
+       // Replace *[100]_Ctype_foo with *[100]C.foo.
+       // If we're using -x, assume we're debugging and want the full dump, so disable the rewrite.
+       if !cfg.BuildX && cgoLine.MatchString(out) {
+               out = cgoLine.ReplaceAllString(out, "")
+               out = cgoTypeSigRe.ReplaceAllString(out, "C.")
+       }
+
+       err := &cmdError{desc, out, importPath}
+       if cmdErr != nil {
+               // The command failed. Report the output up as an error.
+               return err
+       }
+       // The command didn't fail, so just print the output as appropriate.
+       if a != nil && a.output != nil {
+               // The Action is capturing output.
+               a.output = append(a.output, err.Error()...)
+       } else {
+               // Write directly to the Builder output.
+               b.output.Lock()
+               defer b.output.Unlock()
+               b.Print(err.Error())
+       }
+       return nil
+}
+
+// replacePrefix is like strings.ReplaceAll, but only replaces instances of old
+// that are preceded by ' ', '\t', or appear at the beginning of a line.
+func replacePrefix(s, old, new string) string {
+       n := strings.Count(s, old)
+       if n == 0 {
+               return s
+       }
+
+       s = strings.ReplaceAll(s, " "+old, " "+new)
+       s = strings.ReplaceAll(s, "\n"+old, "\n"+new)
+       s = strings.ReplaceAll(s, "\n\t"+old, "\n\t"+new)
+       if strings.HasPrefix(s, old) {
+               s = new + s[len(old):]
+       }
+       return s
+}
+
+type cmdError struct {
+       desc       string
+       text       string
+       importPath string
+}
+
+func (e *cmdError) Error() string {
+       msg := "# " + e.desc + "\n" + e.text
+       if e.importPath != "" && !strings.HasPrefix(e.desc, e.importPath) {
+               // Ensure the import path is part of the message. We checked the prefix
+               // because desc can be a package ID, which may have text in addition to
+               // the import path.
+               //
+               // TODO(austin): Checking the prefix seems flimsy. reportCmd could
+               // instead check if desc != p.Desc() and leave a flag in cmdError to
+               // signal this code path.
+               msg = fmt.Sprintf("go build %s:\n%s", e.importPath, msg)
+       }
+       return msg
+}
+
+func (e *cmdError) ImportPath() string {
+       return e.importPath
+}
+
 // showOutput 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.