Skip to content

Update to Go 1.25#692

Merged
mtrmac merged 12 commits into
containers:mainfrom
mtrmac:go1.25
Mar 12, 2026
Merged

Update to Go 1.25#692
mtrmac merged 12 commits into
containers:mainfrom
mtrmac:go1.25

Conversation

@mtrmac
Copy link
Copy Markdown
Contributor

@mtrmac mtrmac commented Mar 11, 2026 •
edited
Loading

Use new features, clean up what go fix found, and avoid the old-style sort.

See individual commit messages for details.

@github-actions github-actions Bot added storage Related to "storage" package common Related to "common" package image Related to "image" package labels Mar 11, 2026
@mtrmac mtrmac force-pushed the go1.25 branch 3 times, most recently from bf3cf05 to b31821c Compare March 12, 2026 02:32
Luap99
Luap99 reviewed Mar 12, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

you have two "Use slices.SortFunc instead of sort.Sort" commits which looks a bit odd, should they be squashed or if we like to have parts for easier bisect then part 1/2 names?

Comment thread common/libnetwork/cni/run_test.go
Expect(res[defNet].DNSServerIPs).To(BeEmpty())
Expect(res[defNet].DNSSearchDomains).To(BeEmpty())
var wg sync.WaitGroup
wg.Add(1)
Copy link
Copy Markdown
Member

@Luap99 Luap99 Mar 12, 2026

Choose a reason for hiding this comment

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

there are WIP PRs to remove all cni so in the interest of avoiding merge conflicts it may be best to not touch cni files, though I guess that conflict would be easy to resolve so it should not matter to much
cc @lsm5

@Luap99
Copy link
Copy Markdown
Member

Luap99 commented Mar 12, 2026

you have two "Use slices.SortFunc instead of sort.Sort" commits which looks a bit odd, should they be squashed or if we like to have parts for easier bisect then part 1/2 names?

Ah nvm me, sort.Sort vs sort.Slice so all good

mtrmac added 12 commits March 12, 2026 19:03
@mtrmac
Update to Go 1.25
2d16982 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Use sync.WaitGroup.Go()
26a5929 
Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Use fmt.Appendf
2365ec1 
Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Do not use omitempty on structs
97aaa3a 
It does nothing.

We might want to use "omitzero" in various cases, but
if no-one has noticed so far, it's probably not urgent.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Use "for range" over integers
ef4c408 
... at least where we don't care about the values, I rather
dislike using this when we need to do range over exactly [0, N).

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Use for over a slice to iterate over its indices
2ed2456 
Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Update more code to iterate over slice indices instead of manual bounds
bfb49e2 
Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Use slices.Contains
086fe78 
Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Use slices.SortFunc instead of sort.Sort
e70656f 
It's shorter code, and less chance of typos.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Use slices.SortFunc instead of sort.Slice
4e6b120 
It is typically safer and expected to be faster.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Use strings.Builder
c2ba0f8 
Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Use bytes.Cut
ddad872 
... and then preallocate an array to avoid a now-visible
warning.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@Luap99
Copy link
Copy Markdown
Member

Luap99 commented Mar 12, 2026

@mtrmac Not sure if the test failures are real here? Maybe rebase and I then I am good to merge if it passes.

Luap99 added a commit to Luap99/automation that referenced this pull request Mar 12, 2026
@Luap99
renovate: update to go 1.25
5d25e5e 
We updated most repos to 1.25, only container-libs is not yet merged
containers/container-libs#692

I am not sure this change is needed, it seems renovate PRs are correctly
created with go 1.25 despite this still being set to 1.24. Regardless
lets be safe and bump this for now.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
@Luap99 Luap99 mentioned this pull request Mar 12, 2026
Merged
@mtrmac
Copy link
Copy Markdown
Contributor Author

mtrmac commented Mar 12, 2026

Almost certainly flakes, anyway rebased.

aguidirh
aguidirh reviewed Mar 12, 2026
View reviewed changes
Comment thread common/libimage/image_test.go
names = append(names, name)
go func(name string) {
defer wg.Done()
wg.Go(func() {
Copy link
Copy Markdown
Contributor

@aguidirh aguidirh Mar 12, 2026

Choose a reason for hiding this comment

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

wg.Go requires that the func f does not panic. Is require.NoError going to panic in case of failures?

Copy link
Copy Markdown
Contributor Author

@mtrmac mtrmac Mar 12, 2026

Choose a reason for hiding this comment

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

Well the rules in testing say that FailNow (and therefore require) can only be used on the original goroutine of the test, so this test is already buggy in that sense. I think we are violating that rule all over the place…

Ultimately, maybe It doesn’t really matter whether we fail via an assertion error in the testing framework, via an uncaught panic, or via (as I think will happen) this goroutine exiting, marking the test as failed, but not truly aborting the rest of the test:

func TestGroup(t *testing.T) {
	wg := sync.WaitGroup{}
	wg.Go(func() {
		t.Log("sub 1")
		require.False(t, true, "test failed")
		t.Log("sub after fail")

	})
	t.Log("main waiting")
	wg.Wait()
	t.Log("main after wait")
}

produces

=== RUN   TestGroup
    …: main waiting
    …: sub 1
    …:
                Error Trace:    …
                Error:          Should be false
                Test:           TestGroup
                Messages:       test failed
    …: main after wait
--- FAIL: TestGroup (0.00s)

where the parent goroutine does continue, but otherwise it works out… fine enough.

(A clean fix could be to use x/sync/errgroup to collect the errors.)

Copy link
Copy Markdown
Contributor

@aguidirh aguidirh Mar 13, 2026

Choose a reason for hiding this comment

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

Got it, so it LGTM. Thanks!

Comment thread common/libimage/image_test.go
names = append(names, name)
go func(name string) {
defer wg.Done()
wg.Go(func() {
Copy link
Copy Markdown
Contributor

@aguidirh aguidirh Mar 12, 2026

Choose a reason for hiding this comment

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

Same comment as R312

@mtrmac mtrmac merged commit 32a48a8 into containers:main Mar 12, 2026
38 checks passed
@mtrmac mtrmac deleted the go1.25 branch March 12, 2026 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Luap99 Luap99 Luap99 left review comments

+1 more reviewer

@aguidirh aguidirh aguidirh left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

common Related to "common" package image Related to "image" package storage Related to "storage" package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mtrmac @Luap99 @aguidirh