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

x/build/cmd/golangbuild: disable GOTOOLCHAIN=local for the x/tools/gopls module #67749

Closed
findleyr opened this issue May 31, 2024 · 5 comments
Closed
Assignees
Labels
Builders x/build issues (builders, bots, dashboards) okay-after-rc1 Used by release team to mark a release-blocker issue as okay to resolve either before or after rc1
Milestone

Comments

@findleyr
Copy link
Contributor

findleyr commented May 31, 2024

As soon as 1.23-rc1 is released, we want to update the toolchain requirement for x/tools/gopls to 1.23-rc1, in accordance with the plan in #65917. This means gopls will require at least a 1.23 toolchain to build.

However, we still want to support gopls integrating with the go command of older go versions. This would happen naturally on the release branch trybots, except that due to GOTOOLCHAIN=local they cannot build gopls. https://go.dev/cl/588766 contains a trial CL demonstrating this failure mode.

I propose that we remove the GOTOOLCHAIN=local setting for gopls tests. Given that the gopls build must already download external dependencies, I don't see why GOTOOLCHAIN=local is necessary.

A side effect may be that some gopls tests download additional toolchains. That may or may not be desirable, depending on whether the test itself is intended to exercise toolchain upgrades. Currently, we have some tests that we'd like to write but can't, because of the GOTOOLCHAIN=local restriction. For tests where the toolchain upgrade is unintended, we can guard the test using existing mechanisms to skip on older toolchains.

This issue is somewhat urgent, since we want to make this change in the gopls go.mod file as soon as the Go rc is cut in mid June. Putting this in the 1.23 milestone to reflect the relationship of this issue with the 1.23 release.

CC @adonovan @hyangah @golang/release

@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label May 31, 2024
@gopherbot gopherbot added this to the Unreleased milestone May 31, 2024
@findleyr findleyr modified the milestones: Unreleased, Go1.23 May 31, 2024
@findleyr findleyr added the okay-after-rc1 Used by release team to mark a release-blocker issue as okay to resolve either before or after rc1 label May 31, 2024
@dmitshur
Copy link
Contributor

dmitshur commented May 31, 2024

Given that the gopls build must already download external dependencies, I don't see why GOTOOLCHAIN=local is necessary.

The Go release policy says the last two major Go releases are supported, and accepted proposal #34536 clarifies we support them equally. This means even if someone needs to take 8 months before upgrading to a newer major release after it's out, they can do so while receiving security fixes and other critical bug fixes on a supported older major version. When a newer major Go version bumps minimum OS requirements, the older major Go version can still be used for 6 months before becoming unsupported.

The builders are configured to test both supported release branches, in addition to tip, so that issues that affect only one of the two supported Go versions are caught. Setting GOTOOLCHAIN=local by default in the builders is done since the builders are responsible for selecting the toolchain being tested, and they wouldn't provide signal for said toolchain if it were to be accidentally upgraded to a different version. In other words, it's not done because it is necessary, but in order to increase the signal we get from the go1.22 and go1.21 builders.

I wanted to share that context first. It's certainly possible to make an adjustment for the x/tools/gopls module if there's agreement that it's needed.

Currently, we have some tests that we'd like to write but can't, because of the GOTOOLCHAIN=local restriction.

Individual tests that need finer control can invoke the go tool while overriding its environment (including GOTOOLCHAIN). This is probably not the right or optimal approach in all cases, but it is an option that might work for some.

@findleyr
Copy link
Contributor Author

findleyr commented May 31, 2024

@dmitshur I'm not sure that the Go release policy applies in this way. Gopls is a tool that is used for editing Go code. We have gone to great effort to ensure that a gopls built with the most recent version of Go can be used to edit code targeting older Go versions. This issue is about ensuring that we also integrate with the go command of older go versions. From that perspective, it seems that we are still fully supporting older Go versions.

I suspect that consolidating gopls on the latest version of Go will actually improve its support for older Go versions, since gopls will benefit sooner from parser and type checker improvements.

With respect to updating the go.mod file to -rc1, this is only a temporary measure on the master branch. We will never reference an rc in the go.mod file of a released version of gopls. Updating the go.mod to 1.23-rc1 allows us to start working on simplifying the gopls codebase, removing legacy version support, so that we can release gopls@v0.17.0 soon after Go 1.23 is released.

We regularly update the gopls go.mod file to bump the minimum version requirements of its dependencies to their latest version. Is the go toolchain any different? Serious question.

Here are a couple considerations:

  • An organization may have a policy saying that only Go 1.N.1+ may be used to build tools, perhaps due to concerns about the stability of 1.N.0 releases. But of course, they are installing the latest version of gopls and its dependencies, so I'm not sure this concern is reasonable.
  • [pointed out by @dmitshur] The latest version of Go regularly drops support for older operating systems. Requiring the latest version of Go to build gopls means that users on e.g. Mac OS 10.15 can't build gopls. But of course, they can install an older version of gopls.

This is a tough call. We can of course be more conservative, and only move our go.mod to the oldest supported Go version (rather than the most recent Go version), but that means that our iteration cycle with the standard library is a 9mo-1y, rather than 3-6 months. It also means we have to simultaneously support multiple versions of the parser or type checker, which can have different semantics. This potentially significantly decreases the benefit of bumping our toolchain requirements.

@rsc I think we need your advice here.

@dmitshur
Copy link
Contributor

Thanks. I'm convinced you should be able try this, given you're already taking the support needs into account to a great degree. This is something we can choose to undo if you learn it's not working well or as you intended. Sent you crrev.com/c/5589446.

@findleyr
Copy link
Contributor Author

Thanks Dmitri.

This could be hard to undo if we immediately break compatibility with 1.22 (which we may want to do, because of the gotypesalias change in go/types). We'll discuss this consideration on the team.

In any case, the change to remove GOTOOLCHAIN=local makes sense, because we may even want to keep around the N-2 and N-3 builders for some time, to ensure integration with legacy go commands.

@dmitshur dmitshur changed the title x/build: disable GOTOOLCHAIN=local for the x/tools/gopls repo x/build/cmd/golangbuild: disable GOTOOLCHAIN=local for the x/tools/gopls repo May 31, 2024
@dmitshur dmitshur changed the title x/build/cmd/golangbuild: disable GOTOOLCHAIN=local for the x/tools/gopls repo x/build/cmd/golangbuild: disable GOTOOLCHAIN=local for the x/tools/gopls module May 31, 2024
@findleyr
Copy link
Contributor Author

Looks Dmitri's CL above fixed this! https://go.dev/cl/588766 shows tests passing seamlessly with 1.21.

Closing as completed. Awesome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Builders x/build issues (builders, bots, dashboards) okay-after-rc1 Used by release team to mark a release-blocker issue as okay to resolve either before or after rc1
Projects
None yet
Development

No branches or pull requests

3 participants