x/tools/gopls: decide on a long-term testing strategy for integration with older Go commands (and GOPACKAGESDRIVERs?) #69321
Comments
gopherbot
added
the
Tools
|
Thanks for filing an issue. Since this is about deciding a long-term strategy, I think it's important to consider #56986 here. That work was implemented fairly recently in Go 1.21 and its effect may strengthen as more time passes. Hopefully it means with future Go releases, there will be fewer and fewer reasons for users to stay an older, unsupported Go toolchains, when a newer one will receive security and bug fixes and yet is still capable of working both with newer as well as older Go programs. Additionally, #57001 further reduces the friction in upgrading to a newer toolchain compared to previously. I think it's worth considering option (3) by weighing the costs of additional overhead with the benefits. I'll take some time to understand the proposed option (4) first. |
|
I think only (1) and (2) are consistent with our support policy. Either is fine with me, although I have a preference for (2) since if users want to use unsupported toolchains, I don't think we should actively stop them. Gopls's decision to promise more than the standard support policy as far as which versions of Go it could be built with has historically been a source of drag on the whole Go team (both the gopls maintainers and anyone working on packages or infrastructure that gopls needed). I personally am very excited about gopls becoming compatible with the standard support policy and removing that drag. I don't think we should make unnecessary work for ourselves by deciding to preserve some of the drag after all. It is perfectly fine for us to say that if you are using an old, unsupported Go toolchain, then perhaps you will also need to use an old, unsuported copy of gopls that matches. There is no need to keep the latest gopls working with unsupported Go toolchains. Re "we can do better", it's true that in some ideal world where we had infinite time to maintain Go and attend to every user's every bug and feature request and confusion, we might use some of that infinite time to keep the latest gopls working with very old Go versions. But we don't live in that world, and there are higher impact things we can do with our time. The support policy should be viewed as an explicit prioritization decision that we made both so that users understand what we prioritize and so that we are held to our priorities as well. We have tied ourselves to the mast; we must resist the siren song of keeping unsupported Go versions working. |
|
SGTM. Let's do this after gopls@v0.17.0, please, because we're already going from 4->1 version build compatibility in v0.17.0. Thanks to toolchain upgrades that should be OK for most users, but I anticipate (based on our telemetry) that there will at least be some users that need help with their gopls setup. I'd prefer to not also have to worry about go list compatibility for this release. We can update our support policy documentation for v0.17.0, even though we'd continue to run CI temporarily. With respect to (1) and (2), I lean toward a balanced approach, to let users do what they want at their own risk, but make it clearer to users that they should update Go:
|
|
From discussion with @hyangah: we think that the popup warning (an LSP showMessage) is liable to be confusing for various users, since it will now depend on the toolchain version of their workspace. We think a better strategy is to put a diagnostic on the |
|
(We can also have a quickfix that adds or updates a |
|
Change https://go.dev/cl/623175 mentions this issue: |
As described in golang/go#69630, our 1.21 and 1.22 builders were not actually testing gopls' integration with the Go command, because go test modifies PATH and GOROOT when performing a toolchain switch. "Fix" this by searching PATH for a local (=non-toolchain) go command, and then mutating PATH and unsetting GOROOT to use this go command. This is very much a hack, as noted in the relevant commentary, but allows us to have much needed test coverage on the builders. In golang/go#69321, we hope to design a better solution to this problem. Many tests are updated to make their go version requirements accurate. Fixes golang/go#69630 Change-Id: I431107b97845e1e99799c2c22f33b04f85ce6dd9 Reviewed-on: https://go-review.googlesource.com/c/tools/+/623175 Reviewed-by: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Robert Findley <rfindley@google.com>
As described in #65917, it has been a multi-year journey to finally be able to ensure that gopls is only (and need only be) built with the latest version of Go.
With that achieved, I think the following experience will still be common for our users:
go install golang.org/x/tools/gopls@latestwith go 1.21.5.This means that while gopls is build with 1.23.1, it will still frequently need1 to integrate with
go listfor older go versions such as 1.21.5. For example, https://telemetry.go.dev still shows 62% of gopls users on 1.22 or older, and 16% on 1.21 or older.Right now, we continue to have test coverage for this integration, by virtue of our 1.21 and 1.22 TryBots, because they set
GOTOOLCHAIN=auto, and because the gopls integration tests run on temporary directories outside of gopls' own 1.23.1 module. Therefore, the tests will be built with 1.23.1, but still integrate with the ambient go version.However, in https://go.dev/cl/605000 the question came up about whether we should continue to maintain "legacy" go version builders for gopls -- at this point Go 1.21 is no longer a supported Go version, and so gopls' support of integrating with the last three Go versions (recently narrowed from 4) is inconsistent with that policy.
IMO, testing that gopls integrates with Go 1.21 is not quite the same as supporting 1.21. After all, we have at times considered running tests against other go/packages drivers, such as for Bazel, and doing so would of course not be the same as supporting Bazel itself. Put differently: at this point Go 1.21 is not going to change, so if an integration test starts failing it will be due to a change in gopls. We may want to avoid making a change in gopls that breaks users integrating with Go 1.21.
Therefore, we have at least a few options:
Regarding (1), I think this seems too severe. (2) would be very similar to our approach with other drivers such as Bazel, yet we have inadvertently caused problems for Bazel users in the past, and I think we can do better. (3) seems like a non-starter, since it burdens the rest of the team.
Here is a tentative proposal for "something else" (4):
Using our existing mechanisms for running tests in various environments, we can "opt in" (or opt out) certain tests to run with older Go commands, on the longtest builder. In legacy mode, these tests would configure the Go command to run with GOTOOLCHAIN=.
Then we will opt in integration tests that exercise "basic" functionality of gopls: loading a workspace, processing metadata-affecting changes, and basic features like diagnostics, navigation, and completion. Since longtest builders do not run as a presubmit, failures of these tests with older Go versions will not block changes to gopls. If tests start failing, we can decide whether to revert the gopls change, or opt-out the test.
In this way, we can at least detect when gopls may break users with older Go versions, and be deliberate about when this is acceptable.
CC @golang/tools-team @golang/release
1 while we could perhaps avoid this by setting GOTOOLCHAIN=1.23.1 before invoking
go listfrom gopls, we have a general philosophical stance that it is least surprising if gopls's view of the workspace is consistent with the user's experience running the go command from a terminal).The text was updated successfully, but these errors were encountered: