x/tools/go/packages: Load does not respect Go version consistently

[hugelgupf]

x/tools/go/packages invokes the "go" command, but often behaves differently depending on the underlying Go version. As a consumer of the package, I regularly test my usage against several versions of Go, because these tools behave differently depending on the underlying Go version.

At one point, I believed that setting packages.Config.Env with the correct GOROOT and PATH=$GOROOT/bin:$PATH would make up for this, but because exec.Command uses exec.LookPath (which is hard-coded to the system os.Getenv("PATH"), see #20081, #59753, #60601) and packages.Load does nothing to mitigate that, the Go version used to load packages is not the correct one.

Here's an example usage of packages.Load:

package main

import (
        "fmt"
        "go/build"
        "log"
        "os"
        "runtime"

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

func GOROOT() string {
        if g := build.Default.GOROOT; g != "" {
                return g
        }
        return runtime.GOROOT()
}

func main() {
        fmt.Println("GOROOT:", GOROOT())
        fmt.Println("PATH:", os.Getenv("PATH"))
        cfg := &packages.Config{
                Mode: packages.LoadSyntax | packages.LoadImports,
                Env: []string{
                        "GOPACKAGESDRIVER=off",
                        // Packages driver needs a GOROOT to look up cmd/test2json.
                        fmt.Sprintf("GOROOT=%s", GOROOT()),
                        // Where to find the Go binary itself.
                        fmt.Sprintf("PATH=%s/bin", GOROOT()),
                        // Package driver needs HOME (or GOCACHE) for build cache.
                        fmt.Sprintf("HOME=%s", os.Getenv("HOME")),
                },
                Dir: "",
        }
        pkgs, err := packages.Load(cfg, "cmd/test2json")
        if err != nil {
                log.Fatal(err)
        }
        for _, pkg := range pkgs {
                fmt.Printf("%#v\n", pkg)
        }
}

Say you are on a system where the admin has installed some older Go, but you've used golang.org/dl/go1.20 to download a newer version. Or it's a newer version of Go, but you want to ensure that your packages.Load code works against every Go version 1.18 and up.

$ go1.20 build -o pkgdriver
$ GOROOT=$HOME/sdk/go1.20 strace -fe trace=execve -e signal=SIGIO ./pkgdriver 2>&1 | grep -v "exited with" | grep -v "strace: Process"

[pid 675271] execve("/usr/lib/google-golang/bin/go", ["go", "list", "-e", "-f", "{{context.ReleaseTags}}", "--", "unsafe"], 0xc00007e340 /* 6 vars */) = 0                 
[pid 675272] execve("/usr/lib/google-golang/bin/go", ["go", "list", "-f", "{{context.GOARCH}} {{context.Com"..., "--", "unsafe"], 0xc00009c150 /* 5 vars */) = 0           
[pid 675282] execve("/usr/lib/google-golang/bin/go", ["go", "list", "-e", "-json=Name,ImportPath,Error,Dir,"..., "-compiled=true", "-test=false", "-export=true", "-deps=tr
ue", "-find=false", "-pgo=off", "--", "cmd/test2json"], 0xc00002ea50 /* 5 vars */) = 0                                                                                     
[pid 675291] execve("/usr/local/google/home/chrisko/sdk/go1.20/pkg/tool/linux_amd64/compile", ["/usr/local/google/home/chrisko/s"..., "-V=full"], 0xc0001d9d40 /* 25 vars *
/) = 0                                                                                                                                                                     
[pid 675292] execve("/usr/local/google/home/chrisko/sdk/go1.20/pkg/tool/linux_amd64/compile", ["/usr/local/google/home/chrisko/s"..., "-V=full"], 0xc00013cc30 /* 25 vars *
/ <unfinished ...>
...

Despite best efforts, the system "go" (which might be older) is used to execute the list driver, but $GOROOT/bin/go is then used to compile more information about the package by calling compile/asm functions. This is inconsistent.

Note that if instead of compiling the command and then running it, you run it with go1.20 run, the command seems to mitigate this itself by placing GOROOT/bin in the PATH prior to execution:

$ go1.20 run .
PATH: /usr/local/google/home/chrisko/sdk/go1.20/bin:<snip>

[dmitshur]

CC @matloob, @golang/tools-team.

[matloob]

Is the request that we essentially look up the path to the go command using the given environment before running it?

I think either behavior is consistent with the definition of Env: "Env is the environment to use when invoking the build system's query tool."

Would you be able to set the environment on the tool that you run so that the correct go command is in the path when you run the tool?

[hugelgupf]

Is the request that we essentially look up the path to the go command using the given environment before running it?

Yes. Especially because depending on the situation, the Go packages driver code either directly calls the go command (in which case, system-PATH-go is used) or first invokes a gopackagesdriver that may the same thing (but which now has the PATH set to what I want, and therefore uses the go binary that I want). And if the system-PATH-go command invokes go again, it will then use the go supplied in the PATH I set (This is what you can see in the strace example above -- google/go list invoked by API fork/execs go1.20/go compile, because the PATH changed).

The outcome of a Load call is different depending on the nesting of subprocesses that is invoked by the implementation of Load, which IMO leads to an API that returns confusing results.

Would you be able to set the environment on the tool that you run so that the correct go command is in the path when you run the tool?

I'm supplying these tools as both binaries and APIs. It's possible in the binaries, it's not really possible in the APIs, as I'd have to mess with the caller's PATH for the entire process.

[hugelgupf]

I think another option, rather than using the PATH supplied in env, would be an argument that gives the ability to specify the go command location.

[matloob]

I want to push back a little on this. Right now the contract is that the user of the binary that uses go/packages ensures that the right go command is in their PATH before running it. I think it's reasonable to require users of your API to make sure that they have the correct version of go in their PATH.

What's the use case for using a different version of Go when using go/packages through an API you provide?

[hugelgupf]

The use case is using the golang.org/dl/goN.M binaries. In that case, the binary name that is in PATH is go1.21, not go. It’s how I tend to test these features on different Go versions.

…

On Mon, Jan 8, 2024 at 11:55 Michael Matloob ***@***.***> wrote: I want to push back a little on this. Right now the contract is that the user of the binary that uses go/packages ensures that the right go command is in their PATH before running it. I think it's reasonable to require users of your API to make sure that they have the correct version of go in their PATH. What's the use case for using a different version of Go when using go/packages through an API you provide? — Reply to this email directly, view it on GitHub <#62114 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPG3EVN6PT3IFVBS4V4LO3YNRFJXAVCNFSM6AAAAAA3UPWRM2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBRG4ZTAMRSGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[dmitshur]

the binary name that is in PATH is go1.21, not go.

Those goN.M commands modify PATH in the environment to put their own "go" binary first in the list. See here.

[hugelgupf]

Indeed, but you only get that if your tool using go/packages is run with goN.M run. If you build it with goN.M and then run the binary, you have to supply a PATH and a GOROOT to that binary to get the correct behavior.

…

On Mon, Jan 8, 2024 at 19:28 Dmitri Shuralyov ***@***.***> wrote: the binary name that is in PATH is go1.21, not go. Those goN.M commands modify PATH in the environment to put their own "go" binary first in the list. See here <https://cs.opensource.google/go/dl/+/master:internal/version/version.go;l=63-66;drc=2a66dfb9a9cf29fab51425f5c052d40437875c70> . — Reply to this email directly, view it on GitHub <#62114 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPG3EV3EXYDB47SZYSUKBTYNS2N7AVCNFSM6AAAAAA3UPWRM2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBSGMZTGNJXGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>