Upgrade to Go 1.27.1 and address the lint findings it surfaces - #4098
Upgrade to Go 1.27.1 and address the lint findings it surfaces#4098masih wants to merge 14 commits into
Conversation
Upgrade the entire go module to latest go 1.27.1 and address lint issues, adjust CI builds, etc.
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4098 +/- ##
==========================================
+ Coverage 61.58% 62.13% +0.54%
==========================================
Files 2196 2106 -90
Lines 193825 182135 -11690
==========================================
- Hits 119365 113161 -6204
+ Misses 62888 61728 -1160
+ Partials 11572 7246 -4326
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
# Conflicts: # sei-cosmos/telemetry/wrapper.go
…n behavior changes, narrow nolints
…e go toolchain helper
…3-plus-4 # Conflicts: # sei-cosmos/x/gov/types/genesis.pb.go
PR SummaryHigh Risk Overview Lint policy shifts with the bump: Operator / consensus impact (documented in CHANGELOG):
Smaller follow-ons include loosened EVM Reviewed by Cursor Bugbot for commit e795f73. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
A large but overwhelmingly mechanical Go 1.27.1 / golangci-lint v2.13.2 upgrade; the behaviour-bearing parts (snapshot format 1→2 with dual-format restore, CommitSig.FromProto validation, the p2p frame-header bound, context-aware HTTP dialling) are documented in CHANGELOG.md and covered by updated tests, and the .pb.go churn is descriptor-bytes/comment reflow only. No blockers; a few maintainability and documentation nits around the new toolchain plumbing.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
integration_test/autobahn/README.mdstill states "Both targets require Go 1.25.6 andmake", whilecmd/autobahn-e2e/aws.go— the AWS target that README documents — now installsgo1.27.1. Worth updating alongside the other version references. - [suggestion]
sei-tendermint/scripts/mockery_generate.shalso bumps mockery from v2.53.4 to v2.53.7, which is unrelated to the toolchain change and not mentioned in the PR description. No mocks are regenerated in this PR, so if the generator's output changed the committed mocks are now out of step with what the script produces. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| INVARIANT_CHECK_INTERVAL ?= $(INVARIANT_CHECK_INTERVAL:-0) | ||
| # Pinned here so the lint targets and .github/workflows/golangci.yml cannot drift apart. | ||
| GOLANGCI_LINT := go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.8.0 | ||
| GOLANGCI_LINT := GOTOOLCHAIN=$(shell ./scripts/go-toolchain.sh) go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.13.2 |
There was a problem hiding this comment.
[suggestion] Because this is a simple (:=) assignment, $(shell ./scripts/go-toolchain.sh) runs at parse time on every make invocation, not just the lint targets. scripts/build-static.sh runs make build inside golang:1.27.1-alpine, which has no bash (apk add build-base git doesn't pull it), so the script's #!/usr/bin/env bash fails there and every static build now emits env: can't execute 'bash' before proceeding with an empty GOTOOLCHAIN.
Switching to a recursive assignment (GOLANGCI_LINT = GOTOOLCHAIN=$(shell ./scripts/go-toolchain.sh) go run ...) confines the subprocess to the recipes that actually expand $(GOLANGCI_LINT), which is exactly the lint targets.
| exit 1 | ||
| } | ||
|
|
||
| go_toolchain_for() { |
There was a problem hiding this comment.
[suggestion] This go_toolchain_for helper is copied verbatim into .github/scripts/release-upgrade-test.sh and upgradetest/compile_offline.sh, and it re-implements the same patch-less-version normalisation that the new scripts/go-toolchain.sh already owns — four places that must agree on the rule.
Since all three callers run from the current checkout, they can invoke the shared script instead if it takes the go.mod directory as an optional argument (defaulting to the repo root), e.g. "$REPO_ROOT/scripts/go-toolchain.sh" "$worktree". That puts the normalisation at the single choke point rather than at each caller.
Moves the module to Go 1.27.1 and golangci-lint v2.13.2 and fixes what the new linters report.
Toolchain
go.mod, Dockerfiles and every workflow now target Go 1.27.1;setup-gobumped to v7,golangci-lint-actionto v9.go.modeverywhere: newscripts/go-toolchain.sh(normalises a patch-lessgo 1.28togo1.28.0) is shared by the Makefile,golangci.ymlandmockery_generate.sh.upgradetestharness pins each worktree's toolchain from its owngo.mod. Releases up torelease/v6.7cannot build on Go 1.27 (pinnedcockroachdb/swiss); operators building old tags need the same.Lint
precompiles/*/legacyandutils/helpers/legacytrees are excluded in.golangci.yml, not edited. Livesei-cosmos/codec/legacyandx/auth/legacystay linted.precompiles/p256/verifier.gois byte-identical tomain; its SA1019 is silenced by a.golangci.ymlpath rule sobump_versiondoes not mint a new legacy copy.goconstdisabled (more noise than it is worth); duplicatemisspellremoved.reflect.Pointer,bytes.Clone,fmt.Fprintf,slices.*, stdlibcmp.Ordered, narrowed line-level//nolintwith reasons.Operator-facing changes (also in CHANGELOG.md)
compress/flateemits different bytes for the same input; that reaches state-sync snapshots via the multistore zlib stream and the gzipped wasm code inside it. Upgraded nodes restore both formats; old binaries answerREJECT_FORMATto format-2 snapshots, so keep old-binary snapshot providers up until the rollout completes.CommitSig.FromProtorejects ablock_id_flagoutsideABSENT/COMMIT/NILat decode instead of truncating throughuint8(257used to wrap toAbsentand passValidateBasic).Smaller behavioural effects
eth_callarguments changed wording (encoding/jsonv1 is now a shim over v2). Still geth-identical; the rpc integration tests, the exact-bytes gzip test and the JSON-error-text unit test were loosened accordingly.Env/Infowere checked, and the Determinism and geth-parity jobs pass.