go: Add Go modules support #486
Conversation
Codecov Report
@@ Coverage Diff @@
## master #486 +/- ##
=======================================
Coverage 91.14% 91.14%
=======================================
Files 20 20
Lines 2970 2970
=======================================
Hits 2707 2707
Misses 263 263 |
go.mod
Outdated
| @@ -0,0 +1,3 @@ | |||
| module github.com/ethereum/evmc/v7 | |||
|
|
|||
| go 1.14 | |||
There was a problem hiding this comment.
The readme say "Go (bindings) | 1.9 - 1.12". Is this changing that?
There was a problem hiding this comment.
you can still build it on the old golang, but go modules will require go 1.14
I can adjust README
There was a problem hiding this comment.
updated the readme file
There was a problem hiding this comment.
I'm not sure what this mean, but this works in Go 1.11 (the first version that supports modules).
There was a problem hiding this comment.
I changed that to 1.11 as some random comment on internet this is minimum Go version of the module.
|
turbo-geth code that uses this PR: https://github.com/ledgerwatch/turbo-geth/tree/evmc-upstream/ |
| @@ -0,0 +1,3 @@ | |||
| module github.com/ethereum/evmc/v7 | |||
There was a problem hiding this comment.
So it is enough to only declare it as "v7" here but use dir structure unchanged?
bindings/go/evmc/evmc.go
Outdated
| @@ -15,6 +15,8 @@ package evmc | |||
| #include <stdlib.h> | |||
| #include <string.h> | |||
|
|
|||
| #include "loader/loader.c" | |||
There was a problem hiding this comment.
I think this should be a problem because the C comment in go files are compiled twice if any Go function is exported. I think it only works because there is nothing exported here.
I may have modified version of this, will try to push it there.
There was a problem hiding this comment.
It misses some definitions w/o that import for some reason... At least on a mac.
|
I pushed some additions, including an integration test for Go module. |
| @@ -0,0 +1,3 @@ | |||
| module github.com/ethereum/evmc/v7 | |||
There was a problem hiding this comment.
Why is there a need for v7 here? If we bump to ABI 8 and this is changed to v8, will go modules still be able to download v7 things?
Or the only benefit is knowing it is v7 even if someone uses a commithash after it?
There was a problem hiding this comment.
This is required by go mod. The vX is required if you version is vX.Y.Z. Can be only omitted if X==1 || X==0.
Versions are marked with git tags, and v7 series will continue to work (because e.g. at v7.3.0 this line will be still module github.com/ethereum/evmc/v7.
The usage is something like this:
go get github.com/ethereum/evmc/[email protected]
go get github.com/ethereum/evmc/[email protected]
go get github.com/ethereum/evmc/[email protected]
And also
import "github.com/ethereum/evmc/v7"
import "github.com/ethereum/evmc/v8"So users need to update their import directives when upgrading major version number. In case of EVMC where vX matches ABI version this is reasonable IMHO.
There was a problem hiding this comment.
So it is just a convention required by go mod, it won't magically make v7 work when we move on to v8, because it still just downloads from the repo master, unless a commit hash is specified, correct? Or are the versions used as git tags?
There was a problem hiding this comment.
go get github.com/ethereum/evmc/[email protected]will fail due to mismatch.go get github.com/ethereum/evmc/v7@masterwill also fail if the most recent version isv8.Y.Z.go moduses a kind ofgit describestrategy to figure out the "current" version.
| @@ -0,0 +1,3 @@ | |||
| module github.com/ethereum/evmc/v7 | |||
There was a problem hiding this comment.
Maybe in a future PR, one could update bumpversion to also update this line.
|
Do we have documentation on the bindings? I don't think we have them yet, but maybe we should, where example uses could be explained. |
We don't have any Go documentation. The standard Go packages documentation hosting is https://pkg.go.dev/. For general documentation migration to readthedocs would be nice. |
Go modules support by @mandrigin.
Closes #450