-
Notifications
You must be signed in to change notification settings - Fork 61
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 ARM builds #152
Add ARM builds #152
Conversation
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.
You are on the right track but some changes and additional modifications are needed. I have left comments describing what's necessary to do.
{ref_name, ref} <- Bob.GitHub.diff(@repo, "builds/otp/#{linux}"), | ||
build_ref?(linux, ref_name), | ||
do: Bob.Queue.add(Bob.Job.BuildOTP, [ref_name, ref, linux]) | ||
do: Bob.Queue.add(Bob.Job.BuildOTP, [ref_name, ref, linux, arch]) |
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 need to verify what OTP versions we can build on, I am guessing older versions may not build on arm64, but not sure.
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 arch needs to be added like this so we can fetch the jobs from machines with different architectures.
do: Bob.Queue.add(Bob.Job.BuildOTP, [ref_name, ref, linux, arch]) | |
do: Bob.Queue.add({Bob.Job.BuildOTP, arch}, [ref_name, ref, linux]) |
|
||
def run(_type) do | ||
for linux <- @linuxes, | ||
arch <- @arches, | ||
{ref_name, ref} <- Bob.GitHub.diff(@repo, "builds/otp/#{linux}"), |
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.
This diff won't work since it currently only diffs the existing amd64 builds. We need to track the architectures separately in different files https://github.com/hexpm/bob/blob/main/lib/bob/repo.ex#L5-L10.
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 suggest using the file path builds/otp/#{arch}/#{linux}"
going forward, before the deploy we will move the files in the bucket and we will do a path rewrite on the CDN to ensure the existing paths without arch doesn't break.
|
||
if [[ "$arch" == "arm64" ]]; then | ||
container="otp-build-${linux}-${ref_name}-arm" | ||
platform="--platform linux/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.
This flag and buildx is not necessary. We will run the builds on machines with different architectures so the --platform is not necessary.
@@ -5,12 +5,23 @@ set -euox pipefail | |||
ref_name=$1 | |||
ref=$2 | |||
linux=$3 | |||
arch=$4 |
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.
More changes is needed in this file. As it is now the amd64 and arm64 will upload to the same place and clobber each other. The arm64 need to be uploaded to a different directory.
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.
Let's use the builds/otp/#{arch}/#{linux}"
paths as suggested above #152 (comment).
3a7bf17
to
bc0c95a
Compare
Closing in favor of #174. |
@ericmj Followed your guidance in this issue. Curious if I'm even close here?
Any more guidance to get Erlang ARM builds in Bob would be appreciated.
Thanks,
Anthony