-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Go: Deal with incorrect toolchain versions #15979
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
Conversation
6a14e3f
to
c74d634
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to test an actual download, consider setting GOPATH
to an empty directory so it certainly won't find an existing toolchain downloaded by a prior integration test or similar.
workingDir, | ||
) | ||
|
||
// Run the command. If something goes wrong, report it to the log and signal failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider marking this as "installed" regardless to avoid repeatedly attempting to install the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this. It seems like a rare scenario where we would repeatedly attempt to install the same version and that fails for whatever reason.
If I had implemented this set specifically for the new check, then okay, but since the implementation is more general, it might lead to weird results down the line if we want to utilise the set of installed versions for something else and find that in some cases those aren't actually installed.
@smowton We already set a custom |
seems to work locally for me ( |
...ntegration-tests/all-platforms/go/diagnostics/invalid-toolchain-version/diagnostics.expected
Outdated
Show resolved
Hide resolved
@smowton weirdness seems to have been due to the awful I have fixed those bugs and added an integration test. |
Co-authored-by: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com>
Co-authored-by: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com>
This diagnostic was introduced by #15979. However in the meantime the Go team [has backtracked](golang/go#62278 (comment)) on their decision, which leads to confusing alerts for user (e.g. github/codeql-action#2868). Even using Go toolchains from 1.21 to 1.22 we weren't immediately able to reproduce the problem that this diagnostics was meant to guard against. Therefore it was deemed simpler to just remove it. _En passant_ the `Makefile` now accepts `rtjo` not being set.
Starting in Go 1.21, Go uses a new version format for toolchains which uses the
1.N.P
syntax. This is a problem for our users when theirgo.mod
files only contain ago
directive that uses the old version format, e.g.go 1.22
.1.22
is a valid language version, but not a valid toolchain version. If there is no additionaltoolchain
directive in thego.mod
file, thengo
uses the language version as the toolchain version. In the example,1.22
is not a valid toolchain version and certaingo
commands, particularly those which try to acquire the toolchain corresponding to that version, fail with an unhelpful error.In this PR, we make two changes to the Go autobuilder:
go.mod
files where the language version is1.21
or greater, doesn't use the new toolchain version syntax, and there is no explicittoolchain
directive. We emit a warning-level diagnostic in this case.go version
withGOTOOLCHAIN=go1.N.P
(i.e. the correct representation of the toolchain version) to getgo
to install that version and avoid subsequent errors despite the incorrect version format. As an optimisation for repos with a lot ofgo.mod
files, we now keep track of toolchain versions that we know are installed and only do this if we don't yet know that the corresponding version is installed.