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.

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

x/tools/go/packages: should it respect GOROOT? #41231

Closed
hugelgupf opened this issue Sep 4, 2020 · 7 comments
Closed

x/tools/go/packages: should it respect GOROOT? #41231

hugelgupf opened this issue Sep 4, 2020 · 7 comments
Labels
FrozenDueToAge 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. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@hugelgupf
Copy link
Contributor

What version of Go are you using (go version)?

many

Does this issue reproduce with the latest release?

yes

What did you do?

Different Go versions have slightly different behaviors in go list or go mod. That's ok.

I work on some stuff that uses x/tools/go/packages to do AST transformations on code before compiling it. x/tools/go/packages is near perfect for us, save for one issue: it does not respect GOROOT.

This often comes up for me because I use go get golang.org/dl/go1.x to test whether compilation works under different versions of Go. Those get installed as go1.13 binaries, for example, and invoking

go1.13 run transformer.go -- ...

does not have the desired effect, because for everything underneath, it still uses go from $PATH, which in my case is 1.15. See exec.Command invocation here.

I've had to replicate some of the stuff from x/tools/go/packages in a development branch in order to use $GOROOT/bin/go instead, and I'm wondering if it'd make sense for x/tools/go/packages to also use $GOROOT/bin/go rather than go from $PATH.

(Apologies for not having a specific example in mind right now -- it's been a long time since I actually worked in this code base, and I'm just now picking it up again. I may be able to give you a better example later.)

cc @heschik @stamblerre

@dmitshur
Copy link
Contributor

dmitshur commented Sep 5, 2020

I'm not sure if this is sufficient to address the entire issue or just a part of it, but I want to comment on a part of what you said:

This often comes up for me because I use go get golang.org/dl/go1.x to test whether compilation works under different versions of Go. Those get installed as go1.13 binaries, for example, and invoking

go1.13 run transformer.go -- ...

does not have the desired effect, because for everything underneath, it still uses go from $PATH, which in my case is 1.15.

It should have the desired effect. The golang.org/dl/go1.x commands modify PATH, adding the go1.x's GOROOT/bin to the beginning, so that exec.Command("go") does the right thing. You can test with a simple program:

package main

import ("fmt"; "log"; "os/exec")

func main() {
	out, err := exec.Command("go", "version").Output()
	if err != nil { log.Fatalln(err) }
	fmt.Printf("go version = %s", out)
	out, err = exec.Command("go", "env", "GOROOT").Output()
	if err != nil { log.Fatalln(err) }
	fmt.Printf("GOROOT of 'go' = %s", out)
}
$ go run printgover.go 
go version = go version go1.15.1 darwin/amd64
GOROOT of 'go' = /usr/local/go

$ go1.13.7 run printgover.go
go version = go version go1.13.7 darwin/amd64
GOROOT of 'go' = /Users/gopher/sdk/go1.13.7

See relevant code. Is your golang.org/dl/go1.x command up to date enough to include that?

@dmitshur dmitshur changed the title Should x/tools/go/packages respect GOROOT? x/tools/go/packages: should it respect GOROOT? Sep 5, 2020
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 5, 2020
@gopherbot gopherbot added this to the Unreleased milestone Sep 5, 2020
@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Sep 5, 2020
@hugelgupf
Copy link
Contributor Author

Ah, neat! To be fair, I haven't actually tried it without the $GOROOT/bin/go fix, which I seem to have committed more than 2 years ago. u-root/u-root@e4cad48

What you mention does solve the common go1.x case. I have at times invoked this stuff with a straight up absolute path Go -- e.g. $HOME/golang/bin/go -- when I've messed with the Go tree, but I don't have my Go development tree in my PATH. I'll admit, that's relatively rare, but I feel it should still work with these tools.

@hugelgupf
Copy link
Contributor Author

If this use case is not very desired, I'm wondering if adding $PATH=$GOROOT/bin:$PATH to packages.Config.Env would work, so perhaps this is solvable on my end without intervention upstream. (How does Linux handle 2 PATH vars in Env? I've never tried...)

@dmitshur
Copy link
Contributor

dmitshur commented Sep 5, 2020

In general, I think you should definitely arrange things so that exec.LookPath("go") finds the go binary that you intend to use. If you don't do that, some code somewhere (e.g., a dependency you didn't write) that needs to use the go command may end up picking the wrong one.

I think using packages.Config.Env sounds like the right solution. I'll let others comment on whether something can/should be done in go/packages.

How does Linux handle 2 PATH vars in Env? I've never tried...

Usually duplicate environment variables are okay, the latest one wins. You can also de-duplicate them if want (e.g., see envutil.Dedup), but I wouldn't do it unless you have a concrete need.

@dmitshur
Copy link
Contributor

dmitshur commented Sep 5, 2020

This may be a duplicate of #26845, which was ultimately closed in favor of modifying PATH in environment so the right go binary is found.

@hugelgupf
Copy link
Contributor Author

I'm fine with closing as a dup. I was asking about deduplication because gocommand.Invocation forces os.Environ on the user in addition to the Env configured, so I can't dedup. But our use case at least has a minimum Go version of 1.13 already, so no concrete need.

Thanks!

@heschi
Copy link
Contributor

heschi commented Sep 8, 2020

forces os.Environ

As Dmitri said, it doesn't; later variables override earlier ones. The problem is that exec.Command doesn't respect PATH when finding the command to run. To fix that we'd have to reimplement exec.LookPath.

hugelgupf added a commit to hugelgupf/gobusybox that referenced this issue Sep 9, 2020
This matters between Go 1.12 and Go 1.13, for example, where GO111MODULE
default changed.

See golang/go#41231

Signed-off-by: Chris Koch <chrisko@google.com>
@golang golang locked and limited conversation to collaborators Sep 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge 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. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

4 participants