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

cmd/go: cannot recover from go env -w GOTOOLCHAIN=* #59870

Closed
hyangah opened this issue Apr 27, 2023 · 10 comments
Closed

cmd/go: cannot recover from go env -w GOTOOLCHAIN=* #59870

hyangah opened this issue Apr 27, 2023 · 10 comments
Labels
GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@hyangah
Copy link
Contributor

hyangah commented Apr 27, 2023

If previously set an older version as GOTOOLCHAIN

~$ gotip env -w GOTOOLCHAIN=go1.20.2
~$ gotip version
go version go1.20.2 darwin/amd64
~$ gotip env -w GOTOOLCHAIN=
go: unknown go command variable GOTOOLCHAIN

or if entered a wrong version by mistake

~$ gotip env -w GOTOOLCHAIN=1.20.2
~$ gotip version
go: invalid GOTOOLCHAIN "1.20.2"
~$ gotip env -w GOTOOLCHAIN=local
go: invalid GOTOOLCHAIN "1.20.2"

Some ideas:

  • handle go env command before switch toolchain
  • or disallow versions older than go1.21 for GOTOOLCHAIN value (I hope not...)

And

  • validate the version string before accepting go env -w

cc @rsc @bcmills

EDIT: I found the following helps.

$ GOTOOLCHAIN=local gotip env -w GOTOOLCHAIN=local

I still cannot set to an empty value

$ GOTOOLCHAIN=local gotip env -w GOTOOLCHAIN=
warning: go env -w GOTOOLCHAIN=... does not override conflicting OS environment variable
@bcmills
Copy link
Contributor

bcmills commented Apr 27, 2023

Probably go env -w should apply any GOTOOLCHAIN changes to itself before exec'ing the GOTOOLCHAIN-based switch.

@bcmills bcmills added this to the Go1.21 milestone Apr 27, 2023
@bcmills bcmills added GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Apr 27, 2023
@bcmills
Copy link
Contributor

bcmills commented Apr 27, 2023

Marking as release-blocker for 1.21 because GOTOOLCHAIN is new in 1.21.

@bcmills
Copy link
Contributor

bcmills commented Apr 27, 2023

Probably go env -w should apply any GOTOOLCHAIN changes to itself before exec'ing the GOTOOLCHAIN-based switch.

Actually, I think that implies that you can't use go env -w to set a toolchain older than 1.21, since writing GOTOOLCHAIN into go.env guarantees that the file isn't compatible with a pre-1.21 toolchain. 🤔

@rittneje
Copy link

@bcmills Won't the same sort of problem unfold any time a new variable gets added to go.env? For example, suppose Go 1.22 adds support for GOFOO.

$ go env -w GOFOO=bar
$ go env -w GOTOOLCHAIN=go1.21.0

Now everything will fail because 1.21 doesn't understand GOFOO, and I cannot fix it without manually editing the go.env file.

Even if go env -w prevented me from setting GOTOOLCHAIN in this case, it seems like this could lead to frustration going forward and ultimately undermine the utility of both GOTOOLCHAIN and go.env. Is there a reason the toolchain needs to fail on unrecognized entries instead of just ignoring them?

@bcmills
Copy link
Contributor

bcmills commented May 2, 2023

Is there a reason the toolchain needs to fail on unrecognized entries instead of just ignoring them?

go env -w certainly ought to do that to prevent typos, and I think the original reason for applying the same logic to the file was to detect erroneous hand-edits. But perhaps we should ignore unrecognized settings once they're already in the file.

But I don't see that that would help much for this particular case, because the same error will occur if you set GOTOOLCHAIN=1.18.10 and the 1.18 release is no longer receiving backports.

@rsc
Copy link
Contributor

rsc commented May 2, 2023

Re "I still cannot set to an empty value", the command does set it to an empty value. It just prints a warning that your environment is still going to override the value if you keep your environment that way.

@rsc
Copy link
Contributor

rsc commented May 2, 2023

Thinking out loud ...

There are a few cases here.

If you set GOTOOLCHAIN=garbage, then that will fail consistently, and you have to use GOTOOLCHAIN=local go env -w GOTOOLCHAIN=... to get back out.

If you set GOTOOLCHAIN=go1.19, that works fine for ordinary go commands, but you still have to use GOTOOLCHAIN=local go env -w GOTOOLCHAIN=... to get back out. That will stop being necessary once all the toolchains you care about know about go env -w GOTOOLCHAIN (when you stop caring about Go before Go 1.21).

If you have Go 1.21 and you set go env -w GOTOOLCHAIN=go1.22, then the next go env -w VAR_INTRODUCED_IN_GO1.22=... needs to use Go 1.22; we can't just make go env -w be forced to use the local toolchain.

Note that the go command already does ignore unrecognized settings in the file.

It seems like encouraging GOTOOLCHAIN=local as the workaround is the right answer today.

It's almost always a mistake to set go env -w GOTOOLCHAIN=go1.19 anyway (as opposed to setting a normal environment variable that way), so I'm not sure we have to worry too much about this case.

On the other hand, suppose for the sake of argument that we special case go env -w GOTOOLCHAIN=... to not invoke the known toolchain and just handle that one case itself. What could go wrong if we didn't invoke a newer toolchain for that? I suppose a newer toolchain might do some extra sanity checking of the GOTOOLCHAIN value, so we'd be cutting ourselves off from that. Maybe that's not enough of a compelling reason? Maybe we should add the special case?

@hyangah
Copy link
Contributor Author

hyangah commented May 2, 2023

Another option to consider is to allow only a couple of recognized values (e.g. local, auto??) for go env -w GOTOOLCHAIN=.... Even for go1.22, I cannot think of a valid case where setting this is desirable. Accidentally setting go env -w GOTOOLCHAIN=go1.20 will result in an unpleasant surprise when the user upgrades the local go version.

The only reason I encountered this issue was because I was messing with it to prototype an experimental idea. I ended up creating a special environment variable for the purpose.

@cherrymui
Copy link
Member

@rsc any updates on this? This is currently marked as a release blocker and the freeze is coming soon. Thanks.

@gopherbot
Copy link

Change https://go.dev/cl/496957 mentions this issue: cmd/go: allow using go env with unknown vars already in go/env file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Development

No branches or pull requests

6 participants