From 9cad0cc6e6b2a84134c46ce7069e62de28459f26 Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Mon, 17 Apr 2023 17:37:37 -0400 Subject: [PATCH] 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 Run-TryBot: Dmitri Shuralyov TryBot-Result: Gopher Robot Reviewed-by: Dmitri Shuralyov Auto-Submit: Dmitri Shuralyov --- misc/reboot/reboot_test.go | 22 ++++++++++++++++++++-- src/all.bash | 2 -- src/all.bat | 5 ----- src/cmd/dist/build.go | 23 +++++++++++++++++++++-- 4 files changed, 41 insertions(+), 11 deletions(-) diff --git a/misc/reboot/reboot_test.go b/misc/reboot/reboot_test.go index c4a9f3ef9f..94d61e000e 100644 --- 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 5d994d3d0d..5f8e8fec63 100755 --- 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 dfc83c8b26..d5abec141f 100644 --- a/src/all.bat +++ b/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 diff --git a/src/cmd/dist/build.go b/src/cmd/dist/build.go index 6dbc9951a9..e460b2d1cc 100644 --- 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) } } -- 2.44.0