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

Add arm64 and ppc64le as new platforms #241

Merged
merged 4 commits into from
Aug 13, 2016

Conversation

luxas
Copy link
Contributor

@luxas luxas commented Jun 8, 2016

Made it possible to build for other arches using GOARCH=something ./build
Made the CI spot compilation errors
Updated the golang.org/x/sys/unix/ dependency
travis.yml now has the same layout as etcd: etcd-io/etcd#5450

@steveej

@dcbw
Copy link
Member

dcbw commented Jun 10, 2016

I'm probably ignorant of the travis operation here, but is the point of the test -> build change for !amd64 because CNI doesn't currently pass tests on !amd64? Why wouldn't we want to run the tests for arm/ppc?

@luxas
Copy link
Contributor Author

luxas commented Jun 10, 2016

It isn't possible to run arm, arm64 or ppc64le go binaries on an amd64 host, unfortunately, so we can't run the tests unless we're on a native machine

But catching compilation errors is a good start

@dcbw
Copy link
Member

dcbw commented Jun 10, 2016

Ah, OK. I guess there isn't any non-amd64 arch support in travis...

@steveej
Copy link
Member

steveej commented Jun 17, 2016

Please split out the travis change from the vendoring bump. We'll deal with the latter first to see if that causes the tests to fail. Thanks!

@luxas luxas force-pushed the add_platforms branch 2 times, most recently from 0acbe8f to 0ec60ec Compare June 19, 2016 14:49
@luxas
Copy link
Contributor Author

luxas commented Jun 19, 2016

Tests passing, the only problem was that I replaced GOBIN because it can't be used when cross-compiling, but the test script used the variable. I fixed that now.

Also, I added the arches to your releasing script. I haven't tested it, so please give feedback on it.
It would be really awesome if you may upload the cross-compiled binaries at the same time you're uploading the "normal" amd64 ones. Everything can be done from an amd64 host, so it shouldn't be any problems with it.

We're gonna use cni now in docker-multinode, and it would help if binaries for all arches are easily available.

@dcbw
Copy link
Member

dcbw commented Jun 23, 2016

Seems OK to me, but somebody who does actual releases should take a look too.

@luxas
Copy link
Contributor Author

luxas commented Jun 26, 2016

@steveej Please take a look. If you want me to split the releasing code out from this patch, I'll do that and send a follow-up.
For us, it would be great to have released binaries for all arches already in v0.4.0, and that shouldn't require more code than this.

@luxas
Copy link
Contributor Author

luxas commented Jun 30, 2016

Should I split it up now, so you may merge the code that makes it possible to compile for arm64 and ppc64le, and then we'll deal with the releasing later on?

@steveej
Copy link
Member

steveej commented Jul 1, 2016

@luxas No need to split out the release script change IMO. I'll try the release script and let you know if it works out.

On another note, have you tried running the cross-compiled binaries using qemu-user? I'm not sure if the netlink syscalls are supported but it'd be very useful for CI.

@steveej
Copy link
Member

steveej commented Jul 1, 2016

Please adjust the commit message(s) to conform to https://github.com/containernetworking/cni/blob/master/CONTRIBUTING.md#format-of-the-commit-message, thanks a lot!

@luxas
Copy link
Contributor Author

luxas commented Jul 3, 2016

@steveej Updated

qemu-user is known to be bad with go and multithreading.
at version 2.5, it doesn't work at all for arm64
qemu-user for arm and ppc64le can run the go (1.5.2+) hello world program, but haven't tested much else

Please take this PR into the v0.4 release, that would help Kubernetes multiarch
Thanks :)

@bboreham
Copy link
Contributor

The change from go install to go build means that dependent packages are no longer installed by the build; they are built in a temporary directory and then thrown away.

If you didn't mean to have that effect, go build -i is a closer match.

@luxas
Copy link
Contributor Author

luxas commented Jul 13, 2016

@bboreham Yes, agree. That was unintentional, but by utilizing go build, it's possible to specify output path, that's why I changed it.

But is it possible to merge this now (@steveej)?
I can make a small follow up for that after this PR is merged (adding -i is not critical)

@luxas
Copy link
Contributor Author

luxas commented Jul 20, 2016

ping @steveej

@luxas
Copy link
Contributor Author

luxas commented Jul 30, 2016

Are there any blockers for this PR or haven't you just had time for it...?
This would be useful for Kubernetes.

Side question: When is v0.4.0 going to be released?

cc @dcbw @rosenhouse

@tomdee
Copy link
Member

tomdee commented Aug 12, 2016

I'd be happy to merge this - just needs a rebase

The current vendor of sys/unix is really old, and doesn't work on arm64 and ppc64le
Updating to the latest version might also fix other issues

ref containernetworking#209
… architectures

This makes it possible to cross-compile cni like so:
$ GOARCH=arm ./build
$ GOARCH=arm64 ./build
$ GOARCH=ppc64le ./build

ref containernetworking#209
Cross-compile cni for arm, arm64 and ppc64le with go1.6 only
Allow go tip to fail
Set fast_finish to true, which means travis will instantly return build failure when any of the required builds fail

ref containernetworking#209
Modify the releasing script to cross-compile for the new architectures, but also keep backwards-compability

ref containernetworking#209
@luxas
Copy link
Contributor Author

luxas commented Aug 13, 2016

@tomdee Rebased. PTAL and merge.
When will v0.4.0 be out btw?

@rosenhouse rosenhouse merged commit 9d5e6e6 into containernetworking:master Aug 13, 2016
@tomdee tomdee mentioned this pull request Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants