-
Notifications
You must be signed in to change notification settings - Fork 347
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
test_calculate_dimensions_height_min
test fails
#1404
Comments
All test pass both locally and on the ci for me. Could you please add details about the environment you are using? |
this test in particular does not rely on any particular library so it is really strange for it to fail. did you |
Oh, sure. It failed on the Alpine build system, therefore there were no dirty files. The complete build script is here: https://gitlab.alpinelinux.org/alpine/aports/-/blob/8ed5dc098fc54e52d91145b2eb8b103bc32e8c69/community/dunst/APKBUILD |
Now that's weird. We also have an alpine ci that succeed (https://github.com/dunst-project/dunst/actions/runs/12097617833/job/33733173054). Any possibility that it was a fluke? Could you rerun it if possible? Also was this the only thing to fail? |
Huh, very weird.
Actually I already retried the tests, but they fail on all CI builders across mulitple architectures. There are sometime also other failing tests but they are most likely flakey and not related to this issue. I checked your CI and seems similar to what we do in Alpine. What would come in mind, maybe it's still the dependencies. Your CI still seems to alpine:3.18 (https://github.com/dunst-project/docker-images/blob/master/ci/Dockerfile.alpine#L1C1-L1C17) which I tested against alpine:edge. |
so all the failures come from alpine:edge? |
I was reading the build log you linked and while it might not be related I found many errors like this
which is quite strange for me since glib should be fairly standard on all distros. Maybe new versions changed guint64 from long int to long long int? Needs more investigation EDIT: only appears on x86 so it is indeed unrelated |
I just upgraded to 1.12 on Arch and all my notifications are too tall, downgrading to 1.11 fixes it. Could these issues be related? Edit: Never mind, there's an existing and already solved issue #1392 that was hidden by default. |
Please read the RELEASE_NOTES, I described the required changes |
I bumped alpine docker from 3.18 to 3.21 and also added an alpine edge to the ci. |
I can reproduce exactly the same issue on openSUSE Tumbleweed, where I was trying to bump the package version to 1.12.1. The same test fails, because You can reproduce this issue relatively easily using the following from registry.opensuse.org/opensuse/tumbleweed:latest
RUN set -euo pipefail; \
zypper modifyrepo -e repo-source; \
zypper -n source-install -d dunst; \
zypper -n in 'pkgconfig(wayland-protocols)' gcc git make awk; \
zypper -n clean; rm -rf /var/log/
RUN git clone https://github.com/dunst-project/dunst
WORKDIR /dunst
RUN git checkout v1.12.1
RUN make
RUN make test (the build fails at |
It seems this can be reproduced in the alpine:edge, void linux and tumbleweed ci. However locally and our alpine:edge it is not reproducible. I will try to investigate this. However I am a docker noob and also this week I am very busy. If someone could help me figure this out I would be grateful. Anyway I was thinking that maybe this is a result of compiler flags and some undefined behavior? I don't think this is related to the dependencies since that piece of code shouldn't have any (apart maybe from pango?) |
Sadly, it doesn't appear to be the case. I have taken the compiler flags from Fedora and applied them on Tumbleweed, still get the same failure: from registry.opensuse.org/opensuse/tumbleweed:latest
RUN set -euo pipefail; \
zypper modifyrepo -e repo-source; \
zypper -n source-install -d dunst; \
zypper -n in 'pkgconfig(wayland-protocols)' gcc git make awk libubsan1 libasan8; \
zypper -n clean; rm -rf /var/log/
RUN git clone https://github.com/dunst-project/dunst
WORKDIR /dunst
RUN git checkout v1.12.1
ENV CFLAGS="-O2 -Wall -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -m64 -march=x86-64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -mtls-dialect=gnu2 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -fstack-protector-strong"
RUN make
RUN make test and with that we still get:
So I have tried to use ASAN & UBSAN, but sadly UBSAN doesn't find anything, but ASAN finds a memory leak (but in a totally different place):
To double check, I have also tried to build dunst in the same way on Fedora, and there the test suite passes (without ASAN enabled, with ASAN, you get an even longer memory leak output, because on Fedora dnf pulls in librsvg2). And I have also tried to run the tests with clang on Tumbleweed, which leads to the same failures as with gcc. I'm afraid I'm a bit out of ideas here as well |
I have some updates I opened a docker console with the tumbleweed dockerfile and did some testing. I narrowed down the problem to this line Line 237 in 381fd98
Basically the problem is: the text in pangolayout gives and unreasonably large height in pixel for the "dummy layout" text. (something like 1825239). This gets added to the overall dimension of the notification that then get clamped down to the max height. The expected behavior (that I get on ci and on local machine) is instead that the notification body is small and we pad up to the minimum height. What I did not understand yet is if this error is a bug in a specific version of pango/cairo or a bug on our part disguised by some undefined behavior. probably there is something wrong about the initialization of the pangolayout/context any help is appreciated |
@dcermak @fossdd I have finally discovered the problem: missing fonts 🤣 Basically if no fonts are present in the docker image pangocairo gives the bogus value I described above. So, our ci worked because we installed dejavu-fonts (any font really should work). I used So now my question is, how to fix this? I am not sure that this can be considered a problem on our part since we rely on pangocairo and fontconfig for the text bits. But on the other hand it seems quite weird to require a font in the build file/package. Maybe you can make dunst depend on something like |
Wow thanks that seems to work! Thank you for this work!
I think for desktop applications like dunst a font should be assumed. I'll add it rather as a dependency for tests. |
Ok thanks. This seems like a shortcoming with fontconfig/pango/cairo but I don't expect this to be fixed even if I open a bug report. I was thinking that maybe we could embed a font file in the test directory but I am not sure how to pass it to pango |
I found this blog post saying that you can set FONTCONFIG_FILE to your local fonts.conf. We may do that an ship a random font in the test directory. This way system fonts are not queried at all |
happy new year. I made a pr that includes the fonts in the test suite. now it should work |
Issue description
The following test failure is shown when trying to
make test
.Installation info
1.12.0
The text was updated successfully, but these errors were encountered: