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

28 add arm support #45

Merged
merged 28 commits into from
Mar 19, 2024
Merged

28 add arm support #45

merged 28 commits into from
Mar 19, 2024

Conversation

nuvious
Copy link
Contributor

@nuvious nuvious commented Mar 16, 2024

Added support to build specifically amd64, armv6, armv7, and arm64v8 images.

CICD tested using my own dockerhub

Build logs in github

image
https://hub.docker.com/r/nuvious/proxpi/tags

Arm container tested on my Kubernetes cluster comprising of 11 raspberry pi 4's

image

Issue: #26 (My branch is called issue-28 in error)

Dockerfile Outdated
@@ -2,7 +2,7 @@ FROM python:3.12-alpine

RUN --mount=source=.,target=/root/src/proxpi,rw \
uname -a && cat /etc/issue && apk --version && python --version && pip --version \
&& apk --no-cache add git \
&& apk --no-cache add git build-base libxslt-dev libxml2-dev \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had build failures related to the lxml library which expects gcc and these libraries to be available to build. They likely don't build arm wheels so lxml needs to build from source.

@nuvious
Copy link
Contributor Author

nuvious commented Mar 16, 2024

Performing one more test on my cluster with a reduced image size. I forgot to purge un-needed build packages at the end which increased the image size significantly.

@nuvious
Copy link
Contributor Author

nuvious commented Mar 16, 2024

CICD Tested with corrected apk purge command:

https://github.com/nuvious/proxpi/actions/runs/8304191331/job/22729496210

Did a manual build/test on my raspberry pi as well to confirm the docker image test works on arm; github can only do that test on amd64 architecture:

image

Also corrected usage of apk to purge build-base and the libraries I imported to reduce the image size appropriately:

image

Also re-tested in kubernetes on a pi cluster:

image

@EpicWink
Copy link
Owner

Thanks for the PR. The issue is the same as with #24 and #27, which is that I can't test the ARM image on every release (I don't own an ARM device, I don't have the time to set up ARM emulation, and most importantly, GitHub doesn't have ARM CI runners).

@benedikt-bartscher
Copy link

@EpicWink I could set up repository mirroring to my companys gitlab instance and grant maintainer access for you if that helps (for free). We have many x86 and arm runners with a dedicated buildkit cluster.

Thanks for maintaining proxpi!

@nuvious
Copy link
Contributor Author

nuvious commented Mar 18, 2024

I have an idea that should help you out then. Will get something working on my fork and update this PR accordingly.

@nuvious
Copy link
Contributor Author

nuvious commented Mar 18, 2024

Did some more modifications that run the unit and integration tests in the containers via qemu so you can utilize gtihub x86 runners to do arm testing. I removed the armv6 and armv7 configurations since they were not a part of the original request and because they add significant build time due to emulation speeds. They could be added back under a different issue later potentially.

Also made the job definitions as modular as possible. This means the arm64 and amd64 containers are built and tested on independent runners in parallel but the runner that builds and pushes the final docker image layers has to re-build the process. I tried some tricks to share the docker cache between jobs but nothing was saving time that I tried. This could be modified to run everything under one runner but would sacrifice modularity since the docker commands would require the --platform flag to be added and copied multiple times (or a script written for it).

Given the build only takes about 4 minutes in total that seemed sufficient enough for the task at hand and the modularity makes adding other architectures down the line a lot easier.

Completed job:
image
https://github.com/nuvious/proxpi/actions/runs/8333123237

Look in the build logs for aarch64 for confirmation everything was running under aarch64/arm64. I added uname -a to the unit test execution to make this visible:
image

Images:
image

Let me know if you have further questions, direction or guidance on this effort and thanks for your response!

@nuvious
Copy link
Contributor Author

nuvious commented Mar 18, 2024

P.S. Was able to remove my modifications to the Dockerfile since the lxml library has arm64 support and the scope no longer includes armv6 or armv7. Build log for that test under my 'dev' tag:

https://github.com/nuvious/proxpi/actions/runs/8333385815

@codecov-commenter
Copy link

codecov-commenter commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.45%. Comparing base (57a23e6) to head (02bdb3e).

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #45   +/-   ##
=======================================
  Coverage   89.45%   89.45%           
=======================================
  Files           3        3           
  Lines         673      673           
=======================================
  Hits          602      602           
  Misses         71       71           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@EpicWink EpicWink merged commit c276601 into EpicWink:master Mar 19, 2024
11 checks passed
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.

4 participants