From 97a2ed74adbb389ce2e8da790dc3567e89e2af71 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Thu, 25 May 2023 23:28:39 -0400 Subject: [PATCH] cmd/go: add detection of toolchain switch loops This happens mainly during testing because the virtual toolchain switch is not terribly robust, and if you accidentally try to exec "1.23" instead of "go1.23" it will let you, but it won't work right. Of course, although we feel pretty good about the non-test implementation, perhaps it has a toolchain switch loop lurking too, or perhaps one will be introduced in the future. To handle the test bug, and just in case we have a real bug later, add detection of toolchain switch loops with clear messages. Also fixes a bug in setting the -lang flag properly when invoking the Go compiler: this is the first test using 'go 1.21.x' lines during a build. For #57001. Change-Id: I0ece3dd718596689a23b677cf08ddf32ea97bc57 Reviewed-on: https://go-review.googlesource.com/c/go/+/498436 Run-TryBot: Russ Cox Reviewed-by: Bryan Mills Auto-Submit: Russ Cox TryBot-Result: Gopher Robot --- src/cmd/go/go_test.go | 7 +- .../go/internal/gover/{latest.go => local.go} | 26 ++- src/cmd/go/internal/toolchain/exec.go | 2 +- src/cmd/go/internal/toolchain/toolchain.go | 148 ++++++++++++++---- src/cmd/go/internal/work/exec.go | 13 +- src/cmd/go/internal/work/gc.go | 3 +- src/cmd/go/main.go | 2 +- .../go/testdata/script/gotoolchain_local.txt | 2 +- .../go/testdata/script/gotoolchain_loop.txt | 65 ++++++++ .../script/mod_get_exec_toolchain.txt | 2 +- src/cmd/go/testdata/script/mod_toolchain.txt | 2 +- 11 files changed, 211 insertions(+), 61 deletions(-) rename src/cmd/go/internal/gover/{latest.go => local.go} (52%) create mode 100644 src/cmd/go/testdata/script/gotoolchain_loop.txt diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go index e50144f7f0..54249f6f7a 100644 --- a/src/cmd/go/go_test.go +++ b/src/cmd/go/go_test.go @@ -36,6 +36,7 @@ import ( "cmd/go/internal/gover" "cmd/go/internal/robustio" "cmd/go/internal/search" + "cmd/go/internal/toolchain" "cmd/go/internal/vcs" "cmd/go/internal/vcweb/vcstest" "cmd/go/internal/web" @@ -108,10 +109,8 @@ func TestMain(m *testing.M) { // We set CMDGO_TEST_RUN_MAIN via os.Setenv and testScript.setup. if os.Getenv("CMDGO_TEST_RUN_MAIN") != "" { cfg.SetGOROOT(cfg.GOROOT, true) - - if v := os.Getenv("TESTGO_VERSION"); v != "" { - gover.TestVersion = v - } + gover.TestVersion = os.Getenv("TESTGO_VERSION") + toolchain.TestVersionSwitch = os.Getenv("TESTGO_VERSION_SWITCH") if v := os.Getenv("TESTGO_TOOLCHAIN_VERSION"); v != "" { work.ToolchainVersion = v } diff --git a/src/cmd/go/internal/gover/latest.go b/src/cmd/go/internal/gover/local.go similarity index 52% rename from src/cmd/go/internal/gover/latest.go rename to src/cmd/go/internal/gover/local.go index 16357b8d30..8183a5c3d4 100644 --- a/src/cmd/go/internal/gover/latest.go +++ b/src/cmd/go/internal/gover/local.go @@ -17,14 +17,26 @@ var TestVersion string // Local returns the local Go version, the one implemented by this go command. func Local() string { - v := runtime.Version() + v, _ := local() + return v +} + +// LocalToolchain returns the local toolchain name, the one implemented by this go command. +func LocalToolchain() string { + _, t := local() + return t +} + +func local() (goVers, toolVers string) { + toolVers = runtime.Version() if TestVersion != "" { - v = TestVersion + toolVers = TestVersion } - if v := FromToolchain(v); v != "" { - return v + goVers = FromToolchain(toolVers) + if goVers == "" { + // Development branch. Use "Dev" version with just 1.N, no rc1 or .0 suffix. + goVers = "1." + strconv.Itoa(goversion.Version) + toolVers = "go" + goVers } - - // Development branch. Use "Dev" version with just 1.N, no rc1 or .0 suffix. - return "1." + strconv.Itoa(goversion.Version) + return goVers, toolVers } diff --git a/src/cmd/go/internal/toolchain/exec.go b/src/cmd/go/internal/toolchain/exec.go index 4e6a13e35f..820fe93e87 100644 --- a/src/cmd/go/internal/toolchain/exec.go +++ b/src/cmd/go/internal/toolchain/exec.go @@ -20,7 +20,7 @@ import ( // The GOROOT directory is empty if we are invoking a command named // gotoolchain found in $PATH. func execGoToolchain(gotoolchain, dir, exe string) { - os.Setenv(gotoolchainSwitchEnv, "1") + os.Setenv(targetEnv, gotoolchain) if dir == "" { os.Unsetenv("GOROOT") } else { diff --git a/src/cmd/go/internal/toolchain/toolchain.go b/src/cmd/go/internal/toolchain/toolchain.go index 3a8d348abb..ab03fbe4ff 100644 --- a/src/cmd/go/internal/toolchain/toolchain.go +++ b/src/cmd/go/internal/toolchain/toolchain.go @@ -16,6 +16,7 @@ import ( "path/filepath" "runtime" "sort" + "strconv" "strings" "cmd/go/internal/base" @@ -40,16 +41,46 @@ const ( gotoolchainModule = "golang.org/toolchain" gotoolchainVersion = "v0.0.1" - // gotoolchainSwitchEnv is a special environment variable - // set to 1 during the toolchain switch by the parent process - // and cleared in the child process. When set, that indicates - // to the child not to do its own toolchain switch logic, - // to avoid an infinite recursion if for some reason a toolchain - // did not believe it could handle its own version and then - // reinvoked itself. - gotoolchainSwitchEnv = "GOTOOLCHAIN_INTERNAL_SWITCH" + // targetEnv is a special environment variable set to the expected + // toolchain version during the toolchain switch by the parent + // process and cleared in the child process. When set, that indicates + // to the child to confirm that it provides the expected toolchain version. + targetEnv = "GOTOOLCHAIN_INTERNAL_SWITCH_VERSION" + + // countEnv is a special environment variable + // that is incremented during each toolchain switch, to detect loops. + // It is cleared before invoking programs in 'go run', 'go test', 'go generate', and 'go tool' + // by invoking them in an environment filtered with FilterEnv, + // so user programs should not see this in their environment. + countEnv = "GOTOOLCHAIN_INTERNAL_SWITCH_COUNT" + + // maxSwitch is the maximum toolchain switching depth. + // Most uses should never see more than three. + // (Perhaps one for the initial GOTOOLCHAIN dispatch, + // a second for go get doing an upgrade, and a third if + // for some reason the chosen upgrade version is too small + // by a little.) + // When the count reaches maxSwitch - 10, we start logging + // the switched versions for debugging before crashing with + // a fatal error upon reaching maxSwitch. + // That should be enough to see the repetition. + maxSwitch = 100 ) +// FilterEnv returns a copy of env with internal GOTOOLCHAIN environment +// variables filtered out. +func FilterEnv(env []string) []string { + // Note: Don't need to filter out targetEnv because Switch does that. + var out []string + for _, e := range env { + if strings.HasPrefix(e, countEnv+"=") { + continue + } + out = append(out, e) + } + return out +} + // Switch invokes a different Go toolchain if directed by // the GOTOOLCHAIN environment variable or the user's configuration // or go.mod file. @@ -58,10 +89,6 @@ func Switch() { log.SetPrefix("go: ") defer log.SetPrefix("") - sw := os.Getenv(gotoolchainSwitchEnv) - os.Unsetenv(gotoolchainSwitchEnv) - // The sw == "1" check is delayed until later so that we still fill in gover.Startup for use in errors. - if !modload.WillBeEnabled() { return } @@ -78,20 +105,19 @@ func Switch() { return } - var minToolchain, minVers string - if x, y, ok := strings.Cut(gotoolchain, "+"); ok { // go1.2.3+auto - orig := gotoolchain - minToolchain, gotoolchain = x, y - minVers = gover.FromToolchain(minToolchain) - if minVers == "" { - base.Fatalf("invalid GOTOOLCHAIN %q: invalid minimum toolchain %q", orig, minToolchain) + minToolchain := gover.LocalToolchain() + minVers := gover.Local() + if min, mode, ok := strings.Cut(gotoolchain, "+"); ok { // go1.2.3+auto + v := gover.FromToolchain(min) + if v == "" { + base.Fatalf("invalid GOTOOLCHAIN %q: invalid minimum toolchain %q", gotoolchain, min) } - if gotoolchain != "auto" && gotoolchain != "path" { - base.Fatalf("invalid GOTOOLCHAIN %q: only version suffixes are +auto and +path", orig) + minToolchain = min + minVers = v + if mode != "auto" && mode != "path" { + base.Fatalf("invalid GOTOOLCHAIN %q: only version suffixes are +auto and +path", gotoolchain) } - } else { - minVers = gover.Local() - minToolchain = "go" + minVers + gotoolchain = mode } if gotoolchain == "auto" || gotoolchain == "path" { @@ -153,7 +179,34 @@ func Switch() { } } - if sw == "1" || gotoolchain == "local" || gotoolchain == "go"+gover.Local() { + // If we are invoked as a target toolchain, confirm that + // we provide the expected version and then run. + // This check is delayed until after the handling of auto and path + // so that we have initialized gover.Startup for use in error messages. + if target := os.Getenv(targetEnv); target != "" && TestVersionSwitch != "loop" { + if gover.LocalToolchain() != target { + base.Fatalf("toolchain %v invoked to provide %v", gover.LocalToolchain(), target) + } + os.Unsetenv(targetEnv) + + // Note: It is tempting to check that if gotoolchain != "local" + // then target == gotoolchain here, as a sanity check that + // the child has made the same version determination as the parent. + // This turns out not always to be the case. Specifically, if we are + // running Go 1.21 with GOTOOLCHAIN=go1.22+auto, which invokes + // Go 1.22, then 'go get go@1.23.0' or 'go get needs_go_1_23' + // will invoke Go 1.23, but as the Go 1.23 child the reason for that + // will not be apparent here: it will look like we should be using Go 1.22. + // We rely on the targetEnv being set to know not to downgrade. + // A longer term problem with the sanity check is that the exact details + // may change over time: there may be other reasons that a future Go + // version might invoke an older one, and the older one won't know why. + // Best to just accept that we were invoked to provide a specific toolchain + // (which we just checked) and leave it at that. + return + } + + if gotoolchain == "local" || gotoolchain == gover.LocalToolchain() { // Let the current binary handle the command. return } @@ -287,6 +340,14 @@ func HasPath() bool { return env == "path" || strings.HasSuffix(env, "+path") } +// TestVersionSwitch is set in the test go binary to the value in $TESTGO_VERSION_SWITCH. +// Valid settings are: +// +// "switch" - simulate version switches by reinvoking the test go binary with a different TESTGO_VERSION. +// "mismatch" - like "switch" but forget to set TESTGO_VERSION, so it looks like we invoked a mismatched toolchain +// "loop" - like "switch" but +var TestVersionSwitch string + // SwitchTo invokes the specified Go toolchain or else prints an error and exits the process. // If $GOTOOLCHAIN is set to path or min+path, SwitchTo only considers the PATH // as a source of Go toolchains. Otherwise SwitchTo tries the PATH but then downloads @@ -294,16 +355,32 @@ func HasPath() bool { func SwitchTo(gotoolchain string) { log.SetPrefix("go: ") + count, _ := strconv.Atoi(os.Getenv(countEnv)) + if count >= maxSwitch-10 { + fmt.Fprintf(os.Stderr, "go: switching from go%v to %v [depth %d]\n", gover.Local(), gotoolchain, count) + } + if count >= maxSwitch { + base.Fatalf("too many toolchain switches") + } + os.Setenv(countEnv, fmt.Sprint(count+1)) + env := cfg.Getenv("GOTOOLCHAIN") pathOnly := env == "path" || strings.HasSuffix(env, "+path") // For testing, if TESTGO_VERSION is already in use // (only happens in the cmd/go test binary) - // and TESTGO_VERSION_SWITCH=1 is set, + // and TESTGO_VERSION_SWITCH=switch is set, // "switch" toolchains by changing TESTGO_VERSION // and reinvoking the current binary. - if gover.TestVersion != "" && os.Getenv("TESTGO_VERSION_SWITCH") == "1" { + // The special cases =loop and =mismatch skip the + // setting of TESTGO_VERSION so that it looks like we + // accidentally invoked the wrong toolchain, + // to test detection of that failure mode. + switch TestVersionSwitch { + case "switch": os.Setenv("TESTGO_VERSION", gotoolchain) + fallthrough + case "loop", "mismatch": exe, err := os.Executable() if err != nil { base.Fatalf("%v", err) @@ -422,7 +499,7 @@ func modGoToolchain() (file, goVers, toolchain string) { // goInstallVersion looks at the command line to see if it is go install m@v or go run m@v. // If so, it returns the m@v and the go version from that module's go.mod. -func goInstallVersion() (m module.Version, goVers string, ok bool) { +func goInstallVersion() (m module.Version, goVers string, found bool) { // Note: We assume there are no flags between 'go' and 'install' or 'run'. // During testing there are some debugging flags that are accepted // in that position, but in production go binaries there are not. @@ -503,10 +580,15 @@ func goInstallVersion() (m module.Version, goVers string, ok bool) { } noneSelected := func(path string) (version string) { return "none" } _, err := modload.QueryPackages(ctx, m.Path, m.Version, noneSelected, allowed) - tooNew, ok := err.(*gover.TooNewError) - if !ok { - return module.Version{}, "", false + if tooNew, ok := err.(*gover.TooNewError); ok { + m.Path, m.Version, _ = strings.Cut(tooNew.What, "@") + return m, tooNew.GoVersion, true } - m.Path, m.Version, _ = strings.Cut(tooNew.What, "@") - return m, tooNew.GoVersion, true + + // QueryPackages succeeded, or it failed for a reason other than + // this Go toolchain being too old for the modules encountered. + // Either way, we identified the m@v on the command line, + // so return found == true so the caller does not fall back to + // consulting go.mod. + return m, "", true } diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go index 3303b7c211..998d0007d0 100644 --- a/src/cmd/go/internal/work/exec.go +++ b/src/cmd/go/internal/work/exec.go @@ -35,6 +35,7 @@ import ( "cmd/go/internal/cache" "cmd/go/internal/cfg" "cmd/go/internal/fsys" + "cmd/go/internal/gover" "cmd/go/internal/load" "cmd/go/internal/modload" "cmd/go/internal/slices" @@ -431,17 +432,7 @@ func allowedVersion(v string) bool { if v == "" { return true } - // Special case "1.0" means "go1", which is OK. - if v == "1.0" { - return true - } - // Otherwise look through release tags of form "go1.23" for one that matches. - for _, tag := range cfg.BuildContext.ReleaseTags { - if strings.HasPrefix(tag, "go") && tag[2:] == v { - return true - } - } - return false + return gover.Compare(gover.Local(), v) >= 0 } const ( diff --git a/src/cmd/go/internal/work/gc.go b/src/cmd/go/internal/work/gc.go index 480338bfb2..6043ad5353 100644 --- a/src/cmd/go/internal/work/gc.go +++ b/src/cmd/go/internal/work/gc.go @@ -19,6 +19,7 @@ import ( "cmd/go/internal/base" "cmd/go/internal/cfg" "cmd/go/internal/fsys" + "cmd/go/internal/gover" "cmd/go/internal/load" "cmd/go/internal/str" "cmd/internal/objabi" @@ -99,7 +100,7 @@ func (gcToolchain) gc(b *Builder, a *Action, archive string, importcfg, embedcfg v = "1.16" } if allowedVersion(v) { - defaultGcFlags = append(defaultGcFlags, "-lang=go"+v) + defaultGcFlags = append(defaultGcFlags, "-lang=go"+gover.Lang(v)) } } if p.Standard { diff --git a/src/cmd/go/main.go b/src/cmd/go/main.go index c4a75f87e3..03ac15a37d 100644 --- a/src/cmd/go/main.go +++ b/src/cmd/go/main.go @@ -209,7 +209,7 @@ func invoke(cmd *base.Command, args []string) { // the same default computation of these as we do, // but in practice there might be skew // This makes sure we all agree. - cfg.OrigEnv = os.Environ() + cfg.OrigEnv = toolchain.FilterEnv(os.Environ()) cfg.CmdEnv = envcmd.MkEnv() for _, env := range cfg.CmdEnv { if os.Getenv(env.Name) != env.Value { diff --git a/src/cmd/go/testdata/script/gotoolchain_local.txt b/src/cmd/go/testdata/script/gotoolchain_local.txt index a7e2b36cc2..18b4faabde 100644 --- a/src/cmd/go/testdata/script/gotoolchain_local.txt +++ b/src/cmd/go/testdata/script/gotoolchain_local.txt @@ -3,7 +3,7 @@ # See gotoolchain_net.txt and gotoolchain_path.txt for tests of network and PATH toolchains. env TESTGO_VERSION=go1.500 -env TESTGO_VERSION_SWITCH=1 +env TESTGO_VERSION_SWITCH=switch # Default setting should be auto env GOTOOLCHAIN= diff --git a/src/cmd/go/testdata/script/gotoolchain_loop.txt b/src/cmd/go/testdata/script/gotoolchain_loop.txt new file mode 100644 index 0000000000..a803d2eb9a --- /dev/null +++ b/src/cmd/go/testdata/script/gotoolchain_loop.txt @@ -0,0 +1,65 @@ +env GOTOOLCHAIN=auto +env TESTGO_VERSION=go1.21.1 + +# Basic switch should work. +env TESTGO_VERSION_SWITCH=switch +go version +stdout go1.21.99 + +# Toolchain target mismatch should be detected. +env TESTGO_VERSION_SWITCH=mismatch +! go version +stderr '^go: toolchain go1.21.1 invoked to provide go1.21.99$' + +# Toolchain loop should be detected. +env TESTGO_VERSION_SWITCH=loop +! go version +stderr -count=10 '^go: switching from go1.21.1 to go1.21.99 \[depth 9[0-9]\]$' +stderr -count=1 '^go: switching from go1.21.1 to go1.21.99 \[depth 100\]$' +stderr '^go: too many toolchain switches$' + +[short] skip + +# Internal env vars should not leak to go test or go run. +env TESTGO_VERSION_SWITCH=switch +go version +stdout go1.21.99 +go test +stdout clean +go run . +stdout clean + +-- go.mod -- +module m +go 1.21.99 + +-- m_test.go -- +package main + +import "testing" + +func TestEnv(t *testing.T) { + // the check is in func init in m.go +} + +-- m.go -- +package main + +import "os" + +func init() { + envs := []string{ + "GOTOOLCHAIN_INTERNAL_SWITCH_COUNT", + "GOTOOLCHAIN_INTERNAL_SWITCH_VERSION", + } + for _, e := range envs { + if v := os.Getenv(e); v != "" { + panic("$"+e+"="+v) + } + } + os.Stdout.WriteString("clean\n") +} + +func main() { +} + diff --git a/src/cmd/go/testdata/script/mod_get_exec_toolchain.txt b/src/cmd/go/testdata/script/mod_get_exec_toolchain.txt index a9aa27ec0a..ac8e2cc698 100644 --- a/src/cmd/go/testdata/script/mod_get_exec_toolchain.txt +++ b/src/cmd/go/testdata/script/mod_get_exec_toolchain.txt @@ -1,5 +1,5 @@ env TESTGO_VERSION=go1.21 -env TESTGO_VERSION_SWITCH=1 +env TESTGO_VERSION_SWITCH=switch # GOTOOLCHAIN=auto should run the newer toolchain env GOTOOLCHAIN=auto diff --git a/src/cmd/go/testdata/script/mod_toolchain.txt b/src/cmd/go/testdata/script/mod_toolchain.txt index d0f8b913e7..f92d982232 100644 --- a/src/cmd/go/testdata/script/mod_toolchain.txt +++ b/src/cmd/go/testdata/script/mod_toolchain.txt @@ -1,5 +1,5 @@ env TESTGO_VERSION=go1.100 -env TESTGO_VERSION_SWITCH=1 +env TESTGO_VERSION_SWITCH=switch go get toolchain@go1.22.1 stderr '^go: added toolchain go1.22.1$' -- 2.48.1