Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
bradfitz opened this issue Jan 6, 2017 · 22 comments

Comments

@bradfitz
Copy link
Member

commented Jan 6, 2017

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.

@bradfitz bradfitz added this to the Go1.9 milestone Jan 6, 2017

@rakyll

This comment has been minimized.

Copy link
Member

commented Jan 6, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Jan 6, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2017

@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: #16352

@robpike

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2017

I like building from 1.4. It's faster.

@nathany

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2017

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

@nathany

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2017

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

This comment has been minimized.

Copy link

commented Apr 3, 2017

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

This comment has been minimized.

Copy link
Member

commented Apr 4, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2017

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

This comment has been minimized.

Copy link

commented Jun 8, 2017

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

@derekbruening

This comment has been minimized.

Copy link

commented Jun 8, 2017

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/

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 Jun 13, 2017

@rsc rsc modified the milestones: Go1.10, Go1.11 Dec 1, 2017

@rasky

This comment has been minimized.

Copy link
Member

commented Feb 21, 2018

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:

go/src/make.bash

Lines 59 to 61 in 79fe895

# GOROOT_BOOTSTRAP: A working Go tree >= Go 1.4 for bootstrap.
# If $GOROOT_BOOTSTRAP/bin/go is missing, $(go env GOROOT) is
# tried for all "go" in $PATH. $HOME/go1.4 by default.

go/src/make.bash

Lines 140 to 150 in 79fe895

export GOROOT_BOOTSTRAP=${GOROOT_BOOTSTRAP:-$HOME/go1.4}
export GOROOT="$(cd .. && pwd)"
IFS=$'\n'; for go_exe in $(type -ap go); do
if [ ! -x "$GOROOT_BOOTSTRAP/bin/go" ]; then
goroot=$(GOROOT='' "$go_exe" env GOROOT)
if [ "$goroot" != "$GOROOT" ]; then
GOROOT_BOOTSTRAP=$goroot
fi
fi
done; unset IFS
echo "Building Go cmd/dist using $GOROOT_BOOTSTRAP."

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

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 21, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2018

@rasky

This comment has been minimized.

Copy link
Member

commented Feb 21, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2018

@rasky

This comment has been minimized.

Copy link
Member

commented Feb 22, 2018

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:

  • Install a binary version of Go through the package manager
  • Clone Go's git master
  • Build it

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

This comment has been minimized.

Copy link
Member

commented Feb 22, 2018

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

This comment has been minimized.

Copy link

commented Feb 22, 2018

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

@rasky

This comment has been minimized.

Copy link
Member

commented Feb 22, 2018

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.

@gopherbot gopherbot modified the milestones: Go1.11, Unplanned May 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.