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

Godeps, vendor: switch to Go 1.6 vendored dependencies #2512

Closed
wants to merge 1 commit into from

Conversation

karalabe
Copy link
Member

@karalabe karalabe commented May 3, 2016

This PR finally moves all of our dependencies to the vendor folder. This should enable our repository to be go gettable, and most importantly, reusable as a library in other Go projects without needing to jump though insane dependency management hoops. Yay! 🎉

Note: with the introduction of the vendor folder we can no longer do go install ./..., as that would install everything from vendor too, most of which are junk which we don't vendor nor use. This is a known wontfix issue. I've updated the Makefile accordingly to skip the vendor folder when building and only use it for the dependencies, not as main codebase.

@robotally
Copy link

robotally commented May 3, 2016

Vote Count Reviewers
👍 0
👎 0

Updated: Tue Jul 12 09:39:21 UTC 2016

@karalabe karalabe added this to the 1.5.0 milestone May 3, 2016
@karalabe karalabe force-pushed the go-vendored-deps branch 4 times, most recently from ffac425 to 7711013 Compare May 3, 2016 17:03
@karalabe karalabe force-pushed the go-vendored-deps branch from 7711013 to 46c6e5e Compare May 3, 2016 17:10
@karalabe
Copy link
Member Author

karalabe commented May 3, 2016

@fjl Please review this PR, especially my changes to the makefile and build env files.

The CI servers need to be updated to Go 1.6, otherwise they won't pass. @caktux Could you please update all CI servers to Go 1.6.2?

@@ -152,7 +152,7 @@ func (n *Node) Start() error {
}
// Otherwise copy and specialize the P2P configuration
running := new(p2p.Server)
*running = *n.serverConfig
reflect.ValueOf(running).Elem().Set(reflect.ValueOf(n.serverConfig).Elem()) // Fancy *running = *n.serverConfig to prevent mutex copy
Copy link
Contributor

Choose a reason for hiding this comment

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

??

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the intention to fix the go vet issue but this is not a good fix for it.
We need to find another way. I can remove the mutex as a quick workaround.

@fjl
Copy link
Contributor

fjl commented May 3, 2016

as that would install everything from vendor too, most of which are junk which we don't vendor nor use.

That's weird. Why do we vendor 'junk' that is not used? If godep leaves unused dependencies in the
directory we should switch to a different tool instead.

I don't mind that go install ./... builds everything. It will build everything anyway.
godep removes all _test.go files while vendoring so there should be no tests to run in vendor/.

@karalabe
Copy link
Member Author

karalabe commented May 3, 2016

Well, for starters, the windows pipe package isn't build constrained to windows only, so every other platform will fail to build ./.... Only the windows RPC files reference it, so it is only touched on Windows by our code, but ./... doesn't know that.

@fjl
Copy link
Contributor

fjl commented May 4, 2016

That sounds like an issue we can fix though.
I am arguing to remove the (go list) workaround because
it highlights such issues.

@karalabe
Copy link
Member Author

karalabe commented May 4, 2016

I understand your point, but do tell me how you think we can solve it? I don't think doing manual patches to upstream dependencies is such a good idea.

@karalabe
Copy link
Member Author

karalabe commented May 4, 2016

Another example that breaks is @Gustav-Simonsson 's opencl dependency, since even though ethash has a build tag/constrain to only use it if requested, when doing ./... we inherently build it, which fails if you don't have the OpenCL develop libs available.

@caktux
Copy link
Contributor

caktux commented May 4, 2016

This affects more than just the golang version... Build server steps, Homebrew formula and deb packaging scripts will all need to be changed to support this, and it also means we'll have to yet again split the build procedures for master and develop like the good ol' PoC days. So this will need a little more planning besides just getting the builds to pass again.

@fjl
Copy link
Contributor

fjl commented May 4, 2016

@caktux Thanks. We are aware that many changes are needed to make this work.

The way to build reliably is to run make all. It behaves identically on both master and develop. I have updated the Homebrew formula recently to use it. Can we update the buildbot steps, deb scripts to use make too?

@caktux
Copy link
Contributor

caktux commented May 4, 2016

What about Windows? I think I've got the others ready at least for the buildslaves with 1.6 and the build steps with make all.

@fjl
Copy link
Contributor

fjl commented May 5, 2016

It's true that we can't build with make on Windows.

I'd say the easiest way would be to add a .bat script to go-ethereum that does the build on both master and develop. Compared to the make build it would still require setting up GOPATH and cloning to the right place but that's fine. We only need it so buildbot can run simple steps that we can tweak through the script. I'll look into adding it.

@caktux
Copy link
Contributor

caktux commented May 6, 2016

@karalabe make all is having issues with the ARM builds, something about GOBIN being set in those logs.. since you're the cross-compilation expert, could you give a hand with that builder? Linux and OSX seem to be doing great.

So besides ARM and Windows, all that should be left is the deb packaging (the rules file more specifically).

@tgerring Could you update golang to 1.6 on the Windows buildslave?

@karalabe
Copy link
Member Author

karalabe commented May 6, 2016

@caktux I've souped up our makefile in this PR #2520 to handle the cross build issue of Go, but I hit a gcc issue in your arm images (arm-linux-gnueabi-gcc-5: error: unrecognized command line option '-m64'): https://builds.ethereum.org/builders/ARM%20Go%20pull%20requests/builds/2501/steps/go-test/logs/stdio . You probably need to sort this out on your side.

@caktux
Copy link
Contributor

caktux commented May 6, 2016

Fixed, wasn't passing the env, but then we obviously can't test cross-compiled binaries either so just removed the test step as it was before. Once #2520 is merged it might be good to rebase this one.

@fjl
Copy link
Contributor

fjl commented May 10, 2016

@caktux Windows scripts are present on both master and develop now. Any news on the PPA?

@VoR0220
Copy link
Member

VoR0220 commented May 16, 2016

👍

@fjl
Copy link
Contributor

fjl commented Jul 12, 2016

We'll redo this change in the new build/ci.go script when Go 1.7 is out and available for most distros.

@fjl fjl closed this Jul 12, 2016
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