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

Go: Use toolchain directives for version selection if available, and add tests #16453

Closed
wants to merge 16 commits into from

Conversation

mbg
Copy link
Member

@mbg mbg commented May 8, 2024

Context

In Go 1.21, the Go team started making a distinction between language and toolchain versions. Historically, the Go version is declared with a go directive in a go.mod file, or a go.work file. Since Go 1.21, go directives are used to declare the language version. A new toolchain directive may be used to explicitly declare the toolchain version. For backwards compatibility, if there is no toolchain directive, the language version is used as the toolchain version. In the Go autobuilder, we have numerous places where we try to determine the "version" of Go that is in use, should be installed, etc. Here, we are mainly interested in the toolchain version.

What this PR addresses

So far, we have mainly been looking at go directives in go.mod files, and recently go.work files, for this. However, if a toolchain directive is present in either type of file, then this determines the toolchain version. We have not been considering this and this PR addresses that shortcoming by modifying the autobuilder to check for the presence of toolchain directives when determining the version that is in use.

This PR also adds a number of tests for the functions involved in this process.

How to review

Best reviewed commit-by-commit.

@mbg mbg self-assigned this May 8, 2024
@mbg mbg requested a review from a team as a code owner May 8, 2024 12:16
@github-actions github-actions bot added the Go label May 8, 2024
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broadly fine. I think you should add a few more comments to make it easier to reason about what strings have what prefixes. In the definitions of any structs that have a field containing a go version, add a comment after the field definition giving an example value, so that it's obvious what prefix is expected.

go/extractor/toolchain/toolchain.go Outdated Show resolved Hide resolved
go/extractor/toolchain/toolchain.go Outdated Show resolved Hide resolved
@mbg
Copy link
Member Author

mbg commented May 8, 2024

@owen-mc I addressed your comments, but I also realised that the first batch of changes in this PR meant that GetWorkspaceInfo now returns version strings in the format used by semver (starting with "v"). Rather than trying to identify how that affects downstream code, I tried to make all consumers of the information obtained from GetWorkspaceInfo resilient to both of the different formats (with and without "v"). I also added tests for that.

In retrospect, maybe it would be better to introduce a new type that's a wrapper around string for all the versions that are in the semver format.

// A value indicating whether a version string was found
// A value indicating whether a version string was found.
// If this value is `true`, then `Version` is a valid semantic version.
// IF this value is `false`, then `Version` is the empty string.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// IF this value is `false`, then `Version` is the empty string.
// If this value is `false`, then `Version` is the empty string.

@mbg
Copy link
Member Author

mbg commented Jun 7, 2024

Replaced by #16703

@mbg mbg closed this Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants