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

Build not only x64 #73

Merged
merged 12 commits into from
Jan 18, 2023
Merged

Build not only x64 #73

merged 12 commits into from
Jan 18, 2023

Conversation

Al2Klimov
Copy link
Member

fixes #27

@cla-bot cla-bot bot added the cla/signed label Jan 14, 2022
@Al2Klimov
Copy link
Member Author

@Al2Klimov
Copy link
Member Author

@julianbrost Can we split the Intel/ARM builds like this, docker push and.. everything will land together on DH?

@julianbrost
Copy link
Contributor

I don't know, I also used buildx for the first time this week. But I hope that should be possible.

Also, why drop ccache? Should be quite easy to use now and would be very helpful for local builds: https://github.com/moby/buildkit/blob/master/frontend/dockerfile/docs/syntax.md#run---mounttypecache

@Al2Klimov
Copy link
Member Author

  1. Do you agree that it makes sense to build x64 on x64, arm* on arm64 and then merge them together?
  2. I'm on it.

@julianbrost
Copy link
Contributor

Do you agree that it makes sense to build x64 on x64, arm* on arm64 and then merge them together?

If you'd have the infrastructure for it, yes. What I'm currently playing around with won't allow you to run privileged containers in the end, so that won't work here.

@Al2Klimov
Copy link
Member Author

OK, new findings:

docker buildx build seems to be designed to get all targets via --platform and to directly --push the image into the registry.

That seems to work.

Opera Momentaufnahme_2023-01-02_141931_hub docker com

Shall we just go that way?

@julianbrost
Copy link
Contributor

julianbrost commented Jan 3, 2023

docker buildx build seems to be designed to get all targets via --platform and to directly --push the image into the registry.

In the meantime, I also learned this. I feel like I should also have mentioned this before somewhere.

Shall we just go that way?

If we want ARM support, I'd say that's the way to go. Also, the build time of just over 2h for all 3 architectures sounds quite acceptable (especially when compared to the over 4h for Raspbian packages).

@Al2Klimov
Copy link
Member Author

@Al2Klimov Al2Klimov removed their assignment Jan 5, 2023
@Al2Klimov Al2Klimov marked this pull request as ready for review January 5, 2023 16:25
@Al2Klimov
Copy link
Member Author

In short, I completely re-designed this. I'd checkout the tree and look top-down.

  • build.bash behaves as usual, ex. with the add. arg. all -builds all platforms- or push TAGNAME which in addition pushes the result
  • the GHA-GHA test this GHA's build.bash, with all of course
  • the GHA itself also calls it with all on PR and with push TAGNAME on push/release
  • Dockerfile does the compiling, so no ccache

Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

the GHA-GHA test this GHA's build.bash, with all of course

I don't understand what this is trying to say.

Dockerfile does the compiling, so no ccache

That's no longer a limitation when building directly from a Dockerfile: https://docs.docker.com/engine/reference/builder/#run---mounttypecache

Also, this is somewhat important for my development workflow.

build.bash Outdated Show resolved Hide resolved
build.bash Outdated Show resolved Hide resolved
build.bash Outdated Show resolved Hide resolved
build.bash Show resolved Hide resolved
@Al2Klimov
Copy link
Member Author

the GHA-GHA test this GHA's build.bash, with all of course

I don't understand what this is trying to say.

Actually it's just this change: https://github.com/Icinga/docker-icinga2/pull/73/files#diff-5c3fa597431eda03ac3339ae6bf7f05e1a50d6fc7333679ec38e21b337cb6721R27

Dockerfile does the compiling, so no ccache

That's no longer a limitation when building directly from a Dockerfile: https://docs.docker.com/engine/reference/builder/#run---mounttypecache

Also, this is somewhat important for my development workflow.

We can't just hardcode absolute paths there, can we?

@julianbrost
Copy link
Contributor

We can't just hardcode absolute paths there, can we?

The hardcoded paths are inside the container, so yes we can. If I understand it correctly, that cache won't be shared with the build host but rather just between container builds. But that's fine, compiler and system headers probably won't match exactly between host and container (unless you run the exact same version of Debian).

mktags.bash Outdated Show resolved Hide resolved
mktags.bash Outdated

cat <<<"FROM icinga/icinga2:${BASH_REMATCH[1]}" >Dockerfile

"${BUILDX[@]}" -t "icinga/icinga2:${BASH_REMATCH[2]}" .
Copy link
Contributor

Choose a reason for hiding this comment

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

If I've parsed the use of BASH_REMATCH correctly, this will unconditionally update the icinga/icinga2:2.x.y tag? I'd also check if there's a newer version here, just to make this safe against rerunning the pipeline for an older release.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can re-run pipelines for older releases?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. I thought these workflows generally have a re-run button, but the one I checked didn't, but also I think they disappear after some time. So I can't say for sure.

mktags.bash Outdated Show resolved Hide resolved
mktags.bash Outdated Show resolved Hide resolved
@Al2Klimov
Copy link
Member Author

@Al2Klimov
Copy link
Member Author

mktags test

➜  docker-icinga2 git:(feature/arm-27) ✗ git diff
diff --git a/mktags.bash b/mktags.bash
index 7deffa4..5d59cc2 100755
--- a/mktags.bash
+++ b/mktags.bash
@@ -1,6 +1,6 @@
 #!/bin/bash
 # Icinga 2 Docker image | (c) 2023 Icinga GmbH | GPLv2+
-set -exo pipefail
+set -eo pipefail

 if [[ "$1" =~ ^v((([0-9]+).([0-9]+)).[0-9]+)$ ]]; then
   XYZ="${BASH_REMATCH[1]}"
@@ -8,7 +8,7 @@ if [[ "$1" =~ ^v((([0-9]+).([0-9]+)).[0-9]+)$ ]]; then
   X="${BASH_REMATCH[3]}"
   Y="${BASH_REMATCH[4]}"

-  BUILDX=(docker buildx build --platform "$(cat "$(realpath "$(dirname "$0")")/platforms.txt")" --push)
+  BUILDX=(echo docker buildx build --platform "$(cat "$(realpath "$(dirname "$0")")/platforms.txt")" --push)
   cd "$(mktemp -d)"

   echo "FROM icinga/icinga2:$XYZ" >Dockerfile
➜  docker-icinga2 git:(feature/arm-27) ✗ ./mktags.bash v2.12.1
docker buildx build --platform linux/amd64,linux/arm/v7,linux/arm64/v8 --push -t icinga/icinga2:2.12 .
➜  docker-icinga2 git:(feature/arm-27) ✗ ./mktags.bash v2.13.1
docker buildx build --platform linux/amd64,linux/arm/v7,linux/arm64/v8 --push -t icinga/icinga2:2.13 .
docker buildx build --platform linux/amd64,linux/arm/v7,linux/arm64/v8 --push -t icinga/icinga2:2 .
docker buildx build --platform linux/amd64,linux/arm/v7,linux/arm64/v8 --push -t icinga/icinga2 .
➜  docker-icinga2 git:(feature/arm-27) ✗

@julianbrost julianbrost merged commit 4a72040 into master Jan 18, 2023
@julianbrost julianbrost deleted the feature/arm-27 branch January 18, 2023 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build containers also for Raspberry Pi
2 participants