Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
hugelgupf opened this issue Aug 17, 2023 · 9 comments
Open

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

hugelgupf opened this issue Aug 17, 2023 · 9 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@hugelgupf
Copy link
Contributor

hugelgupf commented Aug 17, 2023 •
edited by dmitshur

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>
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Aug 17, 2023
@gopherbot gopherbot added this to the Unreleased milestone Aug 17, 2023
This was referenced Aug 17, 2023
Merged
Closed
@dmitshur dmitshur changed the title x/tools/packages: Load does not respect Go version consistently x/tools/go/packages: Load does not respect Go version consistently Aug 17, 2023
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 17, 2023
@dmitshur
Copy link
Contributor

CC @matloob, @golang/tools-team.

@matloob
Copy link
Contributor

matloob commented Aug 18, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@bcmills bcmills mentioned this issue Jan 4, 2024
Open
@matloob
Copy link
Contributor

matloob commented Jan 8, 2024

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
Copy link
Contributor Author

hugelgupf commented Jan 9, 2024 via email

@dmitshur
Copy link
Contributor

dmitshur commented Jan 9, 2024

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
Copy link
Contributor Author

hugelgupf commented Jan 9, 2024 via email

@hyangah
Copy link
Contributor

hyangah commented Jan 9, 2024

How about using the new GOTOOLCHAIN environment variable?
One caveat is that the version should be the exact go toolchain version, not the language version.

$ go version
go version go1.21.5 darwin/amd64
$ GOTOOLCHAIN=go1.19.5 go version
go version go1.19.5 darwin/amd64

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@dmitshur @hugelgupf @hyangah @gopherbot @matloob