-
Notifications
You must be signed in to change notification settings - Fork 315
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 support for arm64 build (tested on Mac M1) #573
Conversation
Thanks for this PR, Could you also make one for V14? |
Please rebase and solve conflicts, and do the pre-commit fixes in the same commit that introduces the error. |
PR rebased and I also added arm builds for Odoo version 14, 15 and 17. I have build tested all of them locally on Mac M1 |
Thanks for it. Seems OK. The only point I don't know right now is where to indicate that the pushed build is for both platforms. Do you know it? |
I think we need to adjust the ./hooks/build script or use the available github actions but either way it seems docker buildx is required for multi-platform builds: https://docs.docker.com/build/ci/github-actions/multi-platform. I will make another branch on my fork and link it to my docker hub then do some tests there and later propose those changes to this PR as well. In the meantime people can use this PR to build the *-onbuild image locally. |
Would like to functional test this, I have an arm server available. How can this be tested locally? 😊 👼
|
@bosd I am not sure what is your setup but here are a few steps on how to do this, if you need more details I think it's better to open a github discussion rather than continuing on this PR.
One last thing to note and this will be probably a PR on other repositories, for the development environment some support images are missing arm builds as well. |
@PCatinean Thanks!!
Indeed on the dev env I got an error |
@bosd happy to hear it worked. Yes I will have to take a look at the other images as well, for the wkhtmltopdf version it is a build argument so you can just do --build-arg WKTHTMLTOPDF_VERSION=version and it will use that no need to change the original file |
Yeah, We're getting there to arm support..
Thanks, that will likely work. Well from usability point of view, personally I would prefer to set it by default to the arm supported version instead of throwing an error. |
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
Any news about how to publish this for merging it? |
Not sure what you mean by this. |
It's not for you, @bosd. I mean this comment: #573 (comment) |
ping @PCatinean Can you attend to pedro's question? |
@pedrobaeza here are the latest changes that add for support of multi-arch builds. The following adjustments have been made:
The arm64 builds are a bit slow around this step Tests have been made locally and pushed to: https://hub.docker.com/repository/docker/pledra/doodba/tags?page=1&ordering=last_updated. The architectures available are shown there and when doing a regular pull it should reconcile with the architecture of the system automatically. If there are any other tests I can make or info to share please let me know. |
Hi @pedrobaeza so the PR is finally ready. After a few tests I discovered that the execution time of both test and build (now with 2 platforms) in one single job hit a timeout or resource limit of the github runner. I have now separated the I tested locally on my fork with the same github variable names, the only difference is the github repo name "pledra/doodba" instead of "tecnativa/doodba". PR: https://github.com/pledra/doodba/pull/1 On the PR the test and build ran but only when merged to master did the push also get activated. Hope this work for you :) |
@pedrobaeza Is this one ok now? Ready for merge 🙏 |
@pedrobaeza since this is a bigger change to the workflow if you feel uncomfortable with it we can configure a different branch or a separate repo to push to inside this PR and test there before pushing to master. |
Any update? 👀 |
Until there is some time to review this PR I will maintain my fork and rebase every now and then to make the ARM build available for anyone who needs them: https://github.com/pledra/doodba |
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.
Sorry, lack of time.
Please check when built if everything is OK and pushed, or tell me back if not. |
This PR introduces support for ARM builds and has been tested on Mac M1, a basic rundown on the changes: