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

extension/src/goInstallTools: strengthing minimum go version requirement #3371

Open
wants to merge 1 commit into
base: release
Choose a base branch
from

Conversation

koleskizzzzz
Copy link

This change addresses problems in tools installation surfaced by the recent changes in go and gopls.

  • Automated toolchain switch for Go forwards compatibility

The automated Go toolchain switch change added in go1.21 changed the meaning of 'go version' output different. Previously, it was meant exactly the version of the toolchain on the system. After go1.21, this is a go version for chosen for the workspace, computed based on the local toolchain version and the required go version dictated by go.mod or go.work.

The extension has a facility to detect the go version used to compile tools, and ask users to rebuild them if they were built with older go standard libraries. When a local toolchain is older (for example, go1.21.0) than the workspace's required go version (go1.22.0), the 'go version' reports go1.22.0. The extension will detect tools that need recompiling to support go1.22.0 project. That works as expected.

However, when recompiling those tools with go install, the workspace's go version requirement doesn't come into play at all, but only the local toolchain's go version and the target tool's minimum required go version will be used in the toolchain switch decision making. As of today, none of the tools set their minimum required go version to go1.22.x, so if the local toolchain is still go1.21.0, go install will use go1.21.0 for build.

We need to explicitly specify the minimum required version using the GOTOOLCHAIN environment variable. In this example, go install with GOTOOLCHAIN=go1.22.0+auto will address the issue.

In this CL,

  • extend getGoVersion to allow the local toolchain version by specifying the optional GOTOOLCHAIN parameter.
  • change getGoForInstall to produce the correct GoVersion object that carries the true version of the local toolchain. (computed with getGoVersion(..., 'local')).
  • change installTools to append the GOTOOLCHAIN env var if the go version to be used for install is older than the project's minimum required go version.
  • More frequent crashes/malfunctions upon version mismatch

Gopls is supposed to prompt when it detects it needs to be recompiled to process the modules that require newer go versions. The extension has relied this as the second, and more reliable defense mechanism. Unfortunately, bugs in the recent gopls stopped gopls from reliably detecting this case or, even made the gopls crash before showing the notification.

Similar crashes can occur in other tools (#3168) when the version is mismatched. Previously, installTools warned users only if the go version for install was very old (e.g. go1.15).

In this CL,

  • tighten installTools's installation tool version check further. So, if the project requires a newer version of go and the go configured for tools installation (maybe due to outdated 'go.toolsManagement.go' setting) is older and is a version that cannot handle automated version switch (<go1.21), it prompts users.
  • Other changes

Testing involving two versions of go is complex. Changed the runTest helper for 'Installation Tests' test suite to accept extra parameters and stubs, so we can make the installation function under test believe the local go chosen for installation is an older version.

This CL also adjusted the logging of gopls restart activities. When gopls installed (automatically, or from the activation), the extension may attempt to restart the gopls. That may help investigate spurious gopls restarts obeserved during https://github.com/golang/vscode-go/issues/3307.

This CL stops clearing of the Go outputChannel before tool installation or goplay run. This channel is now a log output channel, so it's better not to clear.

Fixes #3168

Change-Id: Id9a4c0fe98c85efb17eb3351cfba5665a83b094d
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/577095
Reviewed-by: Peter Weinberger pjw@google.com
Commit-Queue: Hyang-Ah Hana Kim hyangah@gmail.com
Reviewed-by: Suzy Mueller suzmue@golang.org
kokoro-CI: kokoro noreply+kokoro@google.com

This PR will be imported into Gerrit with the title and first
comment (this text) used to generate the subject and body of
the Gerrit change.

Please ensure you adhere to every item in this list.

More info can be found at https://github.com/golang/go/wiki/CommitMessage

  • The PR title is formatted as follows: frob the quux before blarfing
    • The part after the colon uses the verb tense + phrase that completes the blank in,
      "This change modifies Go to ___________"
    • Lowercase verb after the colon
    • No trailing period
    • Keep the title as short as possible. ideally under 76 characters or shorter
  • No Markdown
  • The first PR comment (this one) is wrapped at 76 characters, unless it's
    really needed (ASCII art, table, or long link)
  • If there is a corresponding issue, add either Fixes golang/vscode-go#1234 or Updates golang/vscode-go#1234
    (the latter if this is not a complete fix) to this comment
  • If referring to a repo, you can use the owner/repo#issue_number syntax:
    Fixes golang/tools#1234
  • We do not use Signed-off-by lines in Go. Please don't add them.
    Our Gerrit server & GitHub bots enforce CLA compliance instead.
  • Delete these instructions once you have read and applied them

This change addresses problems in tools installation surfaced
by the recent changes in go and gopls.

* Automated toolchain switch for Go forwards compatibility

The automated Go toolchain switch change added in go1.21
changed the meaning of 'go version' output different. Previously,
it was meant exactly the version of the toolchain on the system.
After go1.21, this is a go version for chosen for the workspace,
computed based on the local toolchain version and the required
go version dictated by go.mod or go.work.

The extension has a facility to detect the go version used to
compile tools, and ask users to rebuild them if they were built
with older go standard libraries. When a local toolchain is
older (for example, `go1.21.0`) than the workspace's required go
version (`go1.22.0`), the 'go version' reports `go1.22.0`.
The extension will detect tools that need recompiling to support
`go1.22.0` project. That works as expected.

However, when recompiling those tools with `go install`,
the workspace's go version requirement doesn't come into play
at all, but only the local toolchain's go version and the target
tool's minimum required go version will be used in the toolchain
switch decision making. As of today, none of the tools set their
minimum required go version to go1.22.x, so if the local toolchain
is still go1.21.0, `go install` will use go1.21.0 for build.

We need to explicitly specify the minimum required version using
the `GOTOOLCHAIN` environment variable. In this example, go
install with `GOTOOLCHAIN=go1.22.0+auto` will address the issue.

In this CL,
  - extend `getGoVersion` to allow the local toolchain version
    by specifying the optional GOTOOLCHAIN parameter.
  - change `getGoForInstall` to produce the correct GoVersion
    object that carries the true version of the local toolchain.
    (computed with `getGoVersion(..., 'local')`).
  - change `installTools` to append the `GOTOOLCHAIN` env var
    if the go version to be used for install is older than
    the project's minimum required go version.

* More frequent crashes/malfunctions upon version mismatch

Gopls is supposed to prompt when it detects it needs to be
recompiled to process the modules that require newer go versions.
The extension has relied this as the second, and more reliable
defense mechanism. Unfortunately, bugs in the recent gopls
stopped gopls from reliably detecting this case or, even made
the gopls crash before showing the notification.

Similar crashes can occur in other tools (golang#3168)
when the version is mismatched. Previously, `installTools` warned
users only if the go version for install was very old (e.g. go1.15).

In this CL,
  - tighten `installTools`'s installation tool version check
    further. So, if the project requires a newer version of go
    and the go configured for tools installation (maybe due to
    outdated 'go.toolsManagement.go' setting) is older and
    is a version that cannot handle automated version switch
    (<go1.21), it prompts users.

* Other changes

Testing involving two versions of go is complex. Changed the
runTest helper for 'Installation Tests' test suite to accept
extra parameters and stubs, so we can make the installation
function under test believe the local go chosen for installation
is an older version.

This CL also adjusted the logging of gopls restart activities.
When gopls installed (automatically, or from the activation),
the extension may attempt to restart the gopls. That may help
investigate spurious gopls restarts obeserved during
https://github.com/golang/vscode-go/issues/3307.

This CL stops clearing of the Go outputChannel before tool
installation or goplay run. This channel is now a log output
channel, so it's better not to clear.

Fixes golang#3168

Change-Id: Id9a4c0fe98c85efb17eb3351cfba5665a83b094d
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/577095
Reviewed-by: Peter Weinberger <pjw@google.com>
Commit-Queue: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
kokoro-CI: kokoro <noreply+kokoro@google.com>
Copy link

google-cla bot commented May 1, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants