diff options
author | Dmitri Shuralyov <dmitshur@golang.org> | 2023-04-17 17:37:37 -0400 |
---|---|---|
committer | Gopher Robot <gobot@golang.org> | 2023-04-19 14:36:22 +0000 |
commit | 9cad0cc6e6b2a84134c46ce7069e62de28459f26 (patch) | |
tree | 1f98d2e353e652749636d0bceaf3590c80df8f17 | |
parent | 8fa9e3beee8b0e6baa7333740996181268b60a3a (diff) | |
download | go-9cad0cc6e6b2a84134c46ce7069e62de28459f26.tar.gz go-9cad0cc6e6b2a84134c46ce7069e62de28459f26.zip |
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>
-rw-r--r-- | misc/reboot/reboot_test.go | 22 | ||||
-rwxr-xr-x | src/all.bash | 2 | ||||
-rw-r--r-- | src/all.bat | 5 | ||||
-rw-r--r-- | 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) } } |