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>

diff --git a/misc/reboot/reboot_test.go b/misc/reboot/reboot_test.go
index c4a9f3ef9ff7d264d2133723beee89a889355bb4..94d61e000efca22432a7a7682a75b1f128d54d5b 100644 (file)
--- a/misc/reboot/reboot_test.go
+++ b/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)
+               }
+       })
 }

diff --git a/src/all.bash b/src/all.bash
index 5d994d3d0dae97f070d4299a90443519d387ffd4..5f8e8fec63dc43ff9ee0f34af900cd66844cc2d2 100755 (executable)
--- a/src/all.bash
+++ b/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

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

diff --git a/src/cmd/dist/build.go b/src/cmd/dist/build.go
index 6dbc9951a96ff1d86c7b246b20bd2ea37668aa2c..e460b2d1cca616394cae8b13758e2df6adc31101 100644 (file)
--- a/src/cmd/dist/build.go
+++ b/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)
                }
        }