go/packages: use GOROOT to determine go binary

[marwan-at-work]

This change allows users with multiple Go distributions
installed on their machine to use whichever go binary they prefer
by looking at the runtime GOROOT and executing the bin/go inside of it.
If the binary is not found, it will fallback to using "go" which is
whatever version is set in the user's PATH.

Fixes golang/go#26845

[gopherbot]

Message from Gerrit User 5056:

Patch Set 1:

I've been wondering about whether to do something like this for a while. I am not convinced this CL is exactly right: it matters whether GOROOT is set or not, and runtime.GOROOT does not distinguish.

If GOROOT is set, then yes we probably want to use GOROOT/bin/go.

If GOROOT is unset and my PATH has a go command, then probably I want to use that go command instead of whatever runtime.GOROOT might return (the GOROOT used to build the command in question).

But if GOROOT is set without also putting GOROOT/bin/go ahead of other go commands in PATH, basic things like 'go build' are going to fail. I can't quite convince myself we need to correct the other tools in this one case.

At the least, this CL should use os.Getenv("GOROOT") instead of runtime.GOROOT, and fall back to PATH if $GOROOT is empty. I'm still not sure that's correct, but it's closer.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/128295.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gerrit User 5020:

Patch Set 1:

Patch Set 1:

I've been wondering about whether to do something like this for a while. I am not convinced this CL is exactly right: it matters whether GOROOT is set or not, and runtime.GOROOT does not distinguish.

If GOROOT is set, then yes we probably want to use GOROOT/bin/go.

If GOROOT is unset and my PATH has a go command, then probably I want to use that go command instead of whatever runtime.GOROOT might return (the GOROOT used to build the command in question).

But if GOROOT is set without also putting GOROOT/bin/go ahead of other go commands in PATH, basic things like 'go build' are going to fail. I can't quite convince myself we need to correct the other tools in this one case.

At the least, this CL should use os.Getenv("GOROOT") instead of runtime.GOROOT, and fall back to PATH if $GOROOT is empty. I'm still not sure that's correct, but it's closer.

I would go as far as to say that setting PATH should be the only way to control which Go gets used. Setting GOROOT has been largely unnecessary and discouraged for a while now as far as the go tool itself is concerned. It's only really been necessary for tools, because go/loader relied on it.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/128295.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gerrit User 9711:

Patch Set 1:

I don't think we are likely to accept any cl that adds a dependency on go/build, if we really need functionality from it we should copy it into this package.

If we do want to check the environment, we should be checking cfg.Env not os.Getenv(), as that is the environment we will be running the underlying tool in anyway.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/128295.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gerrit User 28030:

Patch Set 1:

Patch Set 1:

I don't think we are likely to accept any cl that adds a dependency on go/build, if we really need functionality from it we should copy it into this package.

If we do want to check the environment, we should be checking cfg.Env not os.Getenv(), as that is the environment we will be running the underlying tool in anyway.

That makes sense. Per Russ's comments, checking the environment is a safer option than using runtime.GOROOT so I will use cfg.Env in this case. Happy to change it up again if need be.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/128295.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gerrit User 28030:

Patch Set 1:

Patch Set 1:

I don't think we are likely to accept any cl that adds a dependency on go/build, if we really need functionality from it we should copy it into this package.

If we do want to check the environment, we should be checking cfg.Env not os.Getenv(), as that is the environment we will be running the underlying tool in anyway.

In this case, is it necessary to Stat the GOROOT bin path like I have in this CL or should I remove it? Thanks

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/128295.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gerrit User 24096:

Patch Set 1:

I have been bit by this during godoc testing too. Godoc web server tests use the "go" binary found in $PATH to build godoc and run tests. While testing with tip, I just use "gotip" which is an alias to my go tip binary path. I had to manually set my $PATH variable to work around this.

But to solve this, we should avoid using GOROOT altogether like Dominik suggests. It is not recommended anymore and we should not have more code relying on GOROOT.

We can directly use sys.DefaultGoroot from "runtime/internal/sys" if needed. That is the one runtime.GOROOT() falls back to if it does not get anything set in $GOROOT env var.

Or otherwise, keep using $PATH.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/128295.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gerrit User 12446:

Uploaded patch set 3: New patch set was added with same tree, parent, and commit message as Patch Set 2.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/128295.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gerrit User 28030:

Patch Set 3:

Patch Set 1:

I don't think we are likely to accept any cl that adds a dependency on go/build, if we really need functionality from it we should copy it into this package.

If we do want to check the environment, we should be checking cfg.Env not os.Getenv(), as that is the environment we will be running the underlying tool in anyway.

I've updated the CL :)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/128295.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

This PR (HEAD: 4a887cd) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/tools/+/128295 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

[gopherbot]

Message from Gerrit User 28030:

Patch Set 4:

Patch Set 1:

I don't think we are likely to accept any cl that adds a dependency on go/build, if we really need functionality from it we should copy it into this package.

If we do want to check the environment, we should be checking cfg.Env not os.Getenv(), as that is the environment we will be running the underlying tool in anyway.

Updated CL to use GOCMD instead of GOROOT and updated the commit message accordingly.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/128295.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gerrit User 13220:

Patch Set 4:

(1 comment)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/128295.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

This PR (HEAD: 47537e8) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/tools/+/128295 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

[gopherbot]

Message from Gerrit User 13220:

Patch Set 5:

(2 comments)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/128295.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

This PR (HEAD: 275c7f2) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/tools/+/128295 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

[gopherbot]

Message from Gerrit User 5195:

Patch Set 6:

(2 comments)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/128295.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gerrit User 13220:

Patch Set 6:

(1 comment)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/128295.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gerrit User 5195:

Patch Set 6:

(1 comment)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/128295.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gerrit User 13220:

Patch Set 6:

(1 comment)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/128295.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Alan Donovan:

Patch Set 6: Code-Review-1

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/128295.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

This PR is being closed because golang.org/cl/128295 has been abandoned.

[dmitshur]

Copying my message from Gerrit here, since gerritbot didn't (but it will after golang/go#28855 is resolved):

Issue golang/go#26845 has been resolved. It was decided that it's not necessary to add support for GOCMD environment variable, because a valid existing approach of setting PATH to control which go binary is used exists and is recommended instead. For more context, also see golang/go#28043.

I'll close this CL since it doesn't seem to be needed anymore, but please feel free to reopen otherwise.