Skip to content

Go: Make version parsing robust in the face of custom Go builds #21817

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.

Sign up for GitHub

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

Open
jketema wants to merge 1 commit into
base: main
Choose a base branch
from
+10 −2
Open

Go: Make version parsing robust in the face of custom Go builds #21817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion go/extractor/toolchain/toolchain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,13 @@ func parseGoVersion(data string) string {
for sc.Scan() {
lastLine = sc.Text()
}
return strings.Fields(lastLine)[2]

var goVersion = strings.Fields(lastLine)[2]

// Drop custom build suffixes.
goVersion, _, _ = strings.Cut(goVersion, "-")
Comment on lines +134 to +137
Copy link
Copy Markdown
Member

@mbg mbg May 8, 2026

Choose a reason for hiding this comment

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

I am not sure about this approach. It probably solves the immediate issue, but why not use the existing machinery we have in semver.go? I.e. wrap this in SemVer using NewSemVer. That will parse the semver (and should account for some Go special cases). You can then extract only the components you care about (to ignore the prerelease identifier and e.g. the build info if we don't care about those).

That said, we might want to think about the semver ordering rules and how we should treat prerelease identifiers or build info, rather than just discarding it.

Copy link
Copy Markdown
Contributor Author

@jketema jketema May 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure how you can make your first proposal work without creating code that flat out conflicts with the current -rc handling. Specifically

codeql/go/extractor/util/semver.go

Line 84 in 8cc6d78

numeric = version[:rcIndex-1]

Copy link
Copy Markdown
Member

@mbg mbg May 8, 2026

Choose a reason for hiding this comment

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

That part of the implementation is hacky for sure and probably breaks some theoretical inputs in any case, but my reading of it is that:

  • If we have a version string that doesn't include rc anywhere, then it is treated as a semver in the normal way. I.e. the pre-release identifier is kept as is. This should cover most of the potential cases we discussed in the thread.
  • Only if the version string contains rc somewhere, then this logic kicks in to try and convert it to -rc[..].

If we think that the potential breakage in the rc case is a problem, then we should probably address that in the NewSemVer implementation since the same version strings are likely to end up in there at some point as well.

Copy link
Copy Markdown
Contributor Author

@jketema jketema May 8, 2026

Choose a reason for hiding this comment

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

Thanks. It's not clear what we gain by all this though, without yet another major rewrite. Assuming Copilot above is correct, then the case we will be missing here is go version devel go1.22-abc123def456 linux/amd64, but we don't handle that anyway, as we have hardcoded strings.Fields(lastLine)[2].


return goVersion
}

// Returns a value indicating whether the system Go toolchain supports workspaces.
Expand Down
4 changes: 3 additions & 1 deletion go/extractor/toolchain/toolchain_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ import (

func TestParseGoVersion(t *testing.T) {
tests := map[string]string{
"go version go1.18.9 linux/amd64": "go1.18.9",
"go version go1.18.9 linux/amd64": "go1.18.9",
"go version go1.26.3-X:nodwarf5 linux/amd64": "go1.26.3",
"go version go1.26.3rc1 linux/amd64": "go1.26.3rc1",
"warning: GOPATH set to GOROOT (/usr/local/go) has no effect\ngo version go1.18.9 linux/amd64": "go1.18.9",
}
for input, expected := range tests {
Expand Down
Loading