Skip to content

Commit

Permalink
all: consolidate invokeGo implementations
Browse files Browse the repository at this point in the history 
Over time we have accumulated a disturbing number of ways to run the Go
command, each of which supported slightly different features and
workarounds. Combine all of them.

This unavoidably introduces some changes in debug logging behavior; I
think we should try to fix them incrementally and consistently.

Updates golang/go#37368.

Change-Id: I664ca8685bf247a226be3cb807789c2fcddf233d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220737
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
  • Loading branch information
@heschi
heschi committed Feb 25, 2020
1 parent ee3f114 commit c5cec67
Show file tree
Hide file tree
Showing 12 changed files with 206 additions and 225 deletions.
102 changes: 23 additions & 79 deletions go/internal/packagesdriver/sizes.go
View file
Expand Up @@ -11,11 +11,10 @@ import (
"encoding/json"
"fmt"
"go/types"
"log"
"os"
"os/exec"
"strings"
"time"

"golang.org/x/tools/internal/gocommand"
)

var debug = false
Expand Down Expand Up @@ -78,97 +77,42 @@ func GetSizes(ctx context.Context, buildFlags, env []string, dir string, usesExp
}

func GetSizesGolist(ctx context.Context, buildFlags, env []string, dir string, usesExportData bool) (types.Sizes, error) {
args := []string{"list", "-f", "{{context.GOARCH}} {{context.Compiler}}"}
args = append(args, buildFlags...)
args = append(args, "--", "unsafe")
stdout, stderr, err := invokeGo(ctx, env, dir, usesExportData, args...)
inv := gocommand.Invocation{
Verb: "list",
Args: []string{"-f", "{{context.GOARCH}} {{context.Compiler}}", "--", "unsafe"},
Env: env,
BuildFlags: buildFlags,
WorkingDir: dir,
}
stdout, stderr, friendlyErr, rawErr := inv.RunRaw(ctx)
var goarch, compiler string
if err != nil {
if strings.Contains(err.Error(), "cannot find main module") {
if rawErr != nil {
if strings.Contains(rawErr.Error(), "cannot find main module") {
// User's running outside of a module. All bets are off. Get GOARCH and guess compiler is gc.
// TODO(matloob): Is this a problem in practice?
envout, _, enverr := invokeGo(ctx, env, dir, usesExportData, "env", "GOARCH")
inv := gocommand.Invocation{
Verb: "env",
Args: []string{"GOARCH"},
Env: env,
WorkingDir: dir,
}
envout, enverr := inv.Run(ctx)
if enverr != nil {
return nil, err
return nil, enverr
}
goarch = strings.TrimSpace(envout.String())
compiler = "gc"
} else {
return nil, err
return nil, friendlyErr
}
} else {
fields := strings.Fields(stdout.String())
if len(fields) < 2 {
return nil, fmt.Errorf("could not parse GOARCH and Go compiler in format \"<GOARCH> <compiler>\" from stdout of go command:\n%s\ndir: %s\nstdout: <<%s>>\nstderr: <<%s>>",
cmdDebugStr(env, args...), dir, stdout.String(), stderr.String())
return nil, fmt.Errorf("could not parse GOARCH and Go compiler in format \"<GOARCH> <compiler>\":\nstdout: <<%s>>\nstderr: <<%s>>",
stdout.String(), stderr.String())
}
goarch = fields[0]
compiler = fields[1]
}
return types.SizesFor(compiler, goarch), nil
}

// invokeGo returns the stdout and stderr of a go command invocation.
func invokeGo(ctx context.Context, env []string, dir string, usesExportData bool, args ...string) (*bytes.Buffer, *bytes.Buffer, error) {
if debug {
defer func(start time.Time) { log.Printf("%s for %v", time.Since(start), cmdDebugStr(env, args...)) }(time.Now())
}
stdout := new(bytes.Buffer)
stderr := new(bytes.Buffer)
cmd := exec.CommandContext(ctx, "go", args...)
// On darwin the cwd gets resolved to the real path, which breaks anything that
// expects the working directory to keep the original path, including the
// go command when dealing with modules.
// The Go stdlib has a special feature where if the cwd and the PWD are the
// same node then it trusts the PWD, so by setting it in the env for the child
// process we fix up all the paths returned by the go command.
cmd.Env = append(append([]string{}, env...), "PWD="+dir)
cmd.Dir = dir
cmd.Stdout = stdout
cmd.Stderr = stderr
if err := cmd.Run(); err != nil {
exitErr, ok := err.(*exec.ExitError)
if !ok {
// Catastrophic error:
// - executable not found
// - context cancellation
return nil, nil, fmt.Errorf("couldn't exec 'go %v': %s %T", args, err, err)
}

// Export mode entails a build.
// If that build fails, errors appear on stderr
// (despite the -e flag) and the Export field is blank.
// Do not fail in that case.
if !usesExportData {
return nil, nil, fmt.Errorf("go %v: %s: %s", args, exitErr, stderr)
}
}

// As of writing, go list -export prints some non-fatal compilation
// errors to stderr, even with -e set. We would prefer that it put
// them in the Package.Error JSON (see https://golang.org/issue/26319).
// In the meantime, there's nowhere good to put them, but they can
// be useful for debugging. Print them if $GOPACKAGESPRINTGOLISTERRORS
// is set.
if len(stderr.Bytes()) != 0 && os.Getenv("GOPACKAGESPRINTGOLISTERRORS") != "" {
fmt.Fprintf(os.Stderr, "%s stderr: <<%s>>\n", cmdDebugStr(env, args...), stderr)
}

// debugging
if false {
fmt.Fprintf(os.Stderr, "%s stdout: <<%s>>\n", cmdDebugStr(env, args...), stdout)
}

return stdout, stderr, nil
}

func cmdDebugStr(envlist []string, args ...string) string {
env := make(map[string]string)
for _, kv := range envlist {
split := strings.Split(kv, "=")
k, v := split[0], split[1]
env[k] = v
}

return fmt.Sprintf("GOROOT=%v GOPATH=%v GO111MODULE=%v PWD=%v go %v", env["GOROOT"], env["GOPATH"], env["GO111MODULE"], env["PWD"], args)
}
48 changes: 13 additions & 35 deletions go/packages/golist.go
View file
Expand Up @@ -20,10 +20,10 @@ import (
"strconv"
"strings"
"sync"
"time"
"unicode"

"golang.org/x/tools/go/internal/packagesdriver"
"golang.org/x/tools/internal/gocommand"
"golang.org/x/tools/internal/packagesinternal"
)

Expand Down Expand Up @@ -707,29 +707,17 @@ func golistargs(cfg *Config, words []string) []string {
func (state *golistState) invokeGo(verb string, args ...string) (*bytes.Buffer, error) {
cfg := state.cfg

stdout := new(bytes.Buffer)
stderr := new(bytes.Buffer)
goArgs := []string{verb}
if verb != "env" {
goArgs = append(goArgs, cfg.BuildFlags...)
}
goArgs = append(goArgs, args...)
cmd := exec.CommandContext(state.ctx, "go", goArgs...)
// On darwin the cwd gets resolved to the real path, which breaks anything that
// expects the working directory to keep the original path, including the
// go command when dealing with modules.
// The Go stdlib has a special feature where if the cwd and the PWD are the
// same node then it trusts the PWD, so by setting it in the env for the child
// process we fix up all the paths returned by the go command.
cmd.Env = append(append([]string{}, cfg.Env...), "PWD="+cfg.Dir)
cmd.Dir = cfg.Dir
cmd.Stdout = stdout
cmd.Stderr = stderr
defer func(start time.Time) {
cfg.Logf("%s for %v, stderr: <<%s>> stdout: <<%s>>\n", time.Since(start), cmdDebugStr(cmd, goArgs...), stderr, stdout)
}(time.Now())

if err := cmd.Run(); err != nil {
inv := &gocommand.Invocation{
Verb: verb,
Args: args,
BuildFlags: cfg.BuildFlags,
Env: cfg.Env,
Logf: cfg.Logf,
WorkingDir: cfg.Dir,
}

stdout, stderr, _, err := inv.RunRaw(cfg.Context)

This comment has been minimized.

Sign in to view

Copy link
@jirfag

jirfag May 18, 2020 •
edited

Contributor

I think we shouldn't ignore friendlyErr here (3-rd return value) because of the case with context deadline.

This comment has been minimized.

Sign in to view

Copy link
@heschi

heschi May 18, 2020

Author Contributor

I don't know what case you're referring to. This code needs to work with the raw error for the type assertions below, and it explicitly mentions context cancellation on 729 as a catastrophic error. If you have a specific concern can you explain what it is?

This comment has been minimized.

Sign in to view

Copy link
@jirfag

jirfag May 18, 2020 •
edited

Contributor

I don't see cancellation checks here:

Code 
        stdout, stderr, _, err := gocmdRunner.RunRaw(cfg.Context, inv)
        if err != nil {
                // Check for 'go' executable not being found.
                if ee, ok := err.(*exec.Error); ok && ee.Err == exec.ErrNotFound {
                        return nil, fmt.Errorf("'go list' driver requires 'go', but %s", exec.ErrNotFound)
                }

                exitErr, ok := err.(*exec.ExitError)
                if !ok {
                        // Catastrophic error:
                        // - context cancellation
                        return nil, xerrors.Errorf("couldn't run 'go': %w", err)
                }

                // Old go version?
                if strings.Contains(stderr.String(), "flag provided but not defined") {
                        return nil, goTooOldError{fmt.Errorf("unsupported version of go: %s: %s", exitErr, stderr)}
                }

                // Related to #24854
                if len(stderr.String()) > 0 && strings.Contains(stderr.String(), "unexpected directory layout") {
                        return nil, fmt.Errorf("%s", stderr.String())
                }

                // Is there an error running the C compiler in cgo? This will be reported in the "Error" field
                // and should be suppressed by go list -e.
                //
                // This condition is not perfect yet because the error message can include other error messages than runtime/cgo.
               isPkgPathRune := func(r rune) bool {
                        // From https://golang.org/ref/spec#Import_declarations:
                        //    Implementation restriction: A compiler may restrict ImportPaths to non-empty strings
                        //    using only characters belonging to Unicode's L, M, N, P, and S general categories
                        //    (the Graphic characters without spaces) and may also exclude the
                        //    characters !"#$%&'()*,:;<=>?[\]^`{|} and the Unicode replacement character U+FFFD.
                        return unicode.IsOneOf([]*unicode.RangeTable{unicode.L, unicode.M, unicode.N, unicode.P, unicode.S}, r) &&
                                !strings.ContainsRune("!\"#$%&'()*,:;<=>?[\\]^`{|}\uFFFD", r)
                }
                if len(stderr.String()) > 0 && strings.HasPrefix(stderr.String(), "# ") {
                        msg := stderr.String()[len("# "):]
                        if strings.HasPrefix(strings.TrimLeftFunc(msg, isPkgPathRune), "\n") {
                                return stdout, nil
                        }
                        // Treat pkg-config errors as a special case (golang.org/issue/36770).
                        if strings.HasPrefix(msg, "pkg-config") {
                                return stdout, nil
                        }
                }

                // This error only appears in stderr. See golang.org/cl/166398 for a fix in go list to show
                // the error in the Err section of stdout in case -e option is provided.
                // This fix is provided for backwards compatibility.
                if len(stderr.String()) > 0 && strings.Contains(stderr.String(), "named files must be .go files") {
                        output := fmt.Sprintf(`{"ImportPath": "command-line-arguments","Incomplete": true,"Error": {"Pos": "","Err": %q}}`,
                                strings.Trim(stderr.String(), "\n"))
                        return bytes.NewBufferString(output), nil
                }

                // Similar to the previous error, but currently lacks a fix in Go.
                if len(stderr.String()) > 0 && strings.Contains(stderr.String(), "named files must all be in one directory") {
                        output := fmt.Sprintf(`{"ImportPath": "command-line-arguments","Incomplete": true,"Error": {"Pos": "","Err": %q}}`,
                                strings.Trim(stderr.String(), "\n"))
                        return bytes.NewBufferString(output), nil
                }

                // Backwards compatibility for Go 1.11 because 1.12 and 1.13 put the directory in the ImportPath.
                // If the package doesn't exist, put the absolute path of the directory into the error message,
                // as Go 1.13 list does.
                const noSuchDirectory = "no such directory"
                if len(stderr.String()) > 0 && strings.Contains(stderr.String(), noSuchDirectory) {
                      abspath := strings.TrimSpace(errstr[strings.Index(errstr, noSuchDirectory)+len(noSuchDirectory):])
                        output := fmt.Sprintf(`{"ImportPath": %q,"Incomplete": true,"Error": {"Pos": "","Err": %q}}`,
                                abspath, strings.Trim(stderr.String(), "\n"))
                        return bytes.NewBufferString(output), nil
                }

                // Workaround for #29280: go list -e has incorrect behavior when an ad-hoc package doesn't exist.
                // Note that the error message we look for in this case is different that the one looked for above.
                if len(stderr.String()) > 0 && strings.Contains(stderr.String(), "no such file or directory") {
                        output := fmt.Sprintf(`{"ImportPath": "command-line-arguments","Incomplete": true,"Error": {"Pos": "","Err": %q}}`,
                                strings.Trim(stderr.String(), "\n"))
                        return bytes.NewBufferString(output), nil
                }

                // Workaround for #34273. go list -e with GO111MODULE=on has incorrect behavior when listing a
                // directory outside any module.
                if len(stderr.String()) > 0 && strings.Contains(stderr.String(), "outside available modules") {
                        output := fmt.Sprintf(`{"ImportPath": %q,"Incomplete": true,"Error": {"Pos": "","Err": %q}}`,
                                // TODO(matloob): command-line-arguments isn't correct here.
                                "command-line-arguments", strings.Trim(stderr.String(), "\n"))
                        return bytes.NewBufferString(output), nil
                }

                // Another variation of the previous error
                if len(stderr.String()) > 0 && strings.Contains(stderr.String(), "outside module root") {
                        output := fmt.Sprintf(`{"ImportPath": %q,"Incomplete": true,"Error": {"Pos": "","Err": %q}}`,
                                // TODO(matloob): command-line-arguments isn't correct here.
                                "command-line-arguments", strings.Trim(stderr.String(), "\n"))
                        return bytes.NewBufferString(output), nil
                }

                // Workaround for an instance of golang.org/issue/26755: go list -e  will return a non-zero exit
                // status if there's a dependency on a package that doesn't exist. But it should return
                // a zero exit status and set an error on that package.
                if len(stderr.String()) > 0 && strings.Contains(stderr.String(), "no Go files in") {
                        // Don't clobber stdout if `go list` actually returned something.
                        if len(stdout.String()) > 0 {
                                return stdout, nil
                        }
                        // try to extract package name from string
                        stderrStr := stderr.String()
                       var importPath string
                        colon := strings.Index(stderrStr, ":")
                        if colon > 0 && strings.HasPrefix(stderrStr, "go build ") {
                                importPath = stderrStr[len("go build "):colon]
                        }
                        output := fmt.Sprintf(`{"ImportPath": %q,"Incomplete": true,"Error": {"Pos": "","Err": %q}}`,
                                importPath, strings.Trim(stderrStr, "\n"))
                        return bytes.NewBufferString(output), nil
                }

                // Export mode entails a build.
                // If that build fails, errors appear on stderr
                // (despite the -e flag) and the Export field is blank.
                // Do not fail in that case.
                // The same is true if an ad-hoc package given to go list doesn't exist.
                // TODO(matloob): Remove these once we can depend on go list to exit with a zero status with -e even when
                // packages don't exist or a build fails.
                if !usesExportData(cfg) && !containsGoFile(args) {
                        return nil, fmt.Errorf("go %v: %s: %s", args, exitErr, stderr)
                }
        }

But inv.RunRaw checks for cancellation error inside and returns a special friendlyErr:

if ctx.Err() != nil {
  friendlyError = ctx.Err()
}

I've described a specific bug here. go/packages returns empty list of packages with no error instead of context cancellation error.

UPDATE: got it. This code doesn't work as expected in my case:

                exitErr, ok := err.(*exec.ExitError)
                if !ok {
                        // Catastrophic error:
                        // - context cancellation
                        return nil, xerrors.Errorf("couldn't run 'go': %w", err)
                }

err is an instance of *exec.ExitError and code returns no error ignoring that friendlyErr contains context deadline error:

(dlv) p err
error(*os/exec.ExitError) *{
        ProcessState: *os.ProcessState {
                pid: 48020,
                status: 2,
                rusage: *(*syscall.Rusage)(0xc00052ebd0),},
        Stderr: []uint8 len: 0, cap: 0, nil,}

This comment has been minimized.

Sign in to view

Copy link
@heschi

heschi May 18, 2020

Author Contributor

If you, as a user of go/packages, want to handle context cancellation specially, I suggest you check ctx.Error -- go/packages does not currently have any error handling discipline that would guarantee that ContextCanceled/DeadlineExceeded are returned directly, and does not use error wrapping.

If you want to report a bug, please file an issue, and make sure to include what you expect to see in it.

This comment has been minimized.

Sign in to view

Copy link
@jirfag

jirfag May 18, 2020

Contributor

thank you, I will check ctx.Err directly in my code
if err != nil {
// Check for 'go' executable not being found.
if ee, ok := err.(*exec.Error); ok && ee.Err == exec.ErrNotFound {
return nil, fmt.Errorf("'go list' driver requires 'go', but %s", exec.ErrNotFound)
Expand All @@ -739,7 +727,7 @@ func (state *golistState) invokeGo(verb string, args ...string) (*bytes.Buffer,
if !ok {
// Catastrophic error:
// - context cancellation
return nil, fmt.Errorf("couldn't exec 'go %v': %s %T", args, err, err)
return nil, fmt.Errorf("couldn't run 'go': %v", err)
}

// Old go version?
Expand Down Expand Up @@ -860,16 +848,6 @@ func (state *golistState) invokeGo(verb string, args ...string) (*bytes.Buffer,
return nil, fmt.Errorf("go %v: %s: %s", args, exitErr, stderr)
}
}

// As of writing, go list -export prints some non-fatal compilation
// errors to stderr, even with -e set. We would prefer that it put
// them in the Package.Error JSON (see https://golang.org/issue/26319).
// In the meantime, there's nowhere good to put them, but they can
// be useful for debugging. Print them if $GOPACKAGESPRINTGOLISTERRORS
// is set.
if len(stderr.Bytes()) != 0 && os.Getenv("GOPACKAGESPRINTGOLISTERRORS") != "" {
fmt.Fprintf(os.Stderr, "%s stderr: <<%s>>\n", cmdDebugStr(cmd, args...), stderr)
}
return stdout, nil
}

Expand Down
28 changes: 10 additions & 18 deletions go/packages/packagestest/modules.go
View file
Expand Up @@ -6,17 +6,16 @@ package packagestest

import (
"archive/zip"
"bytes"
"context"
"fmt"
"io/ioutil"
"os"
"os/exec"
"path"
"path/filepath"
"regexp"
"strings"

"golang.org/x/tools/go/packages"
"golang.org/x/tools/internal/gocommand"
)

// Modules is the exporter that produces module layouts.
Expand Down Expand Up @@ -170,7 +169,14 @@ func (modules) Finalize(exported *Exported) error {

// Run go mod download to recreate the mod cache dir with all the extra
// stuff in cache. All the files created by Export should be recreated.
if err := invokeGo(exported.Config, "mod", "download"); err != nil {
inv := gocommand.Invocation{
Verb: "mod",
Args: []string{"download"},
Env: exported.Config.Env,
BuildFlags: exported.Config.BuildFlags,
WorkingDir: exported.Config.Dir,
}
if _, err := inv.Run(context.Background()); err != nil {
return err
}
return nil
Expand Down Expand Up @@ -237,20 +243,6 @@ func writeModuleProxy(dir, module, ver string, files map[string]string) error {
return nil
}

func invokeGo(cfg *packages.Config, args ...string) error {
stdout := new(bytes.Buffer)
stderr := new(bytes.Buffer)
cmd := exec.Command("go", args...)
cmd.Env = append(append([]string{}, cfg.Env...), "PWD="+cfg.Dir)
cmd.Dir = cfg.Dir
cmd.Stdout = stdout
cmd.Stderr = stderr
if err := cmd.Run(); err != nil {
return fmt.Errorf("go %v: %s: %s", args, err, stderr)
}
return nil
}

func modCache(exported *Exported) string {
return filepath.Join(exported.temp, "modcache/pkg/mod")
}
Expand Down
92 changes: 92 additions & 0 deletions internal/gocommand/invoke.go
View file
@@ -0,0 +1,92 @@
// Package gocommand is a helper for calling the go command.
package gocommand

import (
"bytes"
"context"
"fmt"
"os/exec"
"strings"
"time"
)

// An Invocation represents a call to the go command.
type Invocation struct {
Verb string
Args []string
BuildFlags []string
Env []string
WorkingDir string
Logf func(format string, args ...interface{})
}

// Run runs the invocation, returning its stdout and an error suitable for
// human consumption, including stderr.
func (i *Invocation) Run(ctx context.Context) (*bytes.Buffer, error) {
stdout, _, friendly, _ := i.RunRaw(ctx)
return stdout, friendly
}

// RunRaw is like Run, but also returns the raw stderr and error for callers
// that want to do low-level error handling/recovery.
func (i *Invocation) RunRaw(ctx context.Context) (stdout *bytes.Buffer, stderr *bytes.Buffer, friendlyError error, rawError error) {
log := i.Logf
if log == nil {
log = func(string, ...interface{}) {}
}

goArgs := []string{i.Verb}
switch i.Verb {
case "mod":
// mod needs the sub-verb before build flags.
goArgs = append(goArgs, i.Args[0])
goArgs = append(goArgs, i.BuildFlags...)
goArgs = append(goArgs, i.Args[1:]...)
case "env":
// env doesn't take build flags.
goArgs = append(goArgs, i.Args...)
default:
goArgs = append(goArgs, i.BuildFlags...)
goArgs = append(goArgs, i.Args...)
}
cmd := exec.CommandContext(ctx, "go", goArgs...)
stdout = &bytes.Buffer{}
stderr = &bytes.Buffer{}
cmd.Stdout = stdout
cmd.Stderr = stderr
// On darwin the cwd gets resolved to the real path, which breaks anything that
// expects the working directory to keep the original path, including the
// go command when dealing with modules.
// The Go stdlib has a special feature where if the cwd and the PWD are the
// same node then it trusts the PWD, so by setting it in the env for the child
// process we fix up all the paths returned by the go command.
cmd.Env = append(append([]string{}, i.Env...), "PWD="+i.WorkingDir)
cmd.Dir = i.WorkingDir

defer func(start time.Time) { log("%s for %v", time.Since(start), cmdDebugStr(cmd)) }(time.Now())

rawError = cmd.Run()
friendlyError = rawError
if rawError != nil {
// Check for 'go' executable not being found.
if ee, ok := rawError.(*exec.Error); ok && ee.Err == exec.ErrNotFound {
friendlyError = fmt.Errorf("go command required, not found: %v", ee)
}
if ctx.Err() != nil {
friendlyError = ctx.Err()
}
friendlyError = fmt.Errorf("err: %v: stderr: %s", rawError, stderr)
}
return
}

func cmdDebugStr(cmd *exec.Cmd) string {
env := make(map[string]string)
for _, kv := range cmd.Env {
split := strings.Split(kv, "=")
k, v := split[0], split[1]
env[k] = v
}

return fmt.Sprintf("GOROOT=%v GOPATH=%v GO111MODULE=%v GOPROXY=%v PWD=%v go %v", env["GOROOT"], env["GOPATH"], env["GO111MODULE"], env["GOPROXY"], env["PWD"], cmd.Args)
}

0 comments on commit c5cec67

Please sign in to comment.