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

Go modules #19605

Closed
wants to merge 10 commits into from
Closed

Go modules #19605

wants to merge 10 commits into from

Conversation

tzdybal
Copy link

@tzdybal tzdybal commented May 21, 2019

Go 1.13, the next release of Go, will enable modules by default.
This PR introduces Go modules into go-ethereum.

env.sh script was removed, as it's not needed anymore. The behavior of build process remains the same - project can be build from any directory, inside or outside of $GOPATH.

@hadv
Copy link
Contributor

hadv commented May 30, 2019

We can remove the vendor folder also if we switch to use go mod, right?

@fjl fjl added this to the 1.9.1 milestone Jun 6, 2019
@karalabe karalabe modified the milestones: 1.9.1, 1.9.2 Jul 23, 2019
@i-norden
Copy link
Contributor

i-norden commented Jul 31, 2019

@tzdybal I'm curious if you have tried this update closer to the head state? go mod inititalization for me at 7f33625 throws unknown revision errors for usb and protobuf dependencies, which have both changed since this was opened.

errors:
go: github.com/karalabe/usb@v0.0.0-20190703132631-6a7de9d893fe: unknown revision 6a7de9d893fe

and

github.com/gogo/protobuf@v0.0.0-00010101000000-000000000000: unknown revision 000000000000

In the case of karalable/usb the issue appears to be exactly what it says, there is no revision 6a7de9d893fe in the repo anymore. There was on another branch but that was merged in as 9be757f. I will open an issue/PR for this unless I realize I'm missing something...

Not sure what the deal is with protobuf though, as the revision we should be using at 7f33625 is b285ee9cfc6c881bb20c0d8dc73370ea9b9ec90f which does exist at that repo, but for some reason it is trying to use the github.com/gogo/protobuf repo which does not have that revision (even though my go.mod has github.com/golang/protobuf v1.3.1). I'm new to go modules so probably missing something obvious here.

@hadv yeah this would enable removal of the vendor dir!

@tzdybal
Copy link
Author

tzdybal commented Aug 8, 2019

I'll re-approach this again in few days. I want to fix problem with linting, and ofc all the errors related to go mod.

@karalabe karalabe modified the milestones: 1.9.2, 1.9.3 Aug 13, 2019
@karalabe karalabe modified the milestones: 1.9.3, 1.9.4 Sep 4, 2019
@tzdybal tzdybal requested a review from gballet as a code owner September 12, 2019 11:26
@tzdybal
Copy link
Author

tzdybal commented Sep 12, 2019

Latest changes:

  • rebased to to catch-up with master
  • updated go.mod and go.sum and fixed problem with pcsc
  • removed vendor folder
  • switched to golangci-lint
  • updated .travis.yml to use go 1.13.x
  • changed bind_test to use local temporary folder (because of modules)

Please re-review.

Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

Please use the latest version of go-libpcsclite

go.mod Outdated Show resolved Hide resolved
accounts/scwallet/hub.go Outdated Show resolved Hide resolved
Approach with `_workspace` directory was replaced with Go modules.
gometalinter was updated to v3.0.0.
To keep things simple - force using modules to install specific version
of gometalinter. Without this, current working directory and GO111MODULE
environment variable must be used to decide whether to use versioned or
unversioned `go get`.
Because of go modules, it's easiest to create tests in local
subdirectory, to ensure that correct version of dependencies is used.
@tzdybal
Copy link
Author

tzdybal commented Sep 16, 2019

All linting errors resolved. Rebased with master. Build passing.

@karalabe
Copy link
Member

Copied this from Discord:

The theory is easy. Unfortunately there are certain builders that will choke on modules
The most notorious is Launchpad PPAs, because they don't have Go 1.13 available as official packages
And there's also no 3rd party PPA with updated Go for all Ubuntu releases (there was one, but was discontinued due to some build issues on PPA, ironic)
I've tried 2 solutions already, one on Friday (download Go 1.13 during PPA build) and one on Monday (upload binaries directly) both failed (chroot with net disabled, PPA must build it itself)
As such now we need to set up an infra to build the packages via Snaps first, and then hack in an empty-package dependency on PPA
Until that's done, we can't enable modules as they would break the binary distributions

@tzdybal
Copy link
Author

tzdybal commented Sep 17, 2019

If modules cannot be used at all, I assume only go 1.10 (or prior) is available (which is rather old and therefore should be updated anyways).

It seems that longsleep/golang-backports PPA provides up-to-date packages for all currently supported Ubuntu distributions (see Ubuntu release page).

Is it really necessary to build packages for unsupported versions of Ubuntu?

@karalabe karalabe modified the milestones: 1.9.4, 1.9.5 Sep 19, 2019
@fjl fjl modified the milestones: 1.9.5, 1.9.6 Sep 20, 2019
@fjl fjl modified the milestones: 1.9.6, 1.9.7 Oct 2, 2019
@zjiekai zjiekai mentioned this pull request Oct 13, 2019
@karalabe karalabe modified the milestones: 1.9.7, 1.9.8 Nov 8, 2019
@karalabe
Copy link
Member

Closing this in favor of #20311

  • Still works with Go 1.11 and 1.12, which we cannot drop due to Ubuntu Trusty support.
  • Fixes the issue of testing the bind package with the correct modules (without generating temp files within the live source tree).
  • Implements Go module support for PPA builds in Launchpad.
  • Ensures build work through the xgo cross compiler for MIPS.
  • Generally just focuses on go module support and not on linter fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants