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

make.{bash,bat,rc}, cmd/dist: important note to place right go binary in PATH is not printed via make.bash #42563

Open
dmitshur opened this issue Nov 12, 2020 · 1 comment
Labels
Milestone

Comments

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Nov 12, 2020

What did you do?

$ git clone https://go.googlesource.com/go $HOME/gotip
$ cd $HOME/gotip/src
src $ ./make.bash

What did you expect to see?

src $ ./make.bash
Building Go cmd/dist using /usr/local/go. (go1.15.4 darwin/amd64)
Building Go toolchain1 using /usr/local/go.
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 darwin/amd64.
---
Installed Go for darwin/amd64 in /Users/gopher/gotip
Installed commands in /Users/gopher/gotip/bin
*** You need to add /Users/gopher/gotip/bin to your PATH.

What did you see instead?

src $ ./make.bash
Building Go cmd/dist using /usr/local/go. (go1.15.4 darwin/amd64)
Building Go toolchain1 using /usr/local/go.
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 darwin/amd64.
---
Installed Go for darwin/amd64 in /Users/gopher/gotip
Installed commands in /Users/gopher/gotip/bin

Note the "*** You need to add /Users/gopher/gotip/bin to your PATH." line is missing.

That line is important because if it isn't done, and there happens to be another go binary in PATH (quite likely for Go developers), the wrong go binary will be used.

Cause

make.bash includes these lines:

# -e doesn't propagate out of eval, so check success by hand.
eval $(./cmd/dist/dist env -p || echo FAIL=true)
if [ "$FAIL" = true ]; then
	exit 1
fi

dist env -p includes a modified version of PATH in its output, which is then evaled.

The banner is printed afterwards with a check for whether PATH contains GOROOT/bin:

if !strings.Contains(pathsep+os.Getenv("PATH")+pathsep, pathsep+gobin+pathsep) {
	xprintf("*** You need to add %s to your PATH.\n", gobin)
}

Where os.Getenv("PATH") ends up containing gobin, but only because make.bash added it temporarily.

As far as I can tell, this same problem affected all.bash 8 years ago in #3699, and it was fixed in CL 6272048, so this is a matter of resolving this for when make.bash (or make.bat, make.rc) is invoked directly.

@dmitshur dmitshur added this to the Backlog milestone Nov 12, 2020
@bcmills
Copy link
Member

@bcmills bcmills commented Nov 12, 2020

I'm not entirely sure, but you might be able to add an assertion to the misc/reboot test to catch future regressions of this behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants