-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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 CI job to cross compile all the things #5262
Conversation
539088e
to
508cbdd
Compare
This currently breaks armhf builds. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
508cbdd
to
587b913
Compare
545e0c7
to
27cbd58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just some questions.
goarch: arm64 | ||
- goos: linux | ||
goarch: arm | ||
goarm: "7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is 7 the lowest version of 32-bit Arm we want to support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add more and maybe worthwhile since I know folks use v5 and v6 with Docker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll also add windows+armv7 but this is known broken right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexellis might have opinion on how "low" to go with arm 32-bit based on his connection to that community
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest 6 is worth adding (that's Pi1 and Pi0, which are fairly popular), but even I'd consider 5 to be "best effort" (and not worth the CPU time to test explicitly).
(my 2¢)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, though cgo means armv6 is not something you'll be able to easily cross-compile from an Ubuntu environment unless you build/download a cross toolchain from elsewhere, so the most you could do is also add 5, which should cover the bases (if the 7 build works and a 5 build works, the odds of 6 having problems are really low -- they'd be toolchain mistakes, not code problems, at that point, like using the wrong C compiler 😄).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And got the windows/arm/v7 build fixed as well (see go-winio update), so added that in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also added linux/arm/v5, guess I didn't comment on that.
27cbd58
to
60a3a9c
Compare
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
This makes sure we can compile on all the platforms and prevent things like integer overflows. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
60a3a9c
to
311e326
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is very similar to what's happening in nightly build script. Should we consolidate them to maintain the code in one place? |
Maybe off-topic here, but I'd also like to see arm64 binaries to be available on https://github.com/containerd/containerd/releases |
@AkihiroSuda +1 and that has been requested since 2018 :) See #2901. |
This makes sure we can compile on all the platforms and prevent things
like integer overflows.