-
Notifications
You must be signed in to change notification settings - Fork 125
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
Re-enable x11comp test #339
Conversation
I simplified the test by wrapping it with |
b549af9
to
4121efd
Compare
I am doing some trials locally, but I encounter a weird bug that seems to be https://gitlab.freedesktop.org/xorg/xserver/-/issues/1249. It may be the same bug that triggers with this test when using |
Seems like the bug you linked to was fixed by @whot some months ago, I guess the xserver in CI is too old to include it? I just updated the CI linux job to run on the latest ubuntu (22.04); it's probably too old as well, but worth a try anyway to rebase on master and see what happens. |
IIRC, I already tried 22.04 locally and the issue was still there. |
Then I'll guess we'll have to wait a few more year :) However, if there are changes you want to make to the test, while still skipping it for now, let me know, we can merge that. |
It's not ready to merge. Maybe the only change worth to notice is the use of |
Looks like it should be included, at least in 22.04:
Note this is the cherry-pick commit of that patch so it only lists the But checking the workflow yaml file: we don't appear to be installing X on purpose, might be good to have this listed explicitly. maybe it's pulled in by graphviz? Anyway, testlog shows it does indeed run but otherwise the output is not very useful. Best way to reproduce locally would be with
and then install the dependencies and run in that container. At least that way it can be debugged interactively instead of having short meson logs. |
632bec1
to
bc6ad55
Compare
7fc2478
to
a1ee3b3
Compare
@whot @bluetech I think I finally make it work! Some comments:
|
.github/workflows/linux.yml
Outdated
libwayland-dev wayland-protocols bison graphviz | ||
doxygen libxcb-xkb-dev valgrind ninja-build meson \ | ||
libwayland-dev wayland-protocols bison graphviz curl | ||
python -m pip install --upgrade PyYAML |
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 rework means we're no longer testing against the latest released meson but against whatever our ubuntu version ships. That's generally not ideal - any specific reason why you reshuffled this? I don't see the old failing logs so it's unclear
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.
Fixed.
Testing locally with podman, it uses root
, which cannot use pip to update system dependencies. How I reproduce the exact same config as the CI locally?
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.
hmm, good question, I don't actually know how to pull the same containers (VMs?) github uses. if you just need an extra user you can create that inside podman and then su
to it.
can you install bash and run in that somehow? You can also add a valgrind suppressions file like the one libinput has - that one has a few bash-specifics in it because we really don't care about those.
tbh, building against whatever the latest version of xkeyboard-config is seems like a good idea anyway, the two projects are quite intertwined so we want things to blow up early. |
bc99771
to
2e92fcf
Compare
@whot I already tried bash and it has similar issues. I think it is better to to avoid The macOS CI is failing. Is |
c3aa615
to
86fd60a
Compare
Let’s investigate the macOS issue in #346. Then I will rebase. |
Fix the issue that caused it to fail early.
xkeyboard-config and xkbcommon projects are quite intertwined so we want things to blow up early. It also solves an issue with the x11comp test.
86fd60a
to
0c082d4
Compare
@whot Rebased. As I wrote hereinabove, I think it is better to avoid I realize that the x11 test is actually skipped in the CI! We should probably use Xvfb there too. |
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.
better run + skip than not run at all! :)
Luckily very little but yes, they do. I think in libinput it averaged to one or two per year but none since 2020 so it's not the worst of all things (and libinput builds against a lot of distros/versions). |
Closing in favour of #348. |
This test works on my machine, let’s see if it works on our CI.
Fixes #30