-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
Use docker buildx for ftl-build containers #69
Conversation
…p using fine-crafted hand-optimized individual containers for every target that tend to break in surprising ways on any major OS update Signed-off-by: DL6ER <dl6er@dl6er.de>
… as armv6), tested with stretch and bullseye Signed-off-by: DL6ER <dl6er@dl6er.de>
I left the removal of the Debian-based image intentionally in the |
…atch, build on pushes to ftl-build related files Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
096695d
to
1dcb90e
Compare
Signed-off-by: DL6ER <dl6er@dl6er.de>
9e87ab4
to
fc75b95
Compare
…d managed separately Signed-off-by: DL6ER <dl6er@dl6er.de>
@PromoFaux Alongside this change the FTL binary names change to
Not sure if this needs some modifications in the v6 |
# Run the full test suite to ensure that the container is still capable of running the tests | ||
RUN git clone https://github.com/pi-hole/FTL.git \ | ||
RUN git clone https://github.com/pi-hole/FTL.git --branch "${GIT_BRANCH}" \ |
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 will checkout sources for every platform you're building which is not optimal. You can just checkout against the build platform in a dedicated stage and mount the sources here. And for cache optim you should also init and checkout instead of cloning like: https://github.com/crazy-max/docker-fail2ban/blob/c779f371005948587ed90fcc28cfd819033a8083/Dockerfile#L6-L11
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
4c5be38
to
3ea78d3
Compare
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
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 removed the pull_request
trigger in ftl-build.yml
(L3-6) so there is no need to check for that trigger anymore.
container: alpine:3.18 | ||
- platform: linux/386 | ||
container: alpine:3.18 | ||
- platform: linux/arm/v5 |
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.
Should the debian ones not be linux/arm/v4
and linux/arm/v5
? Seems we have linux/arm/v6
twice...
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 just remember a very confusing conversation about this.
Pinging: @DL6ER
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.
Yes, because Debian always builds for one architecture lower. In the sense of "if you have an armv6 you can definitely run armv5 binaries". Alpine doesn't do this.
That's why
debian/armv5
actually buildsarmv4
binaries,debian/armv6
actually buildsarmv5
binaries, howeveralpine/armv6
actually buildsarmv6
Forgive my naivety on this kind of thing, but does this mean we can drop some things from the dependencies? (i.e libidn, nettle etc) |
Yes, everywhere except the non-static builds (armv4 and v5) |
Co-authored-by: yubiuser <ckoenig@posteo.de> Signed-off-by: DL6ER <DL6ER@users.noreply.github.com>
Signed-off-by: DL6ER <DL6ER@users.noreply.github.com>
Interestingly, it seems that the most recent commit which did nothing else than |
I checked the previous runs and compared them to the failed ones. |
Could the issue be this line:
Where |
Good find. That probaly shouldn't be hardcoded there On the docker repo I ended up doing the two registries in a matrix just to keep the file clean |
Co-authored-by: yubiuser <ckoenig@posteo.de> Signed-off-by: Adam Warner <me@adamwarner.co.uk>
9bd2f64
to
3820c3d
Compare
This seems to have fixed it. What else do we need for this PR to land? All FTL v6.0 builds are using this containers for a while: https://github.com/pi-hole/FTL/blob/4ac9f120f5750c2a179b2de86e0a35c64adbf5aa/.github/Dockerfile#L2 |
I think nothing. It's your PR if you think it's ready, lets us know to review. |
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.
Miles behind on reviews and things. Sorry!
What does this implement/fix?
This is a complete rewrite of how the
ftl-build
containers are built. Instead of fine-tuned, hand-crafted and with intense care written individual docker files for every target architecture (that were always creating hours of work when the distro was upgraded), we switch todocker buildx
with one simpleDockerfile
used for everything. This makes maintenance a lot easier and greatly simplifies the overall process.Furthermore, this resolves one particular issue we have had a few times in the past with the dependence on particular
glibc
versions by switching everything tomusl
-based so FTL will from now on ship as fully static batteries-included binaries that can run on the irrespective of the operating system version.This PR removes support forarmv4
(last supporting Debian release was Stretch) andarmv5
as these architectures are not supported by Alpine. I tried to create anarmv5
binary usingdebian:stretch
and:bullseye
as base image but it - to my surprise - created anarmv6
binary we cannot use.Update: We will still generate
armv4T
andarmv5TE
binaries using Debian builders.The dependency on a sufficiently recent GLIBC remains.The resulting containers should be tagged
v2.0
Related issue or feature (if applicable): N/A
Pull request in docs with documentation (if applicable): N/A
By submitting this pull request, I confirm the following:
git rebase
)Checklist:
developmental
branch.