search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.01k stars 17.54k forks source link

x/tools/gopls: decide on a long-term testing strategy for integration with older Go commands (and GOPACKAGESDRIVERs?) #69321

Open findleyr opened 2 weeks ago

findleyr commented 2 weeks ago

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:

This means that while gopls is build with 1.23.1, it will still frequently need1 to integrate with go list for 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:

  1. Stop supporting integration with older Go versions, and take a hard stance that users should upgrade to a supported version of Go, perhaps by popping up a message warning users of the lack of support.
  2. Stop supporting integration with older Go versions, but remain silent when users try to use gopls in this way.
  3. Ask the release team to continue maintaining support for legacy TryBots.
  4. Something else (see below...)

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 list from 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).

gabyhelp commented 2 weeks ago

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

dmitshur commented 2 weeks ago

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.

rsc commented 1 week ago

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.

findleyr commented 1 week ago

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: