Update to Go 1.21 #2297
Update to Go 1.21 #2297
Conversation
go.mod
Outdated
| @@ -1,7 +1,7 @@ | |||
| module github.com/containers/skopeo | |||
|
|
|||
| // Minimum required golang version | |||
| go 1.20 // ***** ATTENTION WARNING CAUTION DANGER ****** | |||
| go 1.21 // ***** ATTENTION WARNING CAUTION DANGER ****** | |||
There was a problem hiding this comment.
My understanding is that along with this, we also need toolchain 1.21 as well? Please double-check, and also please remove the big warning comment since it no longer applies.
There was a problem hiding this comment.
My reading of “Go toolchain selection” in https://go.dev/doc/toolchain is that it is not compulsory, and effectively defaults to the go directive.
I don’t mind adding 1.21.0 here … and the comment records that as the original plan, so sure.
Do you want that comment removed entirely, or maybe kept up to the “ golang version consistency is desireable.” point?
There was a problem hiding this comment.
As I understand the situation (could easily be wrong), it's a complex interaction between Renovate proposed package update PRs and our CI systems.
Without the toolchain directive, and without any go version restrictions to Renovate, on Non-Fedora platforms (Debian, Mac, Windows), go will "auto-update" to the in-use version of go for the package update in question (as proposed by Renovate). Meaning for Renovate PRs, builds in CI might not be uniform across all platforms. In other words, Windows might use golang 5.4.3 for updated package FOO 0.2 while Fedora uses golang 1.21 for FOO 0.2 I'm assuming this is a bad/unexpected thing.
OTOH, setting a toolchain forces the maximum go version, and both go and Renovate will honor that for non-Fedora platforms. Meaning builds across all platforms will be exactly the same (for all shared packages).
Note: The only thing making Fedora special in these matters is they set some magic envar by default which disables the auto-update behavior.
@Luap99 please double-check my understanding, when you have a chance.
There was a problem hiding this comment.
The way I read the paragraph linked above, toolchain is the minimum Go version; it triggers unwanted upgrades (but not downgrades).
… and IIRC the effect with Renovate included is that at the times where Renovate adds a toolchain directive, it adds one that matches Renovate’s default toolchain.
… and that behavior we don’t want, you’re right.
I’ll add the toolchain directives tomorrow. (The renovate hard-coded version will, I think, orthogonally, need bumping to 1.21.)
There was a problem hiding this comment.
yes I think we should have the toolchain there so the normal go commands can be used without adding the toolchain later, especially so renovate can be used properly.
I do however think that we need to make sure the toolchain is only ever set to the same value as the go line so we do not force update users unnecessary.
There was a problem hiding this comment.
and yes toolchain is the minimum version, not maximum.
There was a problem hiding this comment.
I have added toolchain 1.21.0, and updated the comment a bit.
I’m very open to suggestions about the comment.
There was a problem hiding this comment.
I think the comment just needs to say:
Warning: Ensure the "go" and "toolchain" versions match exactly to prevent unwanted auto-updates
|
After this and containers/image#2377 merge, do you have plans for buildah and podman? Reason is, we're currently blocking all updates that require anything past Meaning, that will either need to be removed, or relocated to the buildah and podman Renovate config overrides. |
Oh, I completely forgot about that. At least I figured the Cc: list out :) After c/image updates in containers/image#2377 , consumer repos will be forced to update shortly in order to consume c/image. So (assuming we can safely trigger the update), I think bumping that to Go 1.21 for all repos would be appropriate. |
|
(Marking as draft to make sure we have a decision the Renovate configuration first; I understand Renovate would immediately start failing otherwise.) |
I don't believe it will fail, it will simply (and mostly silently) refuse to open any module update PRs which also require a bump of |
|
IIRC from the F38 situation, the Go 1.20 toolchain refuses to work on Either way, “Renovate silently stops filing PRs” is the worst kind of failure mode for me. |
No it will just fail to compile if the repo actually uses newer features, only starting with 1.21 go will enforce the minimum go version stated in go.mod. So we should never set a newer go version than our supported distros use. |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
The build log does contain mentions of /usr/lib/golang/src/vendor/github.com/golang-fips/openssl/v2 , so the FIPS patch does seem to be present. Best I can trace this, golang-fips/go@56ac3db has removed the @derekparker, is the above correct, and if so, what is the recommended migration path? (I seem to vaguely remember that the upstream |
|
Hey, Yes it is correct, that particular symbol is removed in the |
|
@mtrmac @Luap99 Just to be sure I'm keeping-up: Before this (and the c/image) PR merges, I need to open PRs for Buildah and Podman that disable Renovate updates past go 1.20. After those merge, I need another PR to disable the global Renovate go 1.20 limitation. Then this (and the c/image) PRs are safe to merge. Somehow that sounds overcomplicated 😞 |
I see no reason why this couldn't be merged before updating the renovate config. Sure vendor PRs that need a newer golang will be broken but so are they today so there is really no difference regardless of merge order. Ideally these changes are merged sooner than later given the vast amount of deps that need go 1.21 so we can unlock many updates. |
I think that once this merges, Buildah and Podman need to be structurally ready to accept 1.21 (because they will get it on any c/image bump). So I was thinking just bump Renovate globally to 1.21 and that’s all. Probably pay close attention to the Alternatively, we could line up PRs to bump to 1.21 with a |
Yeah that should happen, and even if someone somewhere merges a PR with a to new toolchain directive we can always manually revert it back to an older version so no big problem. |
|
Great, this sounds like a problem that can be solved easily by a short e-mail 😁 |
|
PR merged and e-mail sent. |
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Not "maps" because maps.Keys is not included. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Dealing with this continues in #2305 . |
|
LGTM |
|
(I think those test failures are orthogonal, but it does make a lot of sense to fix those failures before introducing more changes.) |
|
we can ignore the 2 centos-stream failures for now. To use TMT tests, we'll have to switch to |
… primarily because some dependencies started to require it: if we ever needed to quickly update a dependency for a vulnerability fix, we might have to update to Go 1.21 on a short notice, or fork the dependency.
Cc: @jnovy @lsm5 @cevich @TomSweeneyRedHat