From c5032d394f0028702e058dbbabd93c08ddcaea5b Mon Sep 17 00:00:00 2001 From: Ian Cottrell Date: Fri, 19 Oct 2018 16:48:31 -0400 Subject: [PATCH] go/packages: Fixes for bad wd handling Darwin convertes the working directory to a real path which affects the path a command is started with when you exec. This causes a problem where the path the go list command is run with does not match the path the outside program is trying to use, which causes both undesirably changed paths in it's outputs, and sometimes failures to cope with modules when source files are "outside" the module root. The go standard library has a special feature in os.Getwd where it checks if the PWD environment variable is the same inode as the system cwd, and if it is it returns the PWD instead. This change deliberately sets the PWD before running go list so that behaviour is triggered, which fixes all the paths. This was breaking the mac build bots in a goimports test, it should be fixed now. Change-Id: I0f5d3c7d020b55749738036ba51c19884bb26598 Reviewed-on: https://go-review.googlesource.com/c/143517 Run-TryBot: Ian Cottrell TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- go/packages/golist.go | 47 +++++++++++++++++----------------- go/packages/golist_fallback.go | 6 ++--- 2 files changed, 27 insertions(+), 26 deletions(-) diff --git a/go/packages/golist.go b/go/packages/golist.go index 37a6b8b1..a1e9c320 100644 --- a/go/packages/golist.go +++ b/go/packages/golist.go @@ -8,8 +8,6 @@ import ( "bytes" "encoding/json" "fmt" - "golang.org/x/tools/internal/gopathwalk" - "golang.org/x/tools/internal/semver" "io/ioutil" "log" "os" @@ -18,6 +16,9 @@ import ( "regexp" "strings" "sync" + + "golang.org/x/tools/internal/gopathwalk" + "golang.org/x/tools/internal/semver" ) // A goTooOldError reports that the go command @@ -292,20 +293,14 @@ func runNamedQueries(cfg *Config, driver driver, addPkg func(*Package), queries // roots selects the appropriate paths to walk based on the passed-in configuration, // particularly the environment and the presence of a go.mod in cfg.Dir's parents. func roots(cfg *Config) ([]gopathwalk.Root, bool, error) { - stdout := new(bytes.Buffer) - stderr := new(bytes.Buffer) - cmd := exec.CommandContext(cfg.Context, "go", "env", "GOROOT", "GOPATH", "GOMOD") - cmd.Stdout = stdout - cmd.Stderr = stderr - cmd.Dir = cfg.Dir - cmd.Env = cfg.Env - if err := cmd.Run(); err != nil { - return nil, false, fmt.Errorf("running go env: %v (stderr: %q)", err, stderr.Bytes()) + stdout, err := invokeGo(cfg, "env", "GOROOT", "GOPATH", "GOMOD") + if err != nil { + return nil, false, err } - fields := strings.Split(string(stdout.Bytes()), "\n") + fields := strings.Split(stdout.String(), "\n") if len(fields) != 4 || len(fields[3]) != 0 { - return nil, false, fmt.Errorf("go env returned unexpected output: %q (stderr: %q)", stdout.Bytes(), stderr.Bytes()) + return nil, false, fmt.Errorf("go env returned unexpected output: %q", stdout.String()) } goroot, gopath, gomod := fields[0], filepath.SplitList(fields[1]), fields[2] modsEnabled := gomod != "" @@ -449,7 +444,7 @@ func golistDriverCurrent(cfg *Config, words ...string) (*driverResponse, error) // Run "go list" for complete // information on the specified packages. - buf, err := golist(cfg, golistargs(cfg, words)) + buf, err := invokeGo(cfg, golistargs(cfg, words)...) if err != nil { return nil, err } @@ -573,12 +568,18 @@ func golistargs(cfg *Config, words []string) []string { return fullargs } -// golist returns the JSON-encoded result of a "go list args..." query. -func golist(cfg *Config, args []string) (*bytes.Buffer, error) { +// invokeGo returns the stdout of a go command invocation. +func invokeGo(cfg *Config, args ...string) (*bytes.Buffer, error) { stdout := new(bytes.Buffer) stderr := new(bytes.Buffer) cmd := exec.CommandContext(cfg.Context, "go", args...) - cmd.Env = cfg.Env + // 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 @@ -591,9 +592,9 @@ func golist(cfg *Config, args []string) (*bytes.Buffer, error) { return nil, fmt.Errorf("couldn't exec 'go %v': %s %T", args, err, err) } - // Old go list? - if strings.Contains(fmt.Sprint(cmd.Stderr), "flag provided but not defined") { - return nil, goTooOldError{fmt.Errorf("unsupported version of go list: %s: %s", exitErr, cmd.Stderr)} + // 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)} } // Export mode entails a build. @@ -601,7 +602,7 @@ func golist(cfg *Config, args []string) (*bytes.Buffer, error) { // (despite the -e flag) and the Export field is blank. // Do not fail in that case. if !usesExportData(cfg) { - return nil, fmt.Errorf("go %v: %s: %s", args, exitErr, cmd.Stderr) + return nil, fmt.Errorf("go %v: %s: %s", args, exitErr, stderr) } } @@ -612,12 +613,12 @@ func golist(cfg *Config, args []string) (*bytes.Buffer, error) { // be useful for debugging. Print them if $GOPACKAGESPRINTGOLISTERRORS // is set. if len(stderr.Bytes()) != 0 && os.Getenv("GOPACKAGESPRINTGOLISTERRORS") != "" { - fmt.Fprintf(os.Stderr, "go %v stderr: <<\n%s\n>>\n", args, stderr) + fmt.Fprintf(os.Stderr, "go %v stderr: <<%s>>\n", args, stderr) } // debugging if false { - fmt.Fprintln(os.Stderr, stdout) + fmt.Fprintf(os.Stderr, "go %v stdout: <<%s>>\n", args, stdout) } return stdout, nil diff --git a/go/packages/golist_fallback.go b/go/packages/golist_fallback.go index 79b12071..ac0c34f0 100644 --- a/go/packages/golist_fallback.go +++ b/go/packages/golist_fallback.go @@ -227,7 +227,7 @@ func golistDriverFallback(cfg *Config, words ...string) (*driverResponse, error) return &response, nil } - buf, err := golist(cfg, golistArgsFallback(cfg, deps)) + buf, err := invokeGo(cfg, golistArgsFallback(cfg, deps)...) if err != nil { return nil, err } @@ -362,7 +362,7 @@ func vendorlessPath(ipath string) string { // getDeps runs an initial go list to determine all the dependency packages. func getDeps(cfg *Config, words ...string) (originalSet map[string]*jsonPackage, deps []string, err error) { - buf, err := golist(cfg, golistArgsFallback(cfg, words)) + buf, err := invokeGo(cfg, golistArgsFallback(cfg, words)...) if err != nil { return nil, nil, err } @@ -396,7 +396,7 @@ func getDeps(cfg *Config, words ...string) (originalSet map[string]*jsonPackage, } // Get the deps of the packages imported by tests. if len(testImports) > 0 { - buf, err = golist(cfg, golistArgsFallback(cfg, testImports)) + buf, err = invokeGo(cfg, golistArgsFallback(cfg, testImports)...) if err != nil { return nil, nil, err }