-
Notifications
You must be signed in to change notification settings - Fork 581
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
GHA: also build on Alpine to test LibreSSL which is used on OpenBSD #9949
base: master
Are you sure you want to change the base?
Conversation
As this PR was mentioned again over at the LibreSSL PR #9943, I have taken a look at it, rebased it and (hopefully) fixed the build errors. The effective diff between @Al2Klimov's initial state and my change should be: diff --git a/.github/workflows/alpine-bash.Dockerfile b/.github/workflows/alpine-bash.Dockerfile
index 26f6a5f9e..9119f5744 100644
--- a/.github/workflows/alpine-bash.Dockerfile
+++ b/.github/workflows/alpine-bash.Dockerfile
@@ -1,2 +1,8 @@
+# This Dockerfile is used in the linux job for Alpine Linux.
+#
+# As the linux.bash script is, in fact, a bash script and Alpine does not ship
+# a bash by default, the "alpine:bash" container will be built using this
+# Dockerfile in the GitHub Action.
+
FROM alpine:3
RUN ["apk", "--no-cache", "add", "bash"]
diff --git a/.github/workflows/linux.bash b/.github/workflows/linux.bash
index dfa763d91..488276e42 100755
--- a/.github/workflows/linux.bash
+++ b/.github/workflows/linux.bash
@@ -8,9 +8,31 @@ CMAKE_OPTS=''
case "$DISTRO" in
alpine:*)
- apk add bison {boost,libressl}-dev ccache cmake flex g++ ninja-build tzdata
+ # Install packages from Alpine package, just
+ # - LibreSSL instead of OpenSSL 3 and
+ # - no MariaDB or libpq as they depend on OpenSSL.
+ # https://gitlab.alpinelinux.org/alpine/aports/-/blob/master/community/icinga2/APKBUILD
+ apk add \
+ bison \
+ boost-dev \
+ ccache \
+ cmake \
+ flex \
+ g++ \
+ libedit-dev \
+ libressl-dev \
+ ninja-build \
+ samurai \
+ tzdata \
+ yajl-dev
ln -vs /usr/lib/ninja-build/bin/ninja /usr/local/bin/ninja
- CMAKE_OPTS="-DUSE_SYSTEMD=OFF $(echo -DICINGA2_WITH_{MYSQL,PGSQL,COMPAT,LIVESTATUS,PERFDATA,ICINGADB}=OFF)"
+
+ CMAKE_OPTS="-DUSE_SYSTEMD=OFF -DICINGA2_WITH_MYSQL=OFF -DICINGA2_WITH_PGSQL=OFF"
+
+ # This test fails due to some glibc/musl mismatch regarding timezone PST/PDT.
+ # - https://www.openwall.com/lists/musl/2024/03/05/2
+ # - https://gitlab.alpinelinux.org/alpine/aports/-/blob/b3ea02e2251451f9511086e1970f21eb640097f7/community/icinga2/disable-failing-tests.patch
+ sed -i '/icinga_legacytimeperiod\/dst$/d' /icinga2/test/CMakeLists.txt
;;
amazonlinux:2)
@@ -70,6 +92,10 @@ case "$DISTRO" in
dnf install -y bison ccache cmake gcc-c++ flex ninja-build \
{boost,libedit,mariadb,ncurses,openssl,postgresql,systemd}-devel
;;
+
+ *)
+ echo "UNKNOWN DISTRO \"$DISTRO\"" >&2
+ ;;
esac
mkdir /icinga2/build
diff --git a/.github/workflows/linux.yml b/.github/workflows/linux.yml
index 324fdfb62..2aac42c10 100644
--- a/.github/workflows/linux.yml
+++ b/.github/workflows/linux.yml
@@ -21,7 +21,10 @@ jobs:
max-parallel: 2
matrix:
distro:
- - alpine:bash # LibreSSL, used on OpenBSD
+ # Alpine Linux to build Icinga 2 with LibreSSL, OpenBSD's default.
+ # The "alpine:bash" image will be built below based on "alpine:3".
+ - alpine:bash
+
- amazonlinux:2
- amazonlinux:2023
@@ -60,7 +63,7 @@ jobs:
path: ccache
key: ccache/${{ matrix.distro }}
- - name: Build Docker image
+ - name: Build Alpine Docker Image
if: "matrix.distro == 'alpine:bash'"
run: >-
docker build --file .github/workflows/alpine-bash.Dockerfile The excluded test fails due to some mismatch between musl and glibc (and the libcs on the BSDs) I failed to pinpoint. Thus, I removed the test as the Alpine package maintainer did. After reflecting on this PR, this Icinga 2 build is based on musl and LibreSSL - quite unique, to say so. |
I have reverted my change as I did not wanted to "steal" @Al2Klimov's PR. Thus, I rebased the last state again and added my changes to fix the CI as another commit. As I already commented before, LibreSSL has become quite rare. Within the last years, Linux distributions using it primarily have dropped it1. I have not found any prominent Linux distribution using LibreSSL as its primary SSL library. Prominent LibreSSL users are OpenBSD (as its upstream) and its also available in FreeBSD, as reported in #10034. Footnotes |
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.
@oxzi Please wait with pushing:
First, the icinga_legacytimeperiod/dst test was excluded, as it fails on Alpine most likely due to some differences between musl and glibc. After some debugging, I disabled the test as the Alpine packages does. More build dependencies were added from the Alpine package, allowing to only disable MySQL and PostgreSQL support as these libraries have fixed dependencies on OpenSSL, conflicting with LibreSSL. In addition, I have added comments where I was first puzzled.
# This test fails due to some glibc/musl mismatch regarding timezone PST/PDT. | ||
# - https://www.openwall.com/lists/musl/2024/03/05/2 | ||
# - https://gitlab.alpinelinux.org/alpine/aports/-/blob/b3ea02e2251451f9511086e1970f21eb640097f7/community/icinga2/disable-failing-tests.patch | ||
sed -i '/icinga_legacytimeperiod\/dst$/d' /icinga2/test/CMakeLists.txt |
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.
As I wrote that test case, I was curious to find out what's supposed to be wrong here. That reference to https://www.openwall.com/lists/musl/2024/03/05/2 (Subject: Re: [tz] Weird PST8PDT and EST5EDT behavior on Alpine Linux) makes no sense to me. Yes, the test case can use TZ=PST8PDT
, but only if _WIN32
is defined:
icinga2/test/icinga-legacytimeperiod.cpp
Lines 55 to 62 in b96dc39
#ifndef _WIN32 | |
static const char *dst_test_timezone = "America/Los_Angeles"; | |
#else /* _WIN32 */ | |
// Tests are using pacific time because Windows only really supports timezones following US DST rules with the TZ | |
// environment variable. Format is "[Standard TZ][negative UTC offset][DST TZ]". | |
// https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/tzset?view=msvc-160#remarks | |
static const char *dst_test_timezone = "PST8PDT"; | |
#endif /* _WIN32 */ |
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 temporary switched the ifdef
guard logic from ifndef _WIN32
to ifdef __GNUC__
(there does not seem to be a musl macro) to let musl use the Windows parts, but the tests keep failing. As I have not found any real documentation in this regard - next to some hand wavering like the linked post -, I went with excluding the test as my primary motivation was to test the LibreSSL build.
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 went with excluding the test as my primary motivation was to test the LibreSSL build.
Sure, still shouldn't give an incorrect motivation for doing so. Can you please create a separate issue for this including the output of the failing test? (Maybe I remember enough and it gives me an immediate "oh, of course" reaction, but that would be out of scope here.)
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.
Funny is: We want to test OpenBSD, which doesn't exclude that test. We have to test it with Alpine, which fails that one test. xD
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.
Can you please create a separate issue for this including the output of the failing test?
Isn't that issue basically the same as #9801? These test cases also keep falling on my Mac, but I didn't care enough to invest time in debugging and fixing it so far.
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.
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.
Yonas has a point, but I doubt Mac uses musl. :)
No description provided.