cmd/go: for unset GOROOT, try to derive from os.Executable() before runtime.GOROOT()

[minux]

Proposal:

For Go 1.9, I'd like to have cmd/go use the new os.Executable APi to find GOROOT automatically. And then we can eliminate all mentions of GOROOT in user facing documents (in fact, we should actively discourage setting GOROOT in user facing documents.)

In fact, I'd like to go so far as to display a warning if the $GOROOT setting contradicts with auto-detected one.

The proposed GOROOT logic is (error handling omitted):

// if the embedded goroot is wrong, then:
gorootAuto = filepath.Dir(filepath.Dir(filepath.EvalSymlinks(os.Executable())))
// issue a warning if gorootAuto is correct (having matching VERSION) and the VERSION file
// is not os.SameFile with $GOROOT/VERSION.
// use $GOROOT if set, otherwise gorootAuto. [*]

Potential problems:

1. hardlinking bin/go to another location.
2. moving bin/go to another location.

In both cases, the active fallback step [*] will preserve the current behavior.

3. distribution changes that don't follow the official GOROOT layout.

In this case, the distribution must already carry patches on the go command, so they will have to adapt the patch if this proposal is accepted.

[davecheney]

I'd sell my firstborn male heir to get rid of GOROOT once and for all.

[rsc]

I don't object to this in theory, but I don't understand the exact proposal.

// if the embedded goroot is wrong, then:

Please define "wrong".

[rsc]

@davecheney Do you still have to use GOROOT regularly? I don't see why.

[davecheney]

I don't, but an incorrectly set GOROOT is by far the most common cause of issue reports that I see on the gobridge forum and the mailing list. Perhaps others can offer supporting evidence from other forums. Five years of telling, begging, and pleading people not to set GOROOT (which they then forget they have done) has proved ineffective. I think the solution is to nullify GOROOT.

…

On Tue, 24 Jan 2017, 07:58 Russ Cox ***@***.***> wrote: @davecheney <https://github.com/davecheney> Do you still have to use GOROOT regularly? I don't see why. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18678 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAcAyyAkMP_JPVeOMVGkdvKEuKS818Zks5rVRRngaJpZM4LlIpq> .

[minux]

Wrong is defined as $GOROOT/VERSION doesn't exist. Perhaps we can additionally require that $GOROOT/VERSION matches runtime.Version(), but I'm not very sure.

[rsc]

My $GOROOT/VERSION never exists, since I'm always building from a git checkout. Maybe this should be split into two things:

1. Make empty $GOROOT fall back to os.Executable before the baked-in runtime.defaultGoroot.
2. Make go command fail loudly if runtime.Version != $(cat GOROOT/VERSION).

I've wanted each of those separately, and I don't think they necessarily need to be mixed. It sounds like you are proposing:

3. Instead of making go command fail loudly, override $GOROOT silently.

I think that might not be necessary. If we have 1, then all the people who never set $GOROOT are taken care of, and if we have 2, then that avoids silent misbehavior. Trying to go further in 3 and correct silent misbehavior into silent maybe-correct-but-maybe-still-wrong behavior is where things get tricky.

[ianlancetaylor]

I'm concerned that your number 1 (Make empty $GOROOT fall back to os.Executable before the baked-in runtime.defaultGoroot) will break packages that install the go tool in a system-wide bin directory, and expect the baked-in defaultGoroot to let it know where to find the packages.

[davecheney]

Make go command fail loudly if runtime.Version != $(cat GOROOT/VERSION).

I tried to implement this in gb about a year ago because gb, but sadly operating system distros ruined the party because they strip files they think aren't important.

constabulary/gb#325 :(

[rsc]

Re 1, there is a test in the go command already for identifying a $GOROOT. Something like src/cmd/go/doc.go existing or some other similar file. We could use os.Executable first, but if that doesn't produce a valid GOROOT, then fall back to defaultGoroot.

Re 2, reading VERSION is probably not good enough anyway, but the go command could invoke go tool compile version before any compilation steps.

[rsc]

It sounds like we should probably do 1 (use os.Executable for default GOROOT instead of defaultGoroot).

For 2, we'd need to make sure this does not make a noticeable change to compile times. Running

time $(go tool -n compile) -V

in a loop I get times of about 0.050 seconds.

Maybe this issue should be restricted to 1 (what to do when GOROOT is unset).

[davecheney]

For 2, could cmd/go provide the value to cmd/compile to avoid the cost of the lookup?

…

On Tue, 31 Jan 2017, 08:29 Russ Cox ***@***.***> wrote: It sounds like we should probably do 1 (use os.Executable for default GOROOT instead of defaultGoroot). For 2, we'd need to make sure this does not make a noticeable change to compile times. Running time $(go tool -n compile) -V in a loop I get times of about 0.050 seconds. Maybe this issue should be restricted to 1 (what to do when GOROOT is unset). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18678 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAcA2Q_HTWm4wo7BUsC9s3OazIE2k7Cks5rXlZRgaJpZM4LlIpq> .

[rsc]

@davecheney Yes, I think so. Filed #19064 for that.

This issue is now down to just changing the default for an unset GOROOT. I've heard no objections to trying os.Executable before falling back to defaultGoroot.

[davecheney]

Yes, I think so. Filed #19064 for that.

Thanks @rsc

[gopherbot]

CL https://golang.org/cl/42533 mentions this issue.

[egonelbre]

I have test failure after CL 42533:

--- FAIL: TestExecutableGOROOT (0.27s)
        go_test.go:3986: C:\Users\Egon\AppData\Local\Temp\gotest815240476\newgoroot\bin\go.exe env GOROOT = "c:\\Go.tip", want "C:\\Go.tip"
FAIL
FAIL    cmd/go  134.329s

Windows paths are still case-insensitive (at least usually).

[alexbrainman]

--- FAIL: TestExecutableGOROOT

Issue #20336 ?

Alex

[bradfitz]

This was a dup of #17833. Closing that one too.