Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dl/internal/version: consider to unset GOTOOLCHAIN #64665

Open
hyangah opened this issue Dec 12, 2023 · 10 comments
Open

dl/internal/version: consider to unset GOTOOLCHAIN #64665

hyangah opened this issue Dec 12, 2023 · 10 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@hyangah
Copy link
Contributor

hyangah commented Dec 12, 2023

I've updated my default go using go env -w GOTOOLCHAIN=go1.21.5+auto.

$ go env GOTOOLCHAIN
go1.21.5+auto

After that, the instruction in managing go installation does not work any more.

$ go install golang.org/dl/gotip@latest
$ gotip download
...
$ gotip version
go version go1.21.5 darwin/amd64

Same for other version wrappers.

The easiest fix may be to set GOTOOLCHAIN=local or GOTOOLCHAIN= when these wrapper binaries invoke go.

Alternatively, we can deprecate the current "managing go installation" doc, and recommend to use GOTOOLCHAIN to switch the version instead.

@dmitshur dmitshur changed the title golang.org/dl/internal/version: consider to unset GOTOOLCHAIN dl/internal/version: consider to unset GOTOOLCHAIN Dec 12, 2023
@dmitshur
Copy link
Contributor

dmitshur commented Dec 12, 2023

That's an interesting way for these behaviors to intersect. But also it seems unexpected to me that gotip, whose version is something like go1.22-b18b058816 (i.e., language version 1.22) ends up selecting go1.21.5 (i.e., language version 1.21) given that https://go.dev/doc/toolchain#version says:

Released Go toolchains such as Go 1.21.0 and Go 1.21rc1 report that specific version (for example, go1.21.0 or go1.21rc1) from go version and runtime.Version. Unreleased (still in development) Go toolchains built from the Go development repository instead report only the language version (for example, go1.21).

Any two Go versions can be compared to decide whether one is less than, greater than, or equal to the other. If the language versions are different, that decides the comparison: 1.21.9 < 1.22.

And https://go.dev/doc/toolchain#select says:

When GOTOOLCHAIN is set to <name>+auto or <name>+path (or the shorthands auto or path), the go command selects and runs a newer Go version as needed.

Maybe I'm overlooking something, but it seems like unexpected cmd/go behavior (with some similarity to #63357). CC @bcmills.

Edit: Thanks to Bryan's reply below, I realize now that I missed the paragraph above:

When GOTOOLCHAIN is set to <name> (for example, GOTOOLCHAIN=go1.21.0), the go command always runs that specific Go toolchain.

And that the same baseline behavior of starting with <name> applies in the <name>+auto case too, even though it's not emphasized in its paragraph as much.


to set GOTOOLCHAIN=local

Note that doing this would get in the way of another aspect of how these goX.Y.Z commands work. This comment has more context.

@bcmills
Copy link
Contributor

bcmills commented Dec 12, 2023

It is expected that setting an explicit variable in the GOTOOLCHAIN variable results in a downgrade: the version set in the GOTOOLCHAIN variable, if any, indicates the minimum acceptable toolchain version.

(This is in contrast with, say, the toolchain or go line from the main module's go.mod file, which should only ever raise the selected version of the toolchain.)

It isn't obvious to me what the dl wrappers ought to do if GOTOOLCHAIN lists a specific version: in that case, we have two conflicting sources of user information (GOTOOLCHAIN and the name of the command itself), and I don't think I can confidently infer what the user intends to happen. (Clearly in this case @hyangah intends for the name of the command to win, but is that true for everyone in general? 🤔)

@prattmic prattmic added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Dec 12, 2023
@prattmic prattmic added this to the Unreleased milestone Dec 12, 2023
@prattmic
Copy link
Member

we have two conflicting sources of user information, and I don't think I can confidently infer what the user intends to happen.

A (lazy?) approach would be to error in this case and thus require the user to be more specific.

@hyangah
Copy link
Contributor Author

hyangah commented Dec 12, 2023

The interaction with gotip & GOTOOLCHAIN=go1.21.5+auto is quite surprising. It shoud've picked up go1.22 dev version.

For go1.X.Y wrappers, the issue here is user's GOTOOLCHAIN setting coming from go env -w, which is IMO often a footgun :-(.

Clearly in this case @hyangah intends for the name of the command to win, but is that true for everyone in general? 🤔)

Other than go command line tool developers, how often a go user would be interested in testing the toolchain switch behavior (how go1.X.Y interacts with GOTOOLCHAIN environment variables)? It's more likely that they reach out to this wrapper because they expect behavior similar to the old go version and they need to test their program compiled against a specific version of go (e.g. why is my program slow or test broken after I updated my docker container's go version??)

@hyangah
Copy link
Contributor Author

hyangah commented Feb 16, 2024

Today I faced the same issue, and GOTOOLCHAIN's +auto behavior is very confusing.

$ GOTOOLCHAIN=go1.22.0+auto gotip version
go version go1.22.0 darwin/amd64

$ GOTOOLCHAIN=local gotip version
go version devel go1.23-daa58db486 Fri Feb 16 11:59:07 2024 +0000 darwin/amd64

@prattmic Can you tell me more about the approach you are suggesting? Most part of go commands is designed to work without human interaction and this wrapper is designed to be interchangeable with 'go', it's unclear to me how this should be surfaced.

Is there a way to tell whether GOTOOLCHAIN value was set from go env -w or coming form the environment variable? The intention when users run the wrapper with environment variable may be unclear, but go env -w + wrapper is most likely oversight.

@seankhliao
Copy link
Member

see #34208 for identifying the source of settings

@prattmic
Copy link
Member

@hyangah My suggestion was to error on conflict. Something like:

$ GOTOOLCHAIN=go1.21.5 gotip version
error: dl version `tip` and GOTOOLCHAIN version `1.21.5` conflict
<exit code 1>

I don't know if this is a good idea, it just came to mind from Bryan's comment "I don't think I can confidently infer what the user intends to happen". Thus, we refuse to infer at all.

@bcmills
Copy link
Contributor

bcmills commented Feb 23, 2024

I think the vast majority of users will either leave GOTOOLCHAIN unset or use GOTOOLCHAIN=auto or GOTOOLCHAIN=local, and as far as I can tell the behavior for those two cases isn't surprising.

And GOTOOLCHAIN=go1.21.5 go version seems unambiguous, too: GOTOOLCHAIN=go1.21.5 means “use exactly go1.21.5”, so go version go1.21.5 is the only version that can reasonably report. I guess it follows from that that GOTOOLCHAIN=go1.22.0 gotip version ought to exec exactly go1.22.0 and report its version, because gotip is supposed to behave like go.

The confusing combination, I think, is GOTOOLCHAIN=go1.22.0+auto, where the exact version given is older than the toolchain being invoked. But (a) hardly anybody ought to be using a command like that, and (b) it does seem understandable if you reverse-engineer from what actually happens.

That said, maybe we should log something to stderr when we intentionally switch to an older toolchain? 🤔

@bcmills
Copy link
Contributor

bcmills commented Feb 28, 2024

Alternatively, we can deprecate the current "managing go installation" doc, and recommend to use GOTOOLCHAIN to switch the version instead.

FWIW, I think the instructions in that doc are still the best way to manage the minimum Go installation, and I don't think we should suggest setting something like GOTOOLCHAIN=go1.21.5+auto for that — we don't want users to become stuck on an old toolchain when their installed toolchain is newer.

But we also don't currently have a GOTOOLCHAIN setting that means “be sure to upgrade to at least go1.21.5, but newer is ok too.” Perhaps we should. 🤔

@rsc
Copy link
Contributor

rsc commented Feb 28, 2024

I think there's a good argument for gotip to force GOTOOLCHAIN=local as it runs the go binary it picks.

The versioned go commands cannot do this, because we expect a sequence of upgrades as execs happen in the most complex auto-upgrade cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

6 participants