chore: update Go version to `v1.22.0` (new MSRV for v1.x and main) by faddat · Pull Request #2285 · cometbft/cometbft

faddat · 2024-02-10T11:31:50Z

close: #2726

This PR updates go to v1.22.0

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments
Title follows the Conventional Commits spec

…to faddat/go-122

faddat · 2024-02-10T23:46:05Z

issue with pre-commit

This reverts commit ec35b57.

faddat · 2024-02-15T10:08:27Z

This PR requires that:

remove Dockerfile, use only pure go dabase backends cometbft-db#135

be merged in cometbft-db

faddat · 2024-02-22T13:01:43Z

this PR won't work without

remove Dockerfile, use only pure go dabase backends cometbft-db#135

github-actions · 2024-03-06T00:13:32Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

faddat · 2024-03-14T13:12:47Z

@sergio-mena this PR is held up by:

But I just noticed that some people were succeeding by using the full version of go (eg 1.22.0) so I'll try that.

Yep, now just blocked by cometbft-db

faddat · 2024-04-08T16:37:08Z

pacman -S package=1.2.3-1

faddat · 2024-04-08T16:49:42Z

Note, by getting Docker out of tm-db (which means dropping rocksdb support or finding good precompiled binaries) we can get this crazy docker situation out of our lives.

Till then I do suggest that we have a dockerfile here, and a dockerfile there. That way, we aren't blocked here on trivial stuff, like the version of Go we use.

Note to reviewers:

The docker image used here is a custom build that ensures that there's good support for both arm and amd64 because arch doesn't natively support that.

I think that we could get a really sensible maxim out of this:

"let's not use cgo, it is far more trouble than it is worth, and harms performance"

I made a PR to the tm-db repo 2-3 years ago with an arch image that would have saved a lot of time in compiling, it wasn't desired so I'm not making the same PR there again, but since we're chronically blocked by the docker image, doing so here seems to make sense.

faddat · 2024-04-08T17:05:49Z

heh, that doesn't seem to work either. I'm going to head to bed, but should figure out another solution tomorrowish.

Note: I would be very happy to move cometbft-db into this repo, even as a part of v1 (fast, I mean) because that would really simplify a lot of work, and make things easier for validators and other operators of comet nodes.

Lmk if that is an appealing solution from the maintainer's side

andynog

Overall looks good to me, added a few minor comments. The only comment that should drive a bit more attention is related to the change of the Docker image for the e2e. Maybe I'm missing the context on this one.

andynog · 2024-04-08T17:33:09Z

README.md

+| main             | Go version  | 1.22 or higher | 1.22        |
+| v1.x             | Go version  | 1.22 or higher | 1.22        |
+| v0.38.x          | Go version  | 1.20 or higher | 1.22        |
+| v0.37.x          | Go version  | 1.20 or higher | 1.22        |


should we be bumping the v0.37.x to Tested with 1.22 if it is in 1.20 in go.mod ? (same comment as the e2e workflow for v0.37.x

My opinion here is that for

v0.34.x - we should not deprecate this comet at this time because it is the eternal release. Maybe it gets deprecated at 1.0 + 1 release after. I really like 34, even though it is super old. For as long as we will support bugfixes, I'm a proponent of bumping the golang on the 34 release series to the latest release, at all times. There's a technical reason for this -- when chains upgrade, they should always upgrade to the very latest go. This prevents a situation where there are mixed golang runtimes.
v0.37.x - we should bump it to 1.22
v0.38.x - we should bump it to 1.22

Agree with @faddat wrt upgrades.

andynog · 2024-04-08T17:34:00Z

.github/workflows/e2e-nightly-37x.yml

      - uses: actions/setup-go@v5
        with:
-          go-version: '1.21'
+          go-version: "1.22"


should we be bumping the v0.37.x to Tested with 1.22 if it is in 1.20 in go.mod ? In the past we tested with one release higher than the one in go.mod, so should we leave this one as 1.21 ?

Personally, I'd use 1.22, and I'd never use a deprecated Go for a release that's still supported -- afaik, Go supports only the current release, and one before it.

But this is definitely a judgement call.

README.md

go.work

go.mod

andynog · 2024-04-08T18:13:22Z

test/e2e/docker/Dockerfile

Do we really need to change this e2e core Image for this PR ? It would be important to understand the reasoning for this change and how it relates to the core goal of this PR which is to bump v1 and main to go 1.22.

If this is not a requirement for the PR (e.g. it's a change for performance improvements), like bumping to 1.22 breaks the e2e tests and changing the image is the only way to fix it. Even though we'd probably need some additional assessment from people on the team that is more familiar with the e2e infra tests in order to evaluate the new image selected. So, if this is not a requirement I'd keep it separate from the PR since it can have implications to the e2e tests and it is unrelated.

andynog · 2024-04-08T19:51:51Z

docs/tutorials/go-built-in.md

+
 ```

 ## 1.1 Installing CometBFT


should update here too

cometbft/docs/tutorials/go-built-in.md

Line 144 in a46c5aa

go 1.21.8

adizere · 2024-04-09T14:14:17Z

@faddat do you plan to continue contrib here? if not, tomorrow we plan to take it over and push it to completion. cc @andynog

faddat · 2024-04-10T08:39:36Z

@andynog yeah, I think we just need to make a choice about that docker image. FYI it is a pretty hard and fast matter -- something needs to be done or it simply won't be able to run e2e tests.

Luckily I could be wrong here. Just a moment...

faddat · 2024-04-10T08:44:54Z

Ok :) I can mark this ready to go now, turns out we push the docker image at each commit that hits main in cometbft-db

Co-authored-by: Andy Nogueira <me@andynogueira.dev>

melekes

👍

…2285) close: #2726 This PR updates go to v1.22.0 --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev> (cherry picked from commit 15eb404) # Conflicts: # .github/workflows/e2e-nightly-1x.yml

…ackport #2285) (#2768) close: #2726 This PR updates go to v1.22.0 --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #2285 done by [Mergify](https://mergify.com). --------- Co-authored-by: Jacob Gadikian <jacobgadikian@gmail.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

faddat added 4 commits February 10, 2024 17:00

begin upgrade to v1.22

fba0b50

Merge branch 'main' into faddat/go-122

0d87248

update go in ci config

a7c1540

Merge branch 'faddat/go-122' of https://github.com/faddat/cometbft in…

caf9f56

…to faddat/go-122

faddat changed the title ~~deps: use golang v1.22.0~~ chore: use golang v1.22.0 Feb 10, 2024

faddat added 2 commits February 10, 2024 18:37

bump golang in test dockerfile

a10d37f

bump golang

d15d44d

faddat added 5 commits February 11, 2024 14:22

remove unnecessary gofumpt invocation

ec35b57

Revert "remove unnecessary gofumpt invocation"

7182214

This reverts commit ec35b57.

Merge branch 'main' into faddat/go-122

c28b52e

Merge branch 'main' into faddat/go-122

4da7859

bump golang

0dab8a5

faddat added 4 commits February 15, 2024 15:39

update readme.md

26eb86f

Merge branch 'main' into faddat/go-122

b4e0b0b

...adjust spacing in scripts/mockery_generate.sh

2bd1b9e

Merge branch 'main' into faddat/go-122

6407839

faddat mentioned this pull request Feb 22, 2024

feat: go 1.22.x and pebble v1.1.0 celestiaorg/celestia-node#3203

Closed

Merge branch 'main' into faddat/go-122

9521699

github-actions bot added the stale For use by stalebot label Mar 6, 2024

github-actions bot closed this Mar 10, 2024

sergio-mena reopened this Mar 13, 2024

Merge branch 'main' into faddat/go-122

fc78cdc

sergio-mena approved these changes Mar 13, 2024

View reviewed changes

github-actions bot removed the stale For use by stalebot label Mar 14, 2024

Merge remote-tracking branch 'origin/main' into faddat/go-122

c1c31c6

faddat added 3 commits April 8, 2024 22:39

Merge remote-tracking branch 'origin/main' into faddat/go-122

19ef1d8

satisfy codeql

33fd88c

go work sync

905af1b

faddat added 2 commits April 9, 2024 00:44

use arch, so we don't wait forever

c38dad9

fix pacman command

18cc27d

faddat added 4 commits April 9, 2024 00:50

fix rocksdb installation

656fd11

use a v in the rocksdb tag

175bfac

add rocksdb

35df8b8

arch: frequently too up to date

587b997

Merge branch 'main' into faddat/go-122

a46c5aa

faddat marked this pull request as draft April 8, 2024 17:15

andynog approved these changes Apr 8, 2024

View reviewed changes

Merge branch 'main' into faddat/go-122

b7da631

faddat added 2 commits April 10, 2024 16:39

Merge remote-tracking branch 'origin/main' into faddat/go-122

bacf570

use latest release of cometbft-db dockerfile

1020106

faddat marked this pull request as ready for review April 10, 2024 08:44

Update README.md

b9147b4

Co-authored-by: Andy Nogueira <me@andynogueira.dev>

melekes approved these changes Apr 10, 2024

View reviewed changes

melekes added this pull request to the merge queue Apr 10, 2024

Merged via the queue into cometbft:main with commit 15eb404 Apr 10, 2024

mergify bot mentioned this pull request Apr 10, 2024

chore: update Go version to v1.22.0 (new MSRV for v1.x and main) (backport #2285) #2768

Merged

4 tasks

Conversation

faddat commented Feb 10, 2024 • edited by andynog Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

faddat commented Feb 10, 2024

Uh oh!

faddat commented Feb 15, 2024

Uh oh!

faddat commented Feb 22, 2024

Uh oh!

github-actions bot commented Mar 6, 2024

Uh oh!

faddat commented Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

faddat commented Apr 8, 2024

Uh oh!

faddat commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

faddat commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andynog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adizere commented Apr 9, 2024

Uh oh!

faddat commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

faddat commented Apr 10, 2024

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

faddat commented Feb 10, 2024 •

edited by andynog

Loading

faddat commented Mar 14, 2024 •

edited

Loading

faddat commented Apr 8, 2024 •

edited

Loading

faddat commented Apr 8, 2024 •

edited

Loading

faddat commented Apr 10, 2024 •

edited

Loading