go.mod: Force 1.22.4 as min go version #4407
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know whether there are some projects which require runc lib, but they don't use
nsexec, and can't upgrade to 1.22.4 or higher version.These projects will compile failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use a per file version constraint instead so the parts of runc that do need 1.22+ fail to build without it but everything else can still be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like golang doesn't support minimal version grammer in the go compile directives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, we could specify
//go:build go1.22(which means 1.22+) but notgo1.22.4, I see 😞There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something, but I can't see why this is a problem. For several reasons:
Here we are changing from enforce go 1.22.0 to enforce 1.22.4 as min go version, that is all. People that can't use go 1.22 can't use runc main today either (unless they use really older versions and in that case, this change doesn't change them anything either)
Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So they will have to update the minimum version to 1.22.4, right? What is the issue? I don't really understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why they didn't update the minimum version of go, maybe there are some issues in their project, I don't know.
Before this patch, they can compile in 1.22.2 successful, so you mean it's the job of their own, if they want to bump runc to 1.2.0-*, they need to update the min go version. We should not consider such type projects which require runc (without nsexec).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And that is only if they don't merge this PR before upgrading the runc module: cri-o/cri-o#8597
The runc 1.2.0 final release won't be finished in the next few days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lifubang I don't understand where that would be a problem. Sure, you need a newer version of go, instead of 1.22.2, you need 1.22.4. When that is an issue that we really want to let users compile this with 1.22 < 1.22.4?
I mean, 1.22.4 is out for some time (1.22.7 is the latest now), if it introduced a regression, they didn't report it and then all the go 1.22.x > 2 cause an issue for them... Sure, this will be problematic. If they reported the regression, it seems likely that by 1.22.7 it was already fixed.
I think it is unlikely, and we can revisit this decision if some user is actually in such a weird situation.
(and again, I doubt cri-o is in that situation, given that they have a PR open to change the min go version to 1.23 already. The one I linked in my previous comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, taking a step back I think I see what you mean. It's just about projects using "1.22" in the go line that will need to change it when upgrading runc, and maybe that change is not even needed (as they are not using nsexec).
Luckily, that is why the go team added the "toolchain" line with this min go version change. We can suggest go 1.22.4 (using toolchain 1.22.4) when compiling runc, but for projects that use it as a module the go line applies.
I'll update the PR to do that later. I'll mark it as draft until then.
Thanks for the feedback :)