code review 6941058: cmd/go: remove $GOROOT as a go get target

cmd/go: remove $GOROOT as a go get target

Fixes issue 4186.

Back in the day, before the Go 1.0 release, $GOROOT was mandatory for building from source. Fast forward to now and $GOPATH is mandatory and $GOROOT is optional, and mainly used by those who use the binary distribution in uncommon places.

For example, most novices at least know about `sudo` as they would have used it to install the binary tarball into /usr/local. It is logical they would use the `sudo` hammer to `go get` other Go packages when faced with a permission error talking about the path they just had to use `sudo` on last time.

Even if they had read the documentation and set $GOPATH, go get will not work as expected as `sudo` masks most environment variables.

llucky(~) % ~/go/bin/go env | grep GOPATH
GOPATH="/home/dfc"
lucky(~) % sudo ~/go/bin/go env | grep GOPATH
GOPATH=""

This CL therefore proposes to remove support for using `go get` to download source into $GOROOT.

This CL also proposes an error when GOPATH=$GOROOT, as this is another place where new Go users can get stuck.

Further discussion: https://groups.google.com/d/topic/golang-nuts/VIg3fjHiHRI/discussion

[Dustin, 2012-12-15 01:13:41]

I agree with the change, but I think it should include a tl;dr to onramp people
rapidly.

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

https://codereview.appspot.com/6941058/diff/2001/src/cmd/go/get.go#newcode256
src/cmd/go/get.go:256: return fmt.Errorf("cannot download, $GOPATH not set. For
more details see: go help gopath")
IMO, it'd be quite friendly to recommend someone set GOPATH to $HOME/go if
they're unsure, or similar right here.  Something quick such that they don't
believe $PWD is right and can get them going quickly, but without making an
awful mess.

[kisielk, 2012-12-15 01:14:25]

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

https://codereview.appspot.com/6941058/diff/2001/src/cmd/go/get.go#newcode256
src/cmd/go/get.go:256: return fmt.Errorf("cannot download, $GOPATH not set. For
more details see: go help gopath")
This message doesn't make sense in the case where list[0] == goroot, I think you
should add a separate if clause and message for that.

[dave_cheney.net, 2012-12-15 01:15:12]

> IMO, it'd be quite friendly to recommend someone set GOPATH to $HOME/go
> if they're unsure, or similar right here.  Something quick such that

I'm loath to recommend any $GOPATH setting because it will be highly
controversial.

[dave_cheney.net, 2012-12-15 01:35:43]

Thank you for your comments. PTAL.

For the moment, can we keep the discussion on the golang-nuts email thread, as
some may not subscribe to golang-dev.

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

https://codereview.appspot.com/6941058/diff/2001/src/cmd/go/get.go#newcode256
src/cmd/go/get.go:256: return fmt.Errorf("cannot download, $GOPATH not set. For
more details see: go help gopath")
On 2012/12/15 01:14:25, kamil.kisiel wrote:
> This message doesn't make sense in the case where list[0] == goroot, I think
you
> should add a separate if clause and message for that.

Done.

[minux1, 2012-12-15 13:21:29]

I don't understand the GOPATH in this sentence quoted from the description.

Back in the day, before the Go 1.0 release, $GOPATH was mandatory for building
from source.

https://codereview.appspot.com/6941058/diff/8001/src/cmd/go/test.bash
File src/cmd/go/test.bash (right):

https://codereview.appspot.com/6941058/diff/8001/src/cmd/go/test.bash#newcode189
src/cmd/go/test.bash:189: echo 'go get get
code.google.com/p/go.codereview/cmd/hgpatch should not succeed with $GOPATH
unset'
go get get?

https://codereview.appspot.com/6941058/diff/8001/src/cmd/go/test.bash#newcode197
src/cmd/go/test.bash:197: echo 'go get get
code.google.com/p/go.codereview/cmd/hgpatch should not succeed with
GOPATH=$GOROOT'
ditto.

[dave_cheney.net, 2012-12-15 13:22:44]

Awww heck, it is so hard to keep these too straight. PTAL.
On 16 Dec 2012 00:21, <minux.ma@gmail.com> wrote:

> I don't understand the GOPATH in this sentence quoted from the
> description.
>
> Back in the day, before the Go 1.0 release, $GOPATH was mandatory for
> building
> from source.
>
>
>
https://codereview.appspot.**com/6941058/diff/8001/src/cmd/**go/test.bash<htt...
> File src/cmd/go/test.bash (right):
>
> https://codereview.appspot.**com/6941058/diff/8001/src/cmd/**
>
go/test.bash#newcode189<https://codereview.appspot.com/6941058/diff/8001/src/cmd/go/test.bash#newcode189>
> src/cmd/go/test.bash:189: echo 'go get get
>
code.google.com/p/go.**codereview/cmd/hgpatch<http://code.google.com/p/go.codereview/cmd/hgpatch>should
not succeed with
> $GOPATH unset'
> go get get?
>
> https://codereview.appspot.**com/6941058/diff/8001/src/cmd/**
>
go/test.bash#newcode197<https://codereview.appspot.com/6941058/diff/8001/src/cmd/go/test.bash#newcode197>
> src/cmd/go/test.bash:197: echo 'go get get
>
code.google.com/p/go.**codereview/cmd/hgpatch<http://code.google.com/p/go.codereview/cmd/hgpatch>should
not succeed with
> GOPATH=$GOROOT'
> ditto.
>
>
https://codereview.appspot.**com/6941058/<https://codereview.appspot.com/6941...
>

[dave_cheney.net, 2012-12-19 11:04:11]

Please take another look.

[dave_cheney.net, 2012-12-19 11:05:55]

Hello dsallings@gmail.com, kamil.kisiel@gmail.com, minux.ma@gmail.com (cc:
golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2012-12-19 17:13:20]

LGTM

https://codereview.appspot.com/6941058/diff/18001/src/cmd/go/test.bash
File src/cmd/go/test.bash (right):

https://codereview.appspot.com/6941058/diff/18001/src/cmd/go/test.bash#newcod...
src/cmd/go/test.bash:187: mkdir -p $d/src/pkg
why src/pkg and not just src?

Or do you want $d/{src,pkg,bin} ?

https://codereview.appspot.com/6941058/diff/18001/src/cmd/go/test.bash#newcod...
src/cmd/go/test.bash:195: mkdir -p $d/src/pkg
likewise

[minux1, 2012-12-19 17:19:43]

I think you will need to document this change in doc/go1.1.html.

https://codereview.appspot.com/6941058/diff/18001/src/cmd/go/doc.go
File src/cmd/go/doc.go (right):

https://codereview.appspot.com/6941058/diff/18001/src/cmd/go/doc.go#newcode450
src/cmd/go/doc.go:450: GOPATH must be set to get, build and install packages
outside the
this is wrong, because even the package is in the standard Go tree,
go get can't be used without GOPATH being set.

Or, are you intentionally leaving that possibility?

https://codereview.appspot.com/6941058/diff/18001/src/cmd/go/help.go
File src/cmd/go/help.go (right):

https://codereview.appspot.com/6941058/diff/18001/src/cmd/go/help.go#newcode189
src/cmd/go/help.go:189: GOPATH must be set to get, build and install packages
outside the
ditto.

[dave_cheney.net, 2012-12-20 01:05:05]

+ {rsc,adg}

Thank you for your comments, PTAL.

https://codereview.appspot.com/6941058/diff/18001/src/cmd/go/doc.go
File src/cmd/go/doc.go (right):

https://codereview.appspot.com/6941058/diff/18001/src/cmd/go/doc.go#newcode450
src/cmd/go/doc.go:450: GOPATH must be set to get, build and install packages
outside the
On 2012/12/19 17:19:43, minux wrote:
> this is wrong, because even the package is in the standard Go tree,
> go get can't be used without GOPATH being set.
> 
> Or, are you intentionally leaving that possibility?

I think this might be a wording issue, not a surreptitious choice. go build
builds, go install installs, go get, downloads, so technically you don't need
$GOPATH to be set if go get does not need to download anything (ie, the pkg is
already presented locally).

What would you suggest ? Remove the 'get', change it to 'download', or something
else ?

https://codereview.appspot.com/6941058/diff/18001/src/cmd/go/help.go
File src/cmd/go/help.go (right):

https://codereview.appspot.com/6941058/diff/18001/src/cmd/go/help.go#newcode189
src/cmd/go/help.go:189: GOPATH must be set to get, build and install packages
outside the
On 2012/12/19 17:19:43, minux wrote:
> ditto.

See previous

https://codereview.appspot.com/6941058/diff/18001/src/cmd/go/test.bash
File src/cmd/go/test.bash (right):

https://codereview.appspot.com/6941058/diff/18001/src/cmd/go/test.bash#newcod...
src/cmd/go/test.bash:187: mkdir -p $d/src/pkg
On 2012/12/19 17:13:20, bradfitz wrote:
> why src/pkg and not just src?
> 
> Or do you want $d/{src,pkg,bin} ?

yes, that was what I intended, but thinking about it, neither are needed. PTAL.

https://codereview.appspot.com/6941058/diff/18001/src/cmd/go/test.bash#newcod...
src/cmd/go/test.bash:195: mkdir -p $d/src/pkg
On 2012/12/19 17:13:20, bradfitz wrote:
> likewise

Done.

[dave_cheney.net, 2012-12-20 01:46:59]

Sorry, forgot the go1.1.html change, please hold off on reviews for the moment.

[dave_cheney.net, 2012-12-20 05:26:01]

Hello dsallings@gmail.com, kamil.kisiel@gmail.com, minux.ma@gmail.com,
bradfitz@golang.org, rsc@golang.org, adg@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[minux1, 2012-12-21 19:21:34]

the gobuilder assumes the old 'go get' behavior, please fix the gobuilder
and make sure every builder is updated before submitting this CL.

[adg, 2013-01-02 03:37:24]

https://codereview.appspot.com/6941058/diff/24006/doc/go1.1.html
File doc/go1.1.html (right):

https://codereview.appspot.com/6941058/diff/24006/doc/go1.1.html#newcode65
doc/go1.1.html:65: <p>The <code>go</code> tool has acquired several improvements
which are intended to improve the experience for new Go users.</p>
The go tool has been made more user friendly.

https://codereview.appspot.com/6941058/diff/24006/doc/go1.1.html#newcode67
doc/go1.1.html:67: <p>Firstly, when compiling, testing, or running Go code, the
<code>go</code> tool will now give more detailed errors messages, including a
list of paths searched, when a package cannot be located.
Firstly isn't a word.

https://codereview.appspot.com/6941058/diff/24006/doc/go1.1.html#newcode67
doc/go1.1.html:67: <p>Firstly, when compiling, testing, or running Go code, the
<code>go</code> tool will now give more detailed errors messages, including a
list of paths searched, when a package cannot be located.
The go tool now gives more detailed error messages. For example:

https://codereview.appspot.com/6941058/diff/24006/doc/go1.1.html#newcode78
doc/go1.1.html:78: Secondly, the <code>go get</code> command no longer allows
<code>$GOROOT</code> as the default destination when downloading package source.
To use <code>go get</code> command, a valid <code>$GOPATH</code> is now
required.
The go get command will no longer install to $GOROOT. To use...

https://codereview.appspot.com/6941058/diff/24006/doc/go1.1.html#newcode86
doc/go1.1.html:86: <p>Finally, as a result of the previous change, the <code>go
get</code> command will also fail when <code>$GOPATH</code> and
<code>$GOROOT</code> are set to the same value.
Because of these changes, the go get command will fail when $GOPATH and $GOROOT
are set to the same value.

https://codereview.appspot.com/6941058/diff/24006/src/cmd/go/get.go
File src/cmd/go/get.go (right):

https://codereview.appspot.com/6941058/diff/24006/src/cmd/go/get.go#newcode257
src/cmd/go/get.go:257: return fmt.Errorf("cannot download, $GOPATH must not be
set to $GOROOT. For more details see: go help gopath")
s/For more details s/S/

[dave_cheney.net, 2013-01-03 00:56:00]

Hi adg,

Thank you for your comments. I will address them and address the issue of the
dashboard builders relying on the old behaviour soon. 

Apart from my poor word smithing, do you have any high level objections to this
proposal ?

Cheers

Dave

[adg, 2013-01-03 01:03:16]

No objections. I'm pretty happy with this.
On 3 Jan 2013 11:56, <dave@cheney.net> wrote:

> Hi adg,
>
> Thank you for your comments. I will address them and address the issue
> of the dashboard builders relying on the old behaviour soon.
>
> Apart from my poor word smithing, do you have any high level objections
> to this proposal ?
>
> Cheers
>
> Dave
>
>
https://codereview.appspot.**com/6941058/<https://codereview.appspot.com/6941...
>

[dave_cheney.net, 2013-01-03 22:28:32]

https://codereview.appspot.com/7034049/ addresses the changes required to the
dashboard builder.

[dave_cheney.net, 2013-01-03 22:51:46]

Done, thank you for your comments.

https://codereview.appspot.com/6941058/diff/24006/doc/go1.1.html
File doc/go1.1.html (right):

https://codereview.appspot.com/6941058/diff/24006/doc/go1.1.html#newcode65
doc/go1.1.html:65: <p>The <code>go</code> tool has acquired several improvements
which are intended to improve the experience for new Go users.</p>
On 2013/01/02 03:37:24, adg wrote:
> The go tool has been made more user friendly.

Done.

https://codereview.appspot.com/6941058/diff/24006/doc/go1.1.html#newcode67
doc/go1.1.html:67: <p>Firstly, when compiling, testing, or running Go code, the
<code>go</code> tool will now give more detailed errors messages, including a
list of paths searched, when a package cannot be located.
On 2013/01/02 03:37:24, adg wrote:
> The go tool now gives more detailed error messages. For example:

Done.

https://codereview.appspot.com/6941058/diff/24006/doc/go1.1.html#newcode78
doc/go1.1.html:78: Secondly, the <code>go get</code> command no longer allows
<code>$GOROOT</code> as the default destination when downloading package source.
To use <code>go get</code> command, a valid <code>$GOPATH</code> is now
required.
On 2013/01/02 03:37:24, adg wrote:
> The go get command will no longer install to $GOROOT. To use...

Done.

https://codereview.appspot.com/6941058/diff/24006/doc/go1.1.html#newcode86
doc/go1.1.html:86: <p>Finally, as a result of the previous change, the <code>go
get</code> command will also fail when <code>$GOPATH</code> and
<code>$GOROOT</code> are set to the same value.
On 2013/01/02 03:37:24, adg wrote:
> Because of these changes, the go get command will fail when $GOPATH and
$GOROOT
> are set to the same value.

Done.

https://codereview.appspot.com/6941058/diff/24006/src/cmd/go/get.go
File src/cmd/go/get.go (right):

https://codereview.appspot.com/6941058/diff/24006/src/cmd/go/get.go#newcode257
src/cmd/go/get.go:257: return fmt.Errorf("cannot download, $GOPATH must not be
set to $GOROOT. For more details see: go help gopath")
On 2013/01/02 03:37:24, adg wrote:
> s/For more details s/S/

Done.

[dave_cheney.net, 2013-01-03 22:54:52]

Hello dsallings@gmail.com, kamil.kisiel@gmail.com, minux.ma@gmail.com,
bradfitz@golang.org, rsc@golang.org, adg@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[adg, 2013-01-06 21:32:00]

LGTM

But wait for rsc, too.

[dave_cheney.net, 2013-01-06 21:37:46]

Of course. The next step is to work with the builder owners to get their
builders updated to the new version. Then we can return to this CL. 

On 07/01/2013, at 8:32, adg@golang.org wrote:

> LGTM
> 
> But wait for rsc, too.
> 
> https://codereview.appspot.com/6941058/

[dave_cheney.net, 2013-01-08 23:31:36]

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

Please take another look.

[dave_cheney.net, 2013-01-08 23:35:01]

On 2013/01/08 23:31:36, dfc wrote:
> Hello mailto:rsc@golang.org, mailto:adg@golang.org (cc:
mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

With two exceptions the new builder code has been rolled out. 

@rsc, can I ask for some of your time to review this CL acknowledging that I
would not submit until the remaining builders have been updated.

[rsc, 2013-01-09 20:58:10]

I will leave the code review to adg. The new behavior is fine with me. I'm a
little sad that we can't have nice things but I understand the reason.

[minux1, 2013-01-09 21:15:01]

On Thu, Jan 10, 2013 at 4:58 AM, <rsc@golang.org> wrote:

> I will leave the code review to adg. The new behavior is fine with me.
> I'm a little sad that we can't have nice things but I understand the
> reason.
>
Out of curiosity, what do you mean by saying "we can't have nice things"?

[rsc, 2013-01-09 21:55:25]

>> I will leave the code review to adg. The new behavior is fine with me.
>> I'm a little sad that we can't have nice things but I understand the
>> reason.
>
> Out of curiosity, what do you mean by saying "we can't have nice things"?

I am not sure where the phrase comes from. Typically it is like "This
is why we can't have nice things", said after someone has broken
something, actually or metaphorically. The internet is failing me for
finding a page explaining it better.

In this case, I meant that I regret that we cannot provide the ability
to install into GOROOT. We are being forced to take it out because it
confuses users, even though it helps (some) core developers. It's not
a big deal. I agree with taking it out, I'm just a little sad to see
it go.

Russ

[adg, 2013-01-09 22:05:09]

On 10 January 2013 08:55, Russ Cox <rsc@golang.org> wrote:

> I am not sure where the phrase comes from. Typically it is like "This
> is why we can't have nice things", said after someone has broken
> something, actually or metaphorically. The internet is failing me for
> finding a page explaining it better.
>

http://knowyourmeme.com/memes/this-is-why-we-cant-have-nice-things

[adg, 2013-01-09 22:05:14]

LGTM

[dave_cheney.net, 2013-01-09 22:08:26]

Thank you. Please accept my apologies for making the go tool a little
less awesome.

On Thu, Jan 10, 2013 at 9:05 AM,  <adg@golang.org> wrote:
> LGTM
>
> https://codereview.appspot.com/6941058/

[dave_cheney.net, 2013-01-09 22:57:23]

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

cmd/go: remove $GOROOT as a go get target

Fixes issue 4186.

Back in the day, before the Go 1.0 release, $GOROOT was mandatory for building
from source. Fast forward to now and $GOPATH is mandatory and $GOROOT is
optional, and mainly used by those who use the binary distribution in uncommon
places.

For example, most novices at least know about `sudo` as they would have used it
to install the binary tarball into /usr/local. It is logical they would use the
`sudo` hammer to `go get` other Go packages when faced with a permission error
talking about the path they just had to use `sudo` on last time.

Even if they had read the documentation and set $GOPATH, go get will not work as
expected as `sudo` masks most environment variables.

llucky(~) % ~/go/bin/go env | grep GOPATH
GOPATH="/home/dfc"
lucky(~) % sudo ~/go/bin/go env | grep GOPATH
GOPATH=""

This CL therefore proposes to remove support for using `go get` to download
source into $GOROOT.

This CL also proposes an error when GOPATH=$GOROOT, as this is another place
where new Go users can get stuck.

Further discussion:
https://groups.google.com/d/topic/golang-nuts/VIg3fjHiHRI/discussion

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