golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.99k stars 17.67k forks source link

doc: contribute.html doesn't mention GOROOT_BOOTSTRAP #18545

Open bradfitz opened 7 years ago

bradfitz commented 7 years ago

Users should be able to find everything they need to know to contribute to Go by reading contribute.html.

There's no path from that page to learning about GOROOT_BOOTSTRAP, since GOROOT_BOOTSTRAP is only mentioned from the unlinked https://golang.org/doc/install/source

Fix.

rakyll commented 7 years ago

What do you mean by unlinked? Installation is the first link provided in the intro section:

This document explains how to contribute changes to the Go project. It assumes you have followed the installation instructions and have written and tested your code.

I fear that installation guide is not well highlighted on that page. What about creating a new header for "Installing from source" as the first step?

bradfitz commented 7 years ago

Oh, indeed.

I guess I was searching for something else on that page, like "source".

But the page is still misleading. Under "Testing redux" it says:

You've written and tested your code,

And that links to the GOPATH documentation page, but GOPATH is totally unnecessary for hacking on the Go core. (It's only necessary for subrepos)

robpike commented 7 years ago

On related note, something that's thrown me before (although I know too much, so it may not matter to others) is that the official 1.4 download doesn't work on Macs. You need to use the special 1.4 download link on the build-from-source page. It would be nice if either the official 1.4 download were updated, or if another one were added with an annotation on the downloads page.

nathany commented 7 years ago

@robpike As I'm sure you already know, Go 1.6.x and most of the archived versions have issues on macOS Sierra. At the same time, all those releases should work on prior versions of macOS (< 10.12), including the official Go 1.4 download.

Maybe we just need a note on the download page that macOS Sierra requires Go 1.7 or better?

cross-reference to backport issue: https://github.com/golang/go/issues/16352

robpike commented 7 years ago

I like building from 1.4. It's faster.

nathany commented 7 years ago

I'll split the macOS issue to a separate issue: #18554.

nathany commented 7 years ago

What about creating a new header for "Installing from source" as the first step?

đź‘Ť Or perhaps the second step, as installing from source technically isn't required before discussing the idea.

that links to the GOPATH documentation page

Yah. There are a few links out to "How to Write Go Code". Maybe Contributing should be more self-contained? Of course that may be at odds with #17802.

When Alex brought this up, it sounds like he was looking for guidance for working on the core Go code. https://groups.google.com/d/msg/golang-dev/afkXKoAd8IQ/NECZ_PWcEAAJ

jpudney commented 7 years ago

As somebody who wants to contribute code (but yet to) I was caught by this:

You've written and tested your code

Using the contribution guide and following this article I was able to get the source downloaded and able to make changes but I was left wondering whether I was able to use go test to selectively test my changes before running ./all.bash, @vcabbage was also able to help with on slack:

You can use go test. You'll want to use the go binary built from the version you're working on. > Use ./make.bash and $PATH_TO_GO_REPO/bin/go test. If you're making changes to the toolchain code instead of just stdlib you'll want to recompile the toolchain each time.

Using this info and his article lead me to gotip test, and I was sorted.

I think the picking out the salient facts from above and adding them to the contribution guidelines may help.

ALTree commented 7 years ago

@jpudney that's a different issue, though. This one is about GOROOT_BOOTSTRAP. You could open a new issue asking for a clarification of the process of testing a new contribution.

ggirtsou commented 7 years ago

This issue got me as well and spend quite some time on it. Even though it is mentioned in "building from source" page, I think we should mention it on contributing page as well. GOROOT_BOOTSTRAP needs to point to a Go installation, and you should run tests like this:

# first cd to the directory you checked out go code
GOROOT_BOOTSTRAP=/usr/local/go ./all.bash

Alternatively, env variable can be set in ~/.profile to avoid typing it again.

If it's not set, you get this:

$ ./all.bash 
##### Building Go bootstrap tool.
cmd/dist
ERROR: Cannot find /home/george/go1.4/bin/go.
Set $GOROOT_BOOTSTRAP to a working Go tree >= Go 1.4.

That error didn't help me much, as I thought it asked me to set $GOROOT_BOOTSTRAP to go source code, and not the binary release.

Here's another error I encountered when I tried to run tests for a specific package:

# ~/go/src/github.com/golang/go/src
$ go test github.com/golang/go/src/os -run WriteAt
os/file_unix.go:10:2: cannot find package "internal/poll" in any of:
    /home/george/go/src/github.com/golang/go/src/vendor/internal/poll (vendor tree)
    /usr/local/go/src/internal/poll (from $GOROOT)
    /home/george/go/src/internal/poll (from $GOPATH)

I read that checking out go code should not be under your $GOPATH, but I tried a few things to see what'd work.

This works, though: GOROOT_BOOTSTRAP=/usr/local/go ./all.bash

Testing changes in individual package:

Perhaps, this should go on a separate issue to make changes in contribute page, but I'll mention it here anyway. I was trying to test my changes on os package with this command: $ go test os -run WriteAt but I didn't remember it checks $GOROOT (in my case /usr/local/go/src/os) first.

I was expecting my tests to fail, but they were passing. Even though my current working directory was src of checked out Go source code, it was still checking $GOROOT first. This was a bit frustrating.

I still run the whole test suite which is not ideal if you made small changes. Would be nice if I could a specific test: without getting the error about missing internal/poll package. At the end, before pushing my changes, I'd run the whole test suite.

Please let me know your thoughts.

gopherbot commented 7 years ago

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

derekbruening commented 7 years ago

I took a stab at this, adding a step prior to the "go get ... git-codereview" section as a working go is needed for that as well: https://go-review.googlesource.com/c/45140/

rasky commented 6 years ago

I've submitted for review a new contribution guide (https://go-review.googlesource.com/c/go/+/93495), that aims to simplify readability as much as possible. I'm reviewing pending issues to see how the requests would fit into the new guide.

Can somebody explain me when exactly GOROOT_BOOTSTRAP must necessarily be set? I understand that it can be used to force a specific version of Go being used for bootstrap, but I would say most contributors would simply use their standard Go binary installation to bootstrap. It looks like it's an option for advanced uses, not something that everybody should be aware of.

@ggirtsou you say that you get an error running all.bash if it's not set. On my macOS, running all.bash without GOROOT_BOOTSTRAP correctly detects the homebrew-installed tree:

↪ ./all.bash
Building Go cmd/dist using /usr/local/Cellar/go/1.10rc2/libexec.
[...]

There is some code in make.bash that goes through all go binaries in the $PATH and tries to find a good default for GOROOT_BOOTSTRAP, using any of the existing installations:

https://github.com/golang/go/blob/79fe895112dc3759506e57c519a2b38c41ee71dd/src/make.bash#L59-L61 https://github.com/golang/go/blob/79fe895112dc3759506e57c519a2b38c41ee71dd/src/make.bash#L140-L150

So my understanding is that ./all.bash (without setting GOROOT_BOOTSTRAP) should work for all contributors that have at least one (binary) version of Go correctly installed on their system, which is a prerequisite for contribution and a reasonable default to assume for the readers of the document.

What am I missing? @ggirtsou, can you check why the above code doesn't work for you on your system, leaving you forced to manually set a GOROOT_BOOTSTRAP?

ianlancetaylor commented 6 years ago

You need GOROOT_BOOTSTRAP the first time you are building Go on your system, if you don't want to start by downloading a binary package.

rasky commented 6 years ago

Ok then it’s fair to assume that most Go contributors will be Go users in the first place and will have a Go binary installed. Building Go without Go seems a subject for another page, not the contribution guide.

davecheney commented 6 years ago

You need to build Go from source if you want to modify it and contribute a CL.

On 22 February 2018 at 09:24, Giovanni Bajo notifications@github.com wrote:

Ok then it’s fair to assume that most Go contributors will be Go users in the first place and will have a Go binary installed. Building Go without Go seems a subject for another page, not the contribution guide.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/18545#issuecomment-367497018, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAcA6MD0zNSnFajWj-LypBQA48oZ8Bfks5tXJebgaJpZM4LdBl- .

rasky commented 6 years ago

You can build from source without GOROOT_BOOTSTRAP. What Ian says: you need it if you want to build from source WITHOUT a version of Go installed on your system, which I think it’s not a frequent scenario.

davecheney commented 6 years ago

You can build from source without GOROOT_BOOTSTRAP.

To build from source, you need to have a bootstrap compiler installed. GOROOT_BOOTSTRAP is how say where that bootstrap compiler lives.

On 22 February 2018 at 10:52, Giovanni Bajo notifications@github.com wrote:

You can build from source without GOROOT_BOOTSTRAP. What Ian says: you need it if you want to build from source WITHOUT a version of Go installed on your system, which I think it’s not a frequent scenario.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/18545#issuecomment-367518816, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAcAy0IX7iMJa4qbblS_fVHAy0sT88jks5tXKxYgaJpZM4LdBl- .

rasky commented 6 years ago

GOROOT_BOOTSTRAP is how say where that bootstrap compiler lives.

But GOROOT_BOOTSTRAP has a very good default: it defaults to whatever Go version is installed on your computer. So you don't need to specify it most of the times, and thus I don't think it belongs to the contribution guide. It's an information that fits best a guide on "How to build Go, including advanced options" rather than being inserted in the flow of a first-time contributor that just wants to send a small first contribution.

To make sure we're talking of the same thing, I tried with a clean Ubuntu (through a standard Docker image):

↪ docker run -it ubuntu
root@d116025bca83:/# go
bash: go: command not found         # go is not installed

root@d116025bca83:/# apt-get update
Get:1 http://archive.ubuntu.com/ubuntu xenial InRelease [247 kB]
Get:2 http://archive.ubuntu.com/ubuntu xenial-updates InRelease [102 kB]
[....]

root@d116025bca83:/# apt-get install golang
Reading package lists... Done
Building dependency tree
Reading state information... Done
The following additional packages will be installed:
  binutils build-essential bzip2 cpp cpp-5 dpkg-dev fakeroot g++ g++-5 gcc gcc-5 gcc-5-base golang-1.6 golang-1.6-doc golang-1.6-go golang-1.6-race-detector-runtime golang-1.6-src golang-doc
[....]

root@d116025bca83:/# go version
go version go1.6.2 linux/amd64

root@d116025bca83:/# apt-get install git
Reading package lists... Done
Building dependency tree
Reading state information... Done
The following additional packages will be installed:
  ca-certificates git-man ifupdown iproute2 isc-dhcp-client isc-dhcp-common krb5-locales less libasn1-8-heimdal libatm1 libbsd0 libcurl3-gnutls libdns-export162 libedit2 liberror-perl libexpat1
[...]

root@d116025bca83:/# git clone https://go.googlesource.com/go
Cloning into 'go'...
remote: Sending approximately 186.96 MiB ...
remote: Counting objects: 22, done
remote: Total 331237 (delta 256426), reused 331237 (delta 256426)
Receiving objects: 100% (331237/331237), 186.91 MiB | 8.92 MiB/s, done.
Resolving deltas: 100% (256426/256426), done.
Checking connectivity... done.

root@d116025bca83:/# cd go
root@d116025bca83:/go# cd src/
root@d116025bca83:/go/src# ./make.bash
Building Go cmd/dist using /usr/lib/go-1.6.
Building Go toolchain1 using /usr/lib/go-1.6.
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
Building Go toolchain2 using go_bootstrap and Go toolchain1.
Building Go toolchain3 using go_bootstrap and Go toolchain2.
Building packages and commands for linux/amd64.
---
Installed Go for linux/amd64 in /go
Installed commands in /go/bin
root@d116025bca83:/go/src# ../bin/go version
go version devel +1e05924 Thu Feb 22 07:55:14 2018 +0000 linux/amd64

So I was able to:

all without ever specifying GOROOT_BOOTSTRAP.

Thus, I think that GOROOT_BOOTSTRAP doesn't belong to the contribution guide. The maximum I reckon we should do (if we really feel it is necessary) is to modify the text about building Go with a small paragraph saying something along the lines of "if you want to learn more about different options to build Go, check [link].", with a link to a more in-depth guide on the bootstrap process.

vcabbage commented 6 years ago

The default GOROOT_BOOTSTRAP was added relatively recently (#14339) and I think it only works on *nix sytems. make.bat doesn't have similar logic as far as I can tell.

gopherbot commented 6 years ago

Change https://golang.org/cl/96455 mentions this issue: build: add default GOROOT_BOOTSTRAP in Windows

rasky commented 6 years ago

I've just mailed a CL for adding support for default GOROOT_BOOTSTRAP to Windows. After this CL goes in, I think this issue can be closed.