[Merged by Bors] - Bump Go to 1.22.2

[poszu]

Description

Go 1.22.0 release notes: https://tip.golang.org/doc/go1.22

[codecov]

Codecov Report

Attention: Patch coverage is 95.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 80.2%. Comparing base (5d129a4) to head (f1891b7).

Files	Patch %	Lines
node/test_network.go	0.0%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5580     +/-   ##
=========================================
- Coverage     80.2%   80.2%   -0.1%     
=========================================
  Files          285     285             
  Lines        29754   29738     -16     
=========================================
- Hits         23883   23866     -17     
  Misses        4224    4224             
- Partials      1647    1648      +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fasmat]

I think we shouldn't update to 1.22 yet.

Tools and libraries might not yet be compatible with it yet and we are not yet using any of its features.

I went through some of the codebase to check where we can update the code to actually use 1.22 improvements and its quite a bit (WIP: bump-go-1.22...golang-1.22-features)

[poszu]

@fasmat

I think we shouldn't update to 1.22 yet.

There are quite a few tools and libraries not compatible with it yet (e.g. libp2p afaik) and we are not yet using any of its features.

The go-libp2p is a good point. We probably shouldn't update to go 1.22 until this issue is resolved: libp2p/go-libp2p#2706. What other tools and libraries block the upgrade?

I went through some of the codebase to check where we can update the code to actually use 1.22 improvements and its quite a bit (WIP: dependabot/docker/golang-1.22...golang-1.22-features)

Just because there are new functionalities in the language doesn't mean we should use them everywhere and upgrade the codebase in a single huge PR. I intentionally didn't change all for i := 0; i< x; i++ into for range x. Which version is better in each context depends.

[fasmat]

What other tools and libraries block the upgrade?

I saw staticcheck failing earlier, but I haven't checked everything yet. Some internals of the runtime changed with 1.22 which probably broke things (e.g. one of our tests because a different amount of memory is now allocated). I haven't checked yet but this might affect go-scale (where we do unsafe memory operations on strings and bytes).

golangci-lint reports errors like loopclosure: variable X captured by func literal although this is not a problem any more in 1.22. I am not sure if this is a golangci-lint issue or if we need to update our config accordingly, but my VSCode is now full of these false positives 😅.

gci seems to not understand that math/rand/v2 is a standard library package and tries to sort it with external imports.

Just because there are new functionalities in the language doesn't mean we should use them everywhere and upgrade the codebase in a single huge PR.

True, but I still think that we should at least migrate code from math/rand to math/rand/v2 and add the former to the list of not allowed packages in golangci-lint. math/rand/v2 is a big improvement over the old version, but cannot easily be used both in the same package.

[poszu]

Staticcheck required a bump and now passes. I haven't seen any issues with golangci-lint, the CI is clean 🤔.

[fasmat]

Staticcheck required a bump and now passes. I haven't seen any issues with golangci-lint, the CI is clean 🤔.

Try removing a tc := tc statement from any test (which is now possible in Go 1.22) and golangci-lint will start complaining. The same is also true if you import "math/rand/v2" in any file that currently uses "math/rand" and golanci-lint will start complaining about that as well:

syncer/find_fork_test.go:7: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/spacemeshos/go-spacemesh) (gci)
        "math/rand/v2"
syncer/find_fork_test.go:13: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/spacemeshos/go-spacemesh) (gci)
        "go.uber.org/mock/gomock"
...
activation/nipost_test.go:1156:19: loopclosure: loop variable tc captured by func literal (govet)
                                PublishEpoch: tc.epoch,

Both of which are false positives 🙂

[poszu]

Staticcheck required a bump and now passes. I haven't seen any issues with golangci-lint, the CI is clean 🤔.

Try removing a tc := tc statement from any test (which is now possible in Go 1.22) and golangci-lint will start complaining. The same is also true if you import "math/rand/v2" in any file that currently uses "math/rand" and golanci-lint will start complaining about that as well:

syncer/find_fork_test.go:7: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/spacemeshos/go-spacemesh) (gci)
        "math/rand/v2"
syncer/find_fork_test.go:13: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/spacemeshos/go-spacemesh) (gci)
        "go.uber.org/mock/gomock"
...
activation/nipost_test.go:1156:19: loopclosure: loop variable tc captured by func literal (govet)
                                PublishEpoch: tc.epoch,

Both of which are false positives 🙂

The loop variable false positive is already fixed in golangci-lint v1.56.2. The gci false-positive should be fixed in the next release of golangci-lint (it's already fixed on their master branch). Summing up, we need to wait for:

• golangci-lint v1.56.3
• version of go-libp2p that supports go1.22

[poszu]

@fasmat go-libp2p v0.33 which supports go1.22 was released already: https://github.com/libp2p/go-libp2p/releases/tag/v0.33.0

[fasmat]

@poszu but the problem with math/rand/v2 for golangci-lint formatting and the false positives about loop capture haven't been fixed yet or have they?

[poszu]

@poszu but the problem with math/rand/v2 for golangci-lint formatting and the false positives about loop capture haven't been fixed yet or have they?

No, there's no new version of golangci-lint yet. IMHO, bump to go1.22 shouldn't be blocked by that, we cannot use math/rand/v2 anyway in go1.21 🤷. Anyway, I hope it's a matter of days before a new version of golangci-lint is released.

[fasmat]

No, there's no new version of golangci-lint yet. IMHO, bump to go1.22 shouldn't be blocked by that, we cannot use math/rand/v2 anyway in go1.21 🤷.

I don't think I understand. The goal of this PR is to update 1.22 and the argument to not wait for compatibility is because after upgrading to 1.22 we are still using 1.21? What is the point of upgrading then? 😅

[poszu]

No, there's no new version of golangci-lint yet. IMHO, bump to go1.22 shouldn't be blocked by that, we cannot use math/rand/v2 anyway in go1.21 🤷.

I don't think I understand. The goal of this PR is to update 1.22 and the argument to not wait for compatibility is because after upgrading to 1.22 we are still using 1.21? What is the point of upgrading then? 😅

The goal is to update the toolchain to go1.22, not to migrate the codebase to things new in 1.22. The toolchain can be bumped without any issues. Just the fact that some linter will falsely scream if you use a tiny subset of new features shouldn't block from upgrading the toolchain in my opinion.

[fasmat]

The goal is to update the toolchain to go1.22, not to migrate the codebase to things new in 1.22. The toolchain can be bumped without any issues. Just the fact that some linter will falsely scream if you use a tiny subset of new features shouldn't block from upgrading the toolchain in my opinion.

I don't understand. As you said we are not using any of the new features of 1.22 and don't plan on doing so in the near future; actually using some of them will also block merging PRs because of golangci-lint. So why rush upgrading? Go 1.21 will be supported for another 6 months, we can update as soon as golanci-lint 1.56.4 is out?

[dshulyak]

beside changes in language and stdlib, there are also regular improvements in golang runtime. so if we will compile with new toolchain we will benefit from them. that said there are also sometime regressions in newer versions, and i personally would wait until 2 or 3 patch release.

[poszu]

@fasmat I bumped Go to 1.22.2. Do you think you could approve now?

[fasmat]

bors merge

[spacemesh-bors]

Merge conflict.

[fasmat]

bors merge

[spacemesh-bors]

Build failed:

• systest-status

[poszu]

Systest logs are incomplete, can't tell what went wrong - retrying.

bors retry

[spacemesh-bors]

Pull request successfully merged into develop.

Build succeeded:

• systest-status
• ci-status