-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Cross compile for Linux, MacOS and Windows on CI #2665
Conversation
.github/workflows/ci.yml
Outdated
@@ -48,10 +49,15 @@ jobs: | |||
--disable-maintainer-mode \ | |||
--disable-valgrind \ | |||
--with-oniguruma=builtin \ | |||
${{ matrix.configure_flag }} | |||
--host=${{ matrix.arch }}-linux-gnu \ |
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 started looking into doing cross builds yesterday but ended up reading old autoconf documentation about host, target and build and got irritated 😄 what a mess. Also a bit confused why jq's configure script only has --build
and --host
... and --host
is what i would call "target"? :) it's generated by autoconf 2.71 which is quite modern.
Anyways thanks doing this!
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.
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 added cross-compilation for Windows as well. However, I noticed a few things:
- Tests with the
mingw-w64-x86_64
build failed for segmentation fault: (Maybe the same issue as the clang issue). I fallback to using msys gcc for x86_64 build - For Windows i386 build, I needed to add the
__MINGW32__
guard toinclude <pthread.h>
instead of the usingpthread_*
functions defined injv.c
&jv_thread.h
due to "multiple definition" of the same functions (Ref).
make | ||
strip jq | ||
${ARCH}-linux-gnu-strip jq | ||
file ./jq |
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.
Accidentally left in? i think it could make sense to keep
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.
It's debugging, but it doesn't hurt.
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.
The file
line is to ensure the cross-compiled binary is what we expect. I guess it could be removed but it doesn't hurt to have that visibility.
A reason to wait for the docker job it make sure we notice if it fails |
Should we try to not to mix arm64/aarch64 and x86_64/amd64? just use one? |
ARM build for windows could be interesting but maybe for later? |
Sorry now i noticed this is already done in release step 👍 |
.github/workflows/ci.yml
Outdated
@@ -258,7 +269,7 @@ jobs: | |||
runs-on: ubuntu-latest | |||
permissions: | |||
contents: write | |||
needs: [linux, macos, windows, dist, docker] | |||
needs: [linux, macos, windows, dist] |
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 should abort releasing if docker build/publish fails. Even if this change releases the artifact quickly, this isn't good.
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.
To clarify, the release
job creates a draft release instead of releasing it. However, I understand you want to wait for it before creating the draft release. I put it back, provided that you will improve the docker job speed by adopting @userdocs's suggestions.
So I was looking at this closed pr #2652 and it seems apparent the obvious issue was that you are building via emulation, instead of cross compiling and then adding the prebuilt binary to the respective target arch, which would dramatically decrease the time to completion for that job. There is no real good reason to emulate the entire process and not on Github, you'd need to use something like https://buildjet.com/for-github-actions to make it faster. Debian has these toolchains available https://packages.debian.org/source/bookworm/build-essential and when you use that to cross build you can do something like this perhaps?
https://github.com/userdocs/jq-cross-debian/actions/runs/5487472986 to publish binaries as release assets https://github.com/userdocs/jq-cross-debian/releases/tag/master-6944d81 And then just copy those to a docker image which will provide the same outcome at in a fraction of the time. So now you have a full suite of cross built target arch static binaries as release assets people can download and then a very simple docker build process that takes simple copies the binary from the release asset or artifacts of the job. In my example CI you can see the file outputs per build https://github.com/userdocs/jq-cross-debian/actions/runs/5487472986/jobs/9998971987 For example, amd64
s390x
Personally i'd do musl static but since you are already using a debian based solution and the binary appears to be be fullly static with no glibc dependency using crossbuild-essential works well, i think. |
@userdocs Thank you. I'll improve the docker job to use the pre-built executables. EDIT: I have fixed the docker job and it runs within 3 minutes to build the multi-platform image. What a big improvement. |
.github/workflows/ci.yml
Outdated
cp artifacts/jq-linux-ubuntu-22.04-gcc-x86_64/jq release/jq-linux-amd64 | ||
cp artifacts/jq-linux-ubuntu-22.04-gcc-aarch64/jq release/jq-linux-arm64 | ||
cp artifacts/jq-macos-macos-13-clang-x86_64/jq release/jq-macos-amd64 | ||
cp artifacts/jq-macos-macos-13-clang-arm64/jq release/jq-macos-arm64 |
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 can mv instead of cp here.
Only `gcc` is supported well for msys2.
The CI release job doesn't need anything from the Docker job
MinGW includes pthread related functions. Ref: https://github.com/owenthereal/jq/actions/runs/5496335434/jobs/10016293713#step:4:1237
Tests failed for mingw64 build
`LDFLAGS=-s` already strip symbols
Thanks @userdocs for the awesome example of cross-compiling on Linux with crossbuild-essential! I adopted your approach for the Linux build in this PR. I didn't change the Besides, I added cross-compliation for Windows. Now we have complete coverage of builds for most platforms 😄. The Last but not least, we wouldn't need to explicitly This PR is ready to be reviewed again. |
@owenthereal glad it helped. I had used LDFLAGS since the configure appears to look for a non existent strip binaries and never find it, perhaps meaning the configure needs to be overhauled at some point. With the linker flags, the gcc doing the cross compiling will strip it using strip binary it comes with, so made sense as way around that. and also for @itchyny, As for docker, after my post, I attempted to add a matrix multi platform (which would further increase speed) job but it's not quite working. Following this https://docs.docker.com/build/ci/github-actions/multi-platform/ it seems it need to merge the manifests in a following job. I think a combination of the prebuilt binaries + matrixed docker multi platform job would be the fastest it could be. But the matrix part of the docker/buildx job seems to be adding complexity and not sure what the actual benefit of that is yet. |
@owenthereal also, since you are using ubuntu they provide a riscv64 crossbuild that Debian does not. https://packages.ubuntu.com/jammy/crossbuild-essential-riscv64 Since i used Debian i did not provide it but if you add it for ubuntu you used their full suite. https://packages.ubuntu.com/search?suite=jammy&searchon=names&keywords=crossbuild-essential
|
Added riscv64 to the list. And build passed 😄 |
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.
Thank you!
Close #2386? |
This PR includes the following changes:
ubuntu-20.04
build becausegcc
compile target on MacOS because it symlinksgcc
toclang
gcc
is supportedrelease
job by not waiting ondocker
job because it doesn't require anything from thedocker
jobThis closes #2618 #2386
cc: @wader @itchyny