code review 7786047: cmd/go: check GOROOT directory is present before acting

cmd/go: check GOROOT directory is present before acting

Fixes issue 5042.

[brainman, 2013-03-22 05:21:02]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[adg, 2013-03-22 05:48:03]

Oh wait, I was wrong. This LGTM.

[adg, 2013-03-22 05:51:17]

This will prevent people from even seeing "go help" without GOROOT set
correctly. That doesn't seem right.

[brainman, 2013-03-22 05:52:16]

On 2013/03/22 05:51:17, adg wrote:
> This will prevent people from even seeing "go help" without GOROOT set
> correctly. That doesn't seem right.

Good point. I will change it.

Alex

[brainman, 2013-03-24 22:48:28]

On 2013/03/22 05:51:17, adg wrote:
> This will prevent people from even seeing "go help" without GOROOT set
> correctly. That doesn't seem right.

I looked again, and "help" commands is handled up above on line 121 already. So,
I think, my change is OK. Please, look again. Thank you.

Alex

[rsc, 2013-03-25 21:56:05]

https://codereview.appspot.com/7786047/diff/2001/src/cmd/go/main.go
File src/cmd/go/main.go (right):

https://codereview.appspot.com/7786047/diff/2001/src/cmd/go/main.go#newcode148
src/cmd/go/main.go:148: fmt.Fprintf(os.Stderr, "go: cannot find GOROOT
directory: %q\n", goroot)
Should this use %s instead of %q? It's going to print on Windows as

cannot find GOROOT directory: "C:\\go"

which may confuse people.

[brainman, 2013-03-25 22:33:33]

On 2013/03/25 21:56:05, rsc wrote:
> Should this use %s instead of %q? It's going to print on Windows as
> 
> cannot find GOROOT directory: "C:\\go"
> 
> which may confuse people.

I agree. But the original problem was double quotes around environment variable,
and in that instance the message will be:

cannot find GOROOT directory: "C:\go"

which will be even more confusing. Given we are dealing with "unusual"
characters here, we should be as clear as we can be. What do you think?

Alex

[brainman, 2013-05-23 03:16:23]

Can we decide what to do here, please?

Alex

[adg, 2013-05-23 03:21:40]

We don't quote our other diagnostic strings in cmd/go (AFAICT) so it seems like
it should be %v.

[brainman, 2013-05-23 04:09:48]

Hello golang-dev@googlegroups.com, adg@golang.org, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[adg, 2013-05-23 04:11:29]

LGTM

[brainman, 2013-05-23 04:13:14]

*** Submitted as https://code.google.com/p/go/source/detail?r=a1fb1560e22e ***

cmd/go: check GOROOT directory is present before acting

Fixes issue 5042.

R=golang-dev, adg, rsc
CC=golang-dev
https://codereview.appspot.com/7786047