Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure go v1.18 used in make build & make install #3634

Merged
merged 2 commits into from
Dec 6, 2022

Conversation

ValarDragon
Copy link
Member

@ValarDragon ValarDragon commented Dec 5, 2022

What is the purpose of the change

Go 1.19 had changes that are consensus breaking. Thank you to @jhernandezb for pointing this out. This applies his suggested patch to Makefiles to ensure that all binary builds are done with the same go minor release line.

Brief Changelog

  • Require go 1.18 installed for building the binary.

Testing and Verifying

Try building the binary if you don't have go 1.18, or build being a success if you do have go 1.18.

I've tested both

@ValarDragon ValarDragon added A:backport/v12.x backport patches to v12.x branch V:state/compatible/backport State machine compatible PR, should be backported A:backport/v13.x backport patches to v13.x branch labels Dec 5, 2022
@ValarDragon ValarDragon changed the title Apply @jhernandez ' patch to Makefile Ensure go v1.18 used in make build & make install Dec 5, 2022
@faddat
Copy link
Member

faddat commented Dec 6, 2022

Alternative suggestion:

We should require go 1.19. This way, there's no chance of those breaking changes cropping up because that's the latest release (no mixing of runtimes)

@ValarDragon
Copy link
Member Author

I'm worried about changing to go 1.19 right now so close to release. I think in the future we should do latest minor version, but this one was tested + has binaries out with 1.18

Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

ACK

@ValarDragon ValarDragon merged commit f52cb53 into main Dec 6, 2022
@ValarDragon ValarDragon deleted the dev/stargaze_makefile_patch branch December 6, 2022 02:52
mergify bot pushed a commit that referenced this pull request Dec 6, 2022
* Apply @jhernandez ' patch to Makefil

* Add changelog

(cherry picked from commit f52cb53)

# Conflicts:
#	CHANGELOG.md
mergify bot pushed a commit that referenced this pull request Dec 6, 2022
* Apply @jhernandez ' patch to Makefil

* Add changelog

(cherry picked from commit f52cb53)
ValarDragon added a commit that referenced this pull request Dec 6, 2022
* Apply @jhernandez ' patch to Makefil

* Add changelog

(cherry picked from commit f52cb53)

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
@08d2
Copy link

08d2 commented Dec 6, 2022

If some code works when compiled with Go 1.18, and breaks when compiled with Go 1.19, this represents a serious problem with that code that absolutely needs to be fixed. Is there a summary or description of the problem somewhere?

@08d2
Copy link

08d2 commented Dec 6, 2022

This PR breaks all users who have an up-to-date version of go on their build hosts. One can fetch older Go versions and call them as e.g. go1.18 build ..., which would in theory solve the problem here, but it doesn't work because the Makefile doesn't parameterize calls to the go tool.

Quick fix would be

GO ?= go

GO_MAJOR_VERSION = $(shell ${GO} version | cut -c 14- | cut -d' ' -f1 | cut -d'.' -f1)
GO_MINOR_VERSION = $(shell ${GO} version | cut -c 14- | cut -d' ' -f1 | cut -d'.' -f2)

$(BUILD_TARGETS): check_version go.sum $(BUILDDIR)/
	${GO} $@ -mod=readonly $(BUILD_FLAGS) $(BUILD_ARGS) ./...

with equivalent changes (go${GO}) throughout the file.

@ValarDragon
Copy link
Member Author

Oh woah, I didn't know about this! Thats really cool

@ValarDragon
Copy link
Member Author

If some code works when compiled with Go 1.18, and breaks when compiled with Go 1.19, this represents a serious problem with that code that absolutely needs to be fixed. Is there a summary or description of the problem somewhere?

So go minor versions have been known to break things at the CGO boundary numerous times often as bug patches. Its been reported that there have been crashes / breakages due to this in at least two places:

  • Sort behavior has been changed. (golang sort is unstable, so the unstability altered. This is what halted persistence)
  • Gzip encoding edge cases were changed, which breaks hash compatibility of state sync gossip across versions

These are both will little testing in prod thus far

@08d2
Copy link

08d2 commented Dec 11, 2022

Sort behavior has been changed. (golang sort is unstable, so the unstability altered . . .)

It would be a serious problem if the sort behaviors expressed by the Go stdlib API(s) broke between versions. What do you mean by golang sort exactly?

Any code that returns data which should go on-chain must be deterministic, no matter what the language or client or application version is. If that code uses a sort func which isn't stable, that's huge bug in the code, not a problem in the dependency providing the sort func, right?

Gzip encoding edge cases were changed, which breaks hash compatibility of state sync gossip across versions

Er, what? Gzip is neither deterministic nor bijective. The gzip encoding/compression of some data X has arbitrarily many representations X.gz, and arbitrarily many representations of X.gz will decode/decompress to the same data X. If there is any code which assumes otherwise, that's a huge bug in that code, not a problem in the gzip implementation, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v12.x backport patches to v12.x branch A:backport/v13.x backport patches to v13.x branch T:build V:state/compatible/backport State machine compatible PR, should be backported
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants