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

cmd/go: "go install" of std/cmd in a different GOROOT silently does the wrong thing #45888

Open
mvdan opened this issue Apr 30, 2021 · 13 comments
Labels
GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented Apr 30, 2021

$ pwd
/home/mvdan/tip/src/cmd/gofmt
$ go version
go version devel go1.17-8e91458b19 Fri Apr 30 20:00:36 2021 +0000 linux/amd64
$ go install
$ which gofmt
/home/mvdan/tip/bin/gofmt
$ /usr/bin/go version
go version go1.16.3 linux/amd64
$ /usr/bin/go install
$ go version $(which gofmt)
/home/mvdan/tip/bin/gofmt: devel go1.17-8e91458b19 Fri Apr 30 20:00:36 2021 +0000

Note that Go 1.16.3 happily does nothing when it's told to go install cmd/gofmt from tip, even though that should most definitely error. From the last command, you can see that the gofmt I'm using has not changed. The gofmt from /usr/bin can't have been changed either, as I'm not root.

I think I get why this is happening. /usr/bin/go install turns into /usr/bin/go install cmd/gofmt, so that ends up installing 1.16.3's cmd/gofmt, which is already up to date - hence nothing to do.

But that feels wrong. If I'm installing the package at the current directory, and this is a package under a different GOROOT than the toolchain's, that should simply fail very early on.

This caused me confusion on https://go-review.googlesource.com/c/go/+/284139. After a rebase on a cmd/gofmt change, I ran go install and it finished incredibly quickly, even though I knew I had likely made a mistake. Plus, I would expect the first build to take about a second, as I'd need a new binary for sure. It was five minutes later that I realised I had forgot to re-run make.bash earlier in the day, so my go binary was falling back to the system's 1.16.3, rather than tip.

cc @bcmills @matloob

@mvdan mvdan added the GoCommand cmd/go label Apr 30, 2021
@mvdan
Copy link
Member Author

mvdan commented Apr 30, 2021

Also, there isn't a gofmt in my GOBIN either.

@mvdan
Copy link
Member Author

mvdan commented Apr 30, 2021

I've also seen a very similar pattern with new contributors to Go, where they would do the equivalent of:

$ go1.16 version
go version go1.16.3 linux/amd64
$ [clone and build Go tip]
$ cd tip/src
$ go1.16 test ./encoding/json

That should fail with a very clear message, like "you're using the Go toolchain at /usr/lib/go with the GOROOT at /home/user/tip".

@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 30, 2021
@cherrymui cherrymui added this to the Backlog milestone Apr 30, 2021
@cherrymui
Copy link
Member

cc @bcmills @jayconrod @matloob

@cherrymui
Copy link
Member

I had forgot to re-run make.bash earlier in the day, so my go binary was falling back to the system's 1.16.3, rather than tip.

I'm confused by this. Why re-running make.bash does anything with what "go" binary it is on your PATH?

@mvdan
Copy link
Member Author

mvdan commented May 2, 2021

I'm confused by this. Why re-running make.bash does anything with what "go" binary it is on your PATH?

PATH=$HOME/.bin:$HOME/tip/bin:$HOME/go/bin:$PATH

That is, go from tip takes precedence over the system/s in /usr/bin, if it exists.

That's not the issue at hand, though. The problem is that using the go executable from GOROOT "foo" in GOROOT "bar" silently does weird things.

@cherrymui
Copy link
Member

You delete the go binary in GOROOT/bin/go before rerunning make.bash? (I agree this is a valid thing to do. I just never done it, so the "go" binary still there, although may be stale.)

@mvdan
Copy link
Member Author

mvdan commented May 3, 2021

Yes. I essentially do a full git clean whenever I pull the latest master, and shortly after I do a make.bash. Except the time above, when I forgot the extra step :)

@bcmills
Copy link
Contributor

bcmills commented May 3, 2021

I put the current funky semantics in place for #30756@josharian explicitly wanted to be able to run commands while in a different GOROOT, and that was what I could come up with that seemed to fit his existing usage without being too semantically odd (see #30756 (comment)).

That said, this behavior affects a very small number of developers, most of whom are Go contributors, so if we think something else would be clearer (and not break anybody's workflow too badly), I'm happy to adjust it.

@bcmills
Copy link
Contributor

bcmills commented May 3, 2021

Ah, but the funky semantics only really work for module std. For module cmd there are no path-prefix shenanigans, so the import path really does collide with the cmd/gofmt in the real GOROOT/src/cmd/gofmt.

I think that makes this particular example a duplicate of #35270?

@mvdan
Copy link
Member Author

mvdan commented May 4, 2021

I don't think this is the same as #30756. Running Go from GOROOT_A in GOROOT_B, I think it's fine for go build bytes to refer to GOROOT_A/src/bytes. But cd GOROOT_B/src/bytes; go build . should error, as I really am referring to the package in the current directory, not the global "bytes".

Similarly, in my example above I was doing a go install, AKA go install ., not a go install cmd/gofmt.

I think that makes this particular example a duplicate of #35270?

I don't mind if this issue is closed as a duplicate, as long as we agree on the issue and that it needs a fix :) I couldn't find a previous issue about it.

@bcmills
Copy link
Contributor

bcmills commented Jun 30, 2021

But cd GOROOT_B/src/bytes; go build . should error, as I really am referring to the package in the current directory, not the global "bytes".

Based on the semantics in #30756, cd GOROOT_B/src/bytes; go build . should succeed, and should build the package in the current directory as the package with path std/bytes (not bytes), which doesn't collide with the standard library because std/bytes is not normally a valid package path.

Similarly, in my example above I was doing a go install, AKA go install ., not a go install cmd/gofmt.

Ah, but cmd is different because it does have a path prefix, and that path prefix does collide with the standard library. cmd/gofmt really does collide.

@bcmills
Copy link
Contributor

bcmills commented Jun 30, 2021

(To be clear, I would also be ok with changing the semantics to diverge from #30756, if we can come to a consensus about what the actual behavior ought to be.)

@mvdan
Copy link
Member Author

mvdan commented Jul 1, 2021

I honestly don't mind what the final semantics are, as long as running go install of relative paths inside GOROOT_B doesn't silently use packages from GOROOT_A. The source of my confusion was not seeing a new binary or an error when running go install in those scenarios.

I'd slightly lean towards making both std and cmd cases error, because I struggle to imagine why building a std/bytes package would ever be a useful thing. It's also a common thing amongst Go newcomers that they build Go from master but try to go test one of its std packages with a different go command version. It can be an easy thing to miss, given that $PATH can be confusing to inexperienced people. It would be very useful if that scenario simply errored with a useful message right away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants