make.{bash,bat}: check unmodified $PATH for $GOROOT/bin presence

Previously, all.bash and all.bat restored the original $PATH before
calling 'dist banner', so that it would do its job of checking whether
the user still needs to add $GOROOT/bin to their $PATH. That worked for
those scripts, but had no effect on make.bash nor make.bat.

Instead of trying to extend that logic to more scripts, change the
approach to provide dist an unmodified copy of $PATH via an internal
to dist environment variable $DIST_UNMODIFIED_PATH. The make.bash and
make.bat scripts happen to use dist env -p to modify $PATH, making it
viable to add the internal variable there instead of in each script.

It currently works by adding semicolon terminators to dist env output
so that make.bash's 'eval $(dist env -p)' works as before but is able to
export DIST_UNMODIFIED_PATH for following dist invocations to observe.
Nothing needs to be done for Windows since its 'set ENV=val' format
already has that effect.

Plan 9 doesn't use the -p flag of dist env, and checks that GOROOT/bin
is bound before /bin rather than looking at the $PATH env var like other
OSes, so it may not have this bug. I don't have easy access to Plan 9
and haven't tried to confirm.

Fixes #42563.

Change-Id: I74691931167e974a930f7589d22a48bb6b931163
Reviewed-on: https://go-review.googlesource.com/c/go/+/485896
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>

misc/reboot/reboot_test.go

@@ -7,10 +7,13 @@
 package reboot_test
 
 import (
+	"fmt"
+	"io"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"runtime"
+	"strings"
 	"testing"
 	"time"
 )
@@ -67,12 +70,27 @@ func TestRepeatBootstrap(t *testing.T) {
 		makeScript = "make.bash"
 	}
 
+	var stdout strings.Builder
 	cmd := exec.Command(filepath.Join(goroot, "src", makeScript))
 	cmd.Dir = gorootSrc
-	cmd.Env = append(cmd.Environ(), "GOROOT=", "GOROOT_BOOTSTRAP="+realGoroot)
+	cmd.Env = append(cmd.Environ(), "GOROOT=", "GOROOT_FINAL=", "GOROOT_BOOTSTRAP="+realGoroot)
 	cmd.Stderr = os.Stderr
-	cmd.Stdout = os.Stdout
+	cmd.Stdout = io.MultiWriter(os.Stdout, &stdout)
 	if err := cmd.Run(); err != nil {
 		t.Fatal(err)
 	}
+
+	// Test that go.dev/issue/42563 hasn't regressed.
+	t.Run("PATH reminder", func(t *testing.T) {
+		var want string
+		switch gorootBin := filepath.Join(goroot, "bin"); runtime.GOOS {
+		default:
+			want = fmt.Sprintf("*** You need to add %s to your PATH.", gorootBin)
+		case "plan9":
+			want = fmt.Sprintf("*** You need to bind %s before /bin.", gorootBin)
+		}
+		if got := stdout.String(); !strings.Contains(got, want) {
+			t.Errorf("reminder %q is missing from %s stdout:\n%s", want, makeScript, got)
+		}
+	})
 }

src/all.bash

@@ -8,8 +8,6 @@ if [ ! -f make.bash ]; then
 	echo 'all.bash must be run from $GOROOT/src' 1>&2
 	exit 1
 fi
-OLDPATH="$PATH"
 . ./make.bash "$@" --no-banner
 bash run.bash --no-rebuild
-PATH="$OLDPATH"
 $GOTOOLDIR/dist banner  # print build info

src/all.bat

@@ -12,15 +12,10 @@ echo all.bat must be run from go\src
 goto end
 :ok
 
-set OLDPATH=%PATH%
 call .\make.bat --no-banner --no-local
 if %GOBUILDFAIL%==1 goto end
 call .\run.bat --no-rebuild --no-local
 if %GOBUILDFAIL%==1 goto end
-:: we must restore %PATH% before running "dist banner" so that the latter
-:: can get the original %PATH% and give suggestion to add %GOROOT%/bin
-:: to %PATH% if necessary.
-set PATH=%OLDPATH%
 "%GOTOOLDIR%/dist" banner
 
 :end

src/cmd/dist/build.go

@@ -1252,7 +1252,7 @@ func cmdenv() {
 	windows := flag.Bool("w", gohostos == "windows", "emit windows syntax")
 	xflagparse(0)
 
-	format := "%s=\"%s\"\n"
+	format := "%s=\"%s\";\n" // Include ; to separate variables when 'dist env' output is used with eval.
 	switch {
 	case *plan9:
 		format = "%s='%s'\n"
@@ -1299,6 +1299,17 @@ func cmdenv() {
 			sep = ";"
 		}
 		xprintf(format, "PATH", fmt.Sprintf("%s%s%s", gorootBin, sep, os.Getenv("PATH")))
+
+		// Also include $DIST_UNMODIFIED_PATH with the original $PATH
+		// for the internal needs of "dist banner", along with export
+		// so that it reaches the dist process. See its comment below.
+		var exportFormat string
+		if !*windows && !*plan9 {
+			exportFormat = "export " + format
+		} else {
+			exportFormat = format
+		}
+		xprintf(exportFormat, "DIST_UNMODIFIED_PATH", os.Getenv("PATH"))
 	}
 }
 
@@ -1897,7 +1908,15 @@ func banner() {
 		if gohostos == "windows" {
 			pathsep = ";"
 		}
-		if !strings.Contains(pathsep+os.Getenv("PATH")+pathsep, pathsep+gorootBin+pathsep) {
+		path := os.Getenv("PATH")
+		if p, ok := os.LookupEnv("DIST_UNMODIFIED_PATH"); ok {
+			// Scripts that modify $PATH and then run dist should also provide
+			// dist with an unmodified copy of $PATH via $DIST_UNMODIFIED_PATH.
+			// Use it here when determining if the user still needs to update
+			// their $PATH. See go.dev/issue/42563.
+			path = p
+		}
+		if !strings.Contains(pathsep+path+pathsep, pathsep+gorootBin+pathsep) {
 			xprintf("*** You need to add %s to your PATH.\n", gorootBin)
 		}
 	}