Go: Use new type for all semantic versions #16460
Conversation
| if v.goEnvVersionFound { | ||
| v.goEnvVersion = toolchain.GetEnvGoVersion()[2:] | ||
| } | ||
| v.goEnvVersion = toolchain.GetEnvGoSemVer() |
There was a problem hiding this comment.
We've lost a call to toolchain.IsInstalled(). Are you sure that that isn't needed? Do we have any tests that cover that case?
There was a problem hiding this comment.
Good point! Thanks for spotting this.
toolchain.IsInstalled() just looks up the path of the go executable with exec.LookPath and returns true or false depending on whether that succeeded.
Meanwhile, toolchain.GetEnvGoSemVer() calls toolchain.GetEnvGoVersion() which invokes go version. Unlike toolchain.IsInstalled() this fails if something goes wrong.
So, if go is not installed, then previously we would have ended up with no goEnvVersion, but now that would actually terminate the process. So that's definitely a problem.
There was a problem hiding this comment.
With respect to tests, I have some work on testing resolve build-environment specifically. I will try to get that done ASAP as well.
There was a problem hiding this comment.
No need to expedite the tests - I just meant that you could add it to your list of things that need testing.
There was a problem hiding this comment.
The work on those tests is almost done (I did 99% of it a few weeks ago) and they would have caught this.
There was a problem hiding this comment.
Actually those tests wouldn't have caught it either, because there is no nice way to simulate go not being installed in an integration test. I played around with different approaches, but I am not too keen on any of them. Happy to discuss the options I tried with you.
I instead looked at migrating our old resolve build-environment tests from when we initially worked on the feature to the internal repo, and have a draft PR for that there now.
I am also wondering if GetEnvGoSemVer should really use log.Fatalf or if we could just get away with returning nil or throwing it at util.NewSemVer to see if it can make sense of it. We'd have to consider the downstream implications of doing that, though.
For now, I have addressed this specific issue by restoring the check for toolchain.IsInstalled()
| // A value indicating whether a version string was found | ||
| Found bool | ||
| } | ||
| type GoVersionInfo = util.SemVer |
There was a problem hiding this comment.
We're now combining what used to be two different states: "found a go version, but it was invalid" and "didn't find a go version". Is it okay that we can't distinguish those any more?
There was a problem hiding this comment.
I think I checked to see if there were any cases where we cared about that distinction, but I'll check again to make sure.
| // The version in the `go.mod` file is above the supported range. There is no Go version | ||
| // installed. We install the maximum supported version as a best effort. | ||
| msg = "The version of Go found in the `go.mod` file (" + v.goModVersion + | ||
| ") is above the supported range (" + minGoVersion + "-" + maxGoVersion + | ||
| msg = "The version of Go found in the `go.mod` file (" + v.goModVersion.String() + |
There was a problem hiding this comment.
(musing) It's a shame that we have to have all these .String()s everywhere. If we were printing directly we could use %s and the String() method would automatically be called on the argument (I believe). But that would involve major refactoring.
There was a problem hiding this comment.
Yeah, I agree. While I was doing this I thought about replacing all of this with format strings. However, that seemed a bit unrelated to the work in this PR and I didn't want to make the diff even worse than it already is. We could look at doing that separately, though.
|
After the set of most recent changes, some things to check:
|
|
|
||
| // the version string may already contain a "-"; | ||
| // if it does, drop the "-" since we add it back later | ||
| if version[rcIndex-1] != '-' { |
There was a problem hiding this comment.
Nitpick: do we need to worry about the possibility that rcIndex is 0?
| // the "go" prefix, and ones with the "v" prefix. Go's non-semver-compliant release candidate | ||
| // versions are also automatically corrected from e.g. "go1.20rc1" to "v1.20-rc1". If given | ||
| // the empty string, this function return `nil`. Otherwise, for invalid version strings, the function | ||
| // prints a message to the log and exits the process. |
There was a problem hiding this comment.
Explain why we don't call semver.Canonical on the output.
Context
Throughout the Go autobuilder, we handle semantic versions. The
semverpackage has no dedicated type for them and all functions from the package work onstringvalues. This makes it difficult to understand where in the code we have something that we know is a valid semantic version and where we might have astringvalue that isn't. This is made worse bysemverrequiring all semantic versions to have avprefix.What this PR does
We introduce a new
SemVerinterface, backed by an implementation based on a private type, which is intended to guarantee that aSemVervalue is a valid semantic version. Since the type is not exported, theNewSemVerfunction must be used to constructSemVervalues. This function expects any version string as input and formats it according tosemver's requirements.All existing code in the autobuilder is modified to use the
SemVertype whereever we would previously have used functions from thesemverpackage.How to review
Best reviewed commit-by-commit.