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

Switch the nightly build to producing a multiarch docker image that supports both x86_64 and arm64 #133

Merged
merged 4 commits into from
Feb 27, 2024

Conversation

eranrund
Copy link
Collaborator

@eranrund eranrund commented Feb 26, 2024

Hi!

This PR reworks how the Docker image is built in a way that allows building a multiarch image that supports both x86_64 and arm64. I based it on #104 but the changes are pretty significant due to the need to use the docker buildx flow, QEMU, etc. I based it off the example in https://docs.docker.com/build/ci/github-actions/multi-platform/.

I hope you'll be open to this approach. If so, I'll be happy to convert the stable CI job as well. this now includes the stable build as well since it was needed for my use-case.

Happy to answer any questions about why I did things a certain way in case it is not obvious.

Thank you :)

test.sh Show resolved Hide resolved
Copy link
Owner

@clux clux left a comment

Choose a reason for hiding this comment

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

thanks a lot for doing this! i am definitely open to this approach! the relatively small amount of changes herein are definitely small enough that it is maintainable, and the extensions look clean.

CI is red atm, but happy to press buttons whenever. Have only suggested minor things so far from a quick scan.

Dockerfile.arm64 Show resolved Hide resolved
Comment on lines +77 to +81
RUN mkdir $PREFIX && \
echo "$PREFIX/lib" >> /etc/ld-musl-aarch64.path && \
ln -s /usr/include/aarch64-linux-gnu/asm /usr/include/aarch64-linux-musl/asm && \
ln -s /usr/include/asm-generic /usr/include/aarch64-linux-musl/asm-generic && \
ln -s /usr/include/linux /usr/include/aarch64-linux-musl/linux
Copy link
Owner

Choose a reason for hiding this comment

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

this seems to be the most changed bit! apart from a few aarch references this is remarkably similar!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it might be possible to merge the two files in the future. I opted not to try just to make things easier to review.
Similarly, it should be possible to merge the nightly and stable actions by using a reusable action and just passing different parameters.

test.sh Show resolved Hide resolved
.github/workflows/stable.yml Show resolved Hide resolved
@clux clux linked an issue Feb 26, 2024 that may be closed by this pull request
@eranrund
Copy link
Collaborator Author

thanks a lot for doing this! i am definitely open to this approach! the relatively small amount of changes herein are definitely small enough that it is maintainable, and the extensions look clean.

CI is red atm, but happy to press buttons whenever. Have only suggested minor things so far from a quick scan.

You're welcome! I'm happy you're open to accepting it :)
Just committed your suggestions. We do need to get the docker login to work for the buildx stuff to work, and I imagine the dockerhub secret isn't being passed to CI checks coming from a fork (which is reasonable since you don't want random people having access to it).
You might have to push a copy of this branch into this repo yourself, and then create a PR for it to get the dockerhub token secret thing working...

@clux
Copy link
Owner

clux commented Feb 26, 2024

Ah, I didn't realise the CI stuff was already tested elsewhere! Figured there had to be a few cycles.

I'll give it a go!

@eranrund
Copy link
Collaborator Author

Ah, I didn't realise the CI stuff was already tested elsewhere! Figured there had to be a few cycles.

I'll give it a go!

Oh, yes, this took many iterations to nail down :)

@eranrund
Copy link
Collaborator Author

I just realized that we'll need some hack to force the stable build to run again since otherwise CI will just skip it since there hasn't been a new Rust release since the last time it ran. So probably need to comment out that check, let it run that way, and then undo that.

@clux
Copy link
Owner

clux commented Feb 26, 2024

Oh, yes, this took many iterations to nail down :)

🙈 i feel your pain. This must have been quite a slog with how slow the arm build is.

@clux
Copy link
Owner

clux commented Feb 26, 2024

I just realized that we'll need some hack to force the stable build to run again since otherwise CI will just skip it since there hasn't been a new Rust release since the last time it ran.

yeah, that's an easy one to fix by the end. am cleaning up a few things in the justfile and test.sh for quick local testing first. also want to do some sanity on mac tomorrow.

..wondering also if this is a good time to do some other breaking changes i've been wanting to do like fixing the workspace dir and maybe ripping out pq. could do a lot of this under a new tagging scheme.

@clux
Copy link
Owner

clux commented Feb 26, 2024

under a new tagging scheme

ah, i suppose the tags are already overwritten. this worked immediately!

have pushed some small test stuff and will do some sanity tests tomorrow on a mac. once that is done, feel free to cherry pick my changes back into your branch and we can probably merge.

@eranrund
Copy link
Collaborator Author

maybe ripping out pq

Please don't! I am relying on it (and may not be the only one)...

@clux clux mentioned this pull request Feb 26, 2024
@eranrund
Copy link
Collaborator Author

have pushed some small test stuff and will do some sanity tests tomorrow on a mac. once that is done, feel free to cherry pick my changes back into your branch and we can probably merge.

Amazing, thank you. Please let me know once you're done with your testing and I'll cherry pick

@clux
Copy link
Owner

clux commented Feb 26, 2024

Please don't! I am relying on [pq] (and may not be the only one)...

ok, i'll leave it, but be aware that it's not in the greatest state atm.

@eranrund
Copy link
Collaborator Author

Please don't! I am relying on [pq] (and may not be the only one)...

ok, i'll leave it, but be aware that it's not in the greatest state atm.

Interesting, good to know. Just saw you mentioning that blackdex has a newer version so would be nice to see if that can work here as well

Copy link
Owner

@clux clux left a comment

Choose a reason for hiding this comment

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

quick questions

ENV PATH=/root/.cargo/bin:$PREFIX/bin:$PATH \
RUSTUP_HOME=/root/.rustup \
CARGO_BUILD_TARGET=aarch64-unknown-linux-musl \
CARGO_TARGET_AARCH64_UNKNOWN_LINUX_MUSL_RUSTFLAGS="-Clink-self-contained=yes -Clinker=rust-lld -Ctarget-feature=+crt-static" \
Copy link
Owner

Choose a reason for hiding this comment

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

If we were to smash these dockerfiles together, having both CARGO_TARGET_X evars here for different arches should be fine, right? this is one of the few big changes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think so, but we will have to verify.

# TODO: fix so that it works
RUN curl -sSL https://www.openssl.org/source/openssl-$SSL_VER.tar.gz | tar xz && \
cd openssl-$SSL_VER && \
CFLAGS="-mno-outline-atomics" ./Configure no-zlib no-shared -fPIC --prefix=$PREFIX --openssldir=$PREFIX/ssl linux-aarch64 && \
Copy link
Owner

Choose a reason for hiding this comment

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

comparing amd64 and arm64:

-    CFLAGS="-mno-outline-atomics" ./Configure no-zlib no-shared -fPIC --prefix=$PREFIX --openssldir=$PREFIX/ssl linux-aarch64 && \
+    ./Configure no-zlib no-shared -fPIC --prefix=$PREFIX --openssldir=$PREFIX/ssl linux-x86_64 && \

this is the other big change, do you remember what was the deal with this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I recall correctly, without CFLAGS="-mno-outline-atomics", I got a bunch of linker errors for some missing symbols.

Copy link
Owner

Choose a reason for hiding this comment

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

hehe ok. sounds like a standard wtf style google session then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup...

@clux
Copy link
Owner

clux commented Feb 27, 2024

Sanity done in #134, feel free to cherry-pick my changes and i'll merge it in :-)

@eranrund
Copy link
Collaborator Author

Hi @clux , just pushed the cherry-picked changes. Thanks again for moving fast on this!

@clux clux merged commit 24af91b into clux:main Feb 27, 2024
2 of 6 checks passed
@clux
Copy link
Owner

clux commented Feb 27, 2024

Awesome. Thank you so much.

This was one of those big projects that I always struggled to understand how to do it, and you've done it with very elegantly.
Will look towards maybe combining the two dockerfiles later, and moving on some kube stuff to produce multi-arch images as a result!

Super helpful. Thank you again.

@eranrund
Copy link
Collaborator Author

Awesome. Thank you so much.

This was one of those big projects that I always struggled to understand how to do it, and you've done it with very elegantly. Will look towards maybe combining the two dockerfiles later, and moving on some kube stuff to produce multi-arch images as a result!

Super helpful. Thank you again.

My pleasure! Thanks for maintaining this!

Could you please force a release of a new stable image so we can get the multiarch published to dockerhub?

@clux
Copy link
Owner

clux commented Feb 27, 2024

Could you please force a release of a new stable image so we can get the multiarch published to dockerhub?

sure thing, it's building from this as we speak

My pleasure! Thanks for maintaining this!

work is easy when people do all the work for me :-)
on that note, i did also send you collab access if you should ever need it, but don't feel pressured to take it / use it

@eranrund
Copy link
Collaborator Author

sure thing, it's building from this as we speak

It built but the merge step was skipped :(

@eranrund
Copy link
Collaborator Author

Oops, we forgot to remove

@clux
Copy link
Owner

clux commented Feb 27, 2024

yea 😞

running a faster build now without tests just to get something out the door

@clux
Copy link
Owner

clux commented Feb 27, 2024

ok, we have a stable build 🎉

@clux clux mentioned this pull request Feb 27, 2024
@eranrund
Copy link
Collaborator Author

beautiful, thank you! just used it to build a project on my Apple Silicon Mac :)

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.

explore building an arm64 variant
2 participants