chore: update go version check in Makefile for minimum versions #3302

zivkovicmilos · 2024-08-29T13:08:20Z

Description

This PR updates the go version check in the top-level Makefile, that's causing the install Make directive to fail if higher version of go are available system wide (ex. 1.23) when running make install.

The go version check was previously introduced in #2095, whose faulty check caused this issue.

cc @MSalopek

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Included the correct type prefix in the PR title
Targeted the correct branch (see PR Targeting)
Provided a link to the relevant issue or specification
Reviewed "Files changed" and left comments if necessary
Confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

Confirmed the correct type prefix in the PR title
Confirmed all author checklist items have been addressed
Confirmed that this PR does not change production code

MSalopek · 2024-08-29T13:16:14Z

Thanks for opening the issue.

The changes are a little older #2095.

The intention was to prevent node operators from older versions of golang due to non-determinism issues that caused apphashes between go version v1.18.x and v1.19.x.

Given that newer go versions (starting from v1.21 perform additional steps and try to pull the required go tool, we can update the behaviour.

I propose we make it so that a minimum version is enforced. Using a version lower than the minimum should stil hardfail.

Any thoughts on this?

EDIT:
We attempted to remove this earlier this year but validators were not supportive of the change so the behavior where the build panics had to be re-introduced.

zivkovicmilos · 2024-08-29T13:43:33Z

@MSalopek
Thank you for the quick turnaround

Can you expand a bit on this? 👀

We attempted to remove this earlier this year but validators were not supportive of the change so the behavior where the build panics had to be re-introduced.

I'm trying to understand what benefits this make check provides in relation to the existing single go.mod version guard

zivkovicmilos · 2024-08-29T13:46:30Z

@MSalopek
Unrelated to this, it seems you have a CI issue where fork PR tests fail:

=== RUN   TestIntegrationTestSuite
    e2e_setup_test.go:127: setting up e2e integration test suite...
    e2e_setup_test.go:157: starting e2e infrastructure for chain A; chain-id: chain-5yIQ7I; datadir: /var/folders/dx/v25kly5d1ynd2y4ccs6cy77r0000gn/T/gaia-e2e-testnet-1433586664
    e2e_setup_test.go:274: created continuous_vesting genesis account cosmos1zcvyxwq6ns2y3s05h64te0c3xvj2wcedcl6whn
    e2e_setup_test.go:274: created delayed_vesting genesis account cosmos14q6xtpl8xawree5sl5q3kdnxn53epft3t2wlw7
    e2e_setup_test.go:274: created locker_vesting genesis account cosmos1wz4qqg307cc8mdw7ptw5ljdw4juq7ntmry0v9t
    e2e_setup_test.go:274: created periodic_vesting genesis account cosmos1xy77a6eu8s7m7d9s7hrrtqrghksxw768s8savz
    e2e_setup_test.go:543: starting Gaia chain-5yIQ7I validator containers...
    e2e_setup_test.go:575:
        	Error Trace:	/Users/zmilos/Work/gaia/tests/e2e/e2e_setup_test.go:575
        	            				/Users/zmilos/Work/gaia/tests/e2e/e2e_setup_test.go:161
        	            				/Users/zmilos/go/pkg/mod/github.com/stretchr/testify@v1.9.0/suite/suite.go:157
        	            				/Users/zmilos/Work/gaia/tests/e2e/e2e_setup_test.go:123
        	Error:      	Received unexpected error:
        	            	API error (404): pull access denied for cosmos/gaiad-e2e, repository does not exist or may require 'docker login': denied: requested access to the resource is denied
        	Test:       	TestIntegrationTestSuite
--- FAIL: TestIntegrationTestSuite (2.37s)
FAIL
FAIL	github.com/cosmos/gaia/v20/tests/e2e	5.539s
testing: warning: no tests to run
PASS
ok  	github.com/cosmos/gaia/v20/tests/integration	3.942s [no tests to run]
FAIL

MSalopek · 2024-08-29T14:03:54Z

@MSalopek Unrelated to this, it seems you have a CI issue where fork PR tests fail:

=== RUN   TestIntegrationTestSuite
    e2e_setup_test.go:127: setting up e2e integration test suite...
    e2e_setup_test.go:157: starting e2e infrastructure for chain A; chain-id: chain-5yIQ7I; datadir: /var/folders/dx/v25kly5d1ynd2y4ccs6cy77r0000gn/T/gaia-e2e-testnet-1433586664
    e2e_setup_test.go:274: created continuous_vesting genesis account cosmos1zcvyxwq6ns2y3s05h64te0c3xvj2wcedcl6whn
    e2e_setup_test.go:274: created delayed_vesting genesis account cosmos14q6xtpl8xawree5sl5q3kdnxn53epft3t2wlw7
    e2e_setup_test.go:274: created locker_vesting genesis account cosmos1wz4qqg307cc8mdw7ptw5ljdw4juq7ntmry0v9t
    e2e_setup_test.go:274: created periodic_vesting genesis account cosmos1xy77a6eu8s7m7d9s7hrrtqrghksxw768s8savz
    e2e_setup_test.go:543: starting Gaia chain-5yIQ7I validator containers...
    e2e_setup_test.go:575:
        	Error Trace:	/Users/zmilos/Work/gaia/tests/e2e/e2e_setup_test.go:575
        	            				/Users/zmilos/Work/gaia/tests/e2e/e2e_setup_test.go:161
        	            				/Users/zmilos/go/pkg/mod/github.com/stretchr/testify@v1.9.0/suite/suite.go:157
        	            				/Users/zmilos/Work/gaia/tests/e2e/e2e_setup_test.go:123
        	Error:      	Received unexpected error:
        	            	API error (404): pull access denied for cosmos/gaiad-e2e, repository does not exist or may require 'docker login': denied: requested access to the resource is denied
        	Test:       	TestIntegrationTestSuite
--- FAIL: TestIntegrationTestSuite (2.37s)
FAIL
FAIL	github.com/cosmos/gaia/v20/tests/e2e	5.539s
testing: warning: no tests to run
PASS
ok  	github.com/cosmos/gaia/v20/tests/integration	3.942s [no tests to run]
FAIL

Current main is undergoing changes, this is not relevant atm.
We have release branches that are stable. Main should not be considered 100% stable - it's not running on mainnet.

MSalopek · 2024-08-29T14:13:22Z

@MSalopek Thank you for the quick turnaround

Can you expand a bit on this? 👀

We attempted to remove this earlier this year but validators were not supportive of the change so the behavior where the build panics had to be re-introduced.

I'm trying to understand what benefits this make check provides in relation to the existing single go.mod version guard

Yes.

We removed the check once in the past and got some push back.
We have a validators discord group where the testnet and mainnet validators co-ordinate and report issues. The fact that the binary was able to build with an old golang version was raised as an issue during testnet period. In an earlier version of gaia, we noticed that there was a discrepancy in code execution when the binaries were using a different go version. I don't think those issues exist on versions go >=v1.21.x.

We would still like to stop building if you use an "old" golang version. go v1.21 seems like a good candidate for bare minimum version. We don't have a problem with "forward compatibility", we're mostly concerned with "backward compatibility".

Does it make sense?

zivkovicmilos · 2024-08-29T15:03:36Z

@MSalopek Thank you for the quick turnaround
Can you expand a bit on this? 👀

We attempted to remove this earlier this year but validators were not supportive of the change so the behavior where the build panics had to be re-introduced.

I'm trying to understand what benefits this make check provides in relation to the existing single go.mod version guard

Yes.

We removed the check once in the past and got some push back. We have a validators discord group where the testnet and mainnet validators co-ordinate and report issues. The fact that the binary was able to build with an old golang version was raised as an issue during testnet period. In an earlier version of gaia, we noticed that there was a discrepancy in code execution when the binaries were using a different go version. I don't think those issues exist on versions go >=v1.21.x.

We would still like to stop building if you use an "old" golang version. go v1.21 seems like a good candidate for bare minimum version. We don't have a problem with "forward compatibility", we're mostly concerned with "backward compatibility".

Does it make sense?

@MSalopek
Interesting, I still believe it's functionally unnecessary to check this with make, but still I'll apply your suggestion.

What do you think about this?

check_version:
ifneq ($(shell [ "$(GO_SYSTEM_VERSION)" \< "$(REQUIRE_GO_VERSION)" ] && echo true),)
	@echo "ERROR: Minimum Go version $(REQUIRE_GO_VERSION) is required for $(VERSION) of Gaia."
	exit 1
endif

MSalopek · 2024-08-29T15:09:42Z

@MSalopek Thank you for the quick turnaround
Can you expand a bit on this? 👀

We attempted to remove this earlier this year but validators were not supportive of the change so the behavior where the build panics had to be re-introduced.

I'm trying to understand what benefits this make check provides in relation to the existing single go.mod version guard

Yes.
We removed the check once in the past and got some push back. We have a validators discord group where the testnet and mainnet validators co-ordinate and report issues. The fact that the binary was able to build with an old golang version was raised as an issue during testnet period. In an earlier version of gaia, we noticed that there was a discrepancy in code execution when the binaries were using a different go version. I don't think those issues exist on versions go >=v1.21.x.
We would still like to stop building if you use an "old" golang version. go v1.21 seems like a good candidate for bare minimum version. We don't have a problem with "forward compatibility", we're mostly concerned with "backward compatibility".
Does it make sense?

@MSalopek Interesting, I still believe it's functionally unnecessary to check this with make, but still I'll apply your suggestion.

What do you think about this?
check_version:
ifneq ($(shell [ "$(GO_SYSTEM_VERSION)" \< "$(REQUIRE_GO_VERSION)" ] && echo true),)
	@echo "ERROR: Minimum Go version $(REQUIRE_GO_VERSION) is required for $(VERSION) of Gaia."
	exit 1
endif

LGTM.
I'm of the same opinion, but operators are used to it so I would like to keep it given previous experiences.

zivkovicmilos · 2024-08-29T15:16:47Z

@MSalopek

Updated in 0d91599

MSalopek

Approve.

I agree with enfocing a minimum version.

Drop go version check

d8a9095

zivkovicmilos requested review from zmanian, crodriguezvega, jackzampolin and a team as code owners August 29, 2024 13:08

Restore check, update it

0d91599

zivkovicmilos changed the title ~~chore: drop go version check from Makefile~~ chore: update go version check in Makefile for minimum versions Aug 29, 2024

MSalopek approved these changes Aug 29, 2024

View reviewed changes

Merge branch 'main' into dev/zivkovicmilos/go-version

1a06900

mpoke approved these changes Sep 2, 2024

View reviewed changes

Merge branch 'main' into dev/zivkovicmilos/go-version

98f1426

MSalopek merged commit 355dc9c into cosmos:main Sep 2, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update go version check in Makefile for minimum versions #3302

chore: update go version check in Makefile for minimum versions #3302

zivkovicmilos commented Aug 29, 2024 •

edited

Loading

MSalopek commented Aug 29, 2024 •

edited

Loading

zivkovicmilos commented Aug 29, 2024

zivkovicmilos commented Aug 29, 2024

MSalopek commented Aug 29, 2024

MSalopek commented Aug 29, 2024 •

edited

Loading

zivkovicmilos commented Aug 29, 2024

MSalopek commented Aug 29, 2024

zivkovicmilos commented Aug 29, 2024

MSalopek left a comment

chore: update go version check in Makefile for minimum versions #3302

chore: update go version check in Makefile for minimum versions #3302

Conversation

zivkovicmilos commented Aug 29, 2024 • edited Loading

Description

Author Checklist

Reviewers Checklist

MSalopek commented Aug 29, 2024 • edited Loading

zivkovicmilos commented Aug 29, 2024

zivkovicmilos commented Aug 29, 2024

MSalopek commented Aug 29, 2024

MSalopek commented Aug 29, 2024 • edited Loading

zivkovicmilos commented Aug 29, 2024

MSalopek commented Aug 29, 2024

zivkovicmilos commented Aug 29, 2024

MSalopek left a comment

Choose a reason for hiding this comment

zivkovicmilos commented Aug 29, 2024 •

edited

Loading

MSalopek commented Aug 29, 2024 •

edited

Loading

MSalopek commented Aug 29, 2024 •

edited

Loading