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

Re-enable x11comp test #339

Closed
wants to merge 2 commits into from

Conversation

wismill
Copy link
Member

@wismill wismill commented May 10, 2023

This test works on my machine, let’s see if it works on our CI.

Fixes #30

@wismill
Copy link
Member Author

wismill commented May 10, 2023

I simplified the test by wrapping it with xvfb-run. Unfortunately, there is currently a Github incident so the CI is not running. Wait and see

@wismill wismill force-pushed the wip/test/re-enable-x11comp branch 3 times, most recently from b549af9 to 4121efd Compare May 10, 2023 18:20
@wismill
Copy link
Member Author

wismill commented May 11, 2023

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 xkbcomp. I do not think there is much we can do for now…

@bluetech
Copy link
Member

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.

@wismill
Copy link
Member Author

wismill commented May 13, 2023

IIRC, I already tried 22.04 locally and the issue was still there.

@bluetech
Copy link
Member

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.

@wismill
Copy link
Member Author

wismill commented May 13, 2023

It's not ready to merge. Maybe the only change worth to notice is the use of xvfb-run.

@wismill wismill added the testing Indicates a need for improvements or additions to testing label May 14, 2023
@whot
Copy link
Contributor

whot commented May 28, 2023

I guess the xserver in CI is too old to include it?

Looks like it should be included, at least in 22.04:

xserver $ git tag --contains 2c6989f81e62bac05a583c39fa09a06172d1ece5
xorg-server-21.1.2
xorg-server-21.1.3
xorg-server-21.1.4
...

Note this is the cherry-pick commit of that patch so it only lists the 21.1.x releases. The ubuntu package includes 21.1.3 atm. So this should work if it's the same bug.

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

$ podman  run --rm -it ubuntu:22.04

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.

@wismill wismill force-pushed the wip/test/re-enable-x11comp branch 2 times, most recently from 632bec1 to bc6ad55 Compare June 19, 2023 18:33
@wismill wismill marked this pull request as ready for review June 19, 2023 18:37
@wismill wismill marked this pull request as draft June 19, 2023 18:38
@wismill wismill force-pushed the wip/test/re-enable-x11comp branch 15 times, most recently from 7fc2478 to a1ee3b3 Compare June 21, 2023 10:42
@wismill wismill marked this pull request as ready for review June 21, 2023 11:04
@wismill
Copy link
Member Author

wismill commented Jun 21, 2023

@whot @bluetech I think I finally make it work!

Some comments:

  • I initially tried to use xvfb-run, but it turns out to be problematic with valgrind: some memory issues with dash.
  • Linux CI: For some reasons it does not work with older versions of xkb-data package, i.e. xkeyboard-config. I do not know which one precisely, but it seems 2.38 (Ubuntu 23.04) and 2.39 work. I use a hack to override the system version. Not pretty, but it works. Do you have a better idea?
  • macOS CI: for some reason unrelated to my modifications this CI started to fail. Any idea?

.github/workflows/linux.yml Outdated Show resolved Hide resolved
test/x11comp.c Outdated Show resolved Hide resolved
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
Copy link
Contributor

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

Copy link
Member Author

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?

Copy link
Contributor

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.

@whot
Copy link
Contributor

whot commented Jun 23, 2023

I initially tried to use xvfb-run, but it turns out to be problematic with valgrind: some memory issues with dash.

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.

I use a hack to override the system version. Not pretty, but it works. Do you have a better idea?

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.

@wismill wismill force-pushed the wip/test/re-enable-x11comp branch 2 times, most recently from bc99771 to 2e92fcf Compare June 23, 2023 08:59
@wismill
Copy link
Member Author

wismill commented Jun 23, 2023

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.

@whot I already tried bash and it has similar issues. I think it is better to to avoid xvfb-run entirely, as these suppressions files may also require maintenance.

The macOS CI is failing. Is sh: dot: command not found the only reason? It looks totally unrelated to this MR.

@wismill wismill force-pushed the wip/test/re-enable-x11comp branch from c3aa615 to 86fd60a Compare June 23, 2023 09:15
@wismill
Copy link
Member Author

wismill commented Jun 23, 2023

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.
@wismill wismill force-pushed the wip/test/re-enable-x11comp branch from 86fd60a to 0c082d4 Compare June 26, 2023 07:56
@wismill
Copy link
Member Author

wismill commented Jun 26, 2023

@whot Rebased. As I wrote hereinabove, I think it is better to avoid xvfb-run entirely, as valgrind suppressions files may also require maintenance. X11 being in maintenance-only mode, I believe the x11comp test should not require much maintenance, thus let’s not over optimize it!

I realize that the x11 test is actually skipped in the CI! We should probably use Xvfb there too.

Copy link
Contributor

@whot whot left a 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! :)

@whot
Copy link
Contributor

whot commented Jun 27, 2023

as valgrind suppressions files may also require maintenance.

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).

@wismill
Copy link
Member Author

wismill commented Jun 27, 2023

Closing in favour of #348.

@wismill wismill closed this Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Indicates a need for improvements or additions to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test/x11comp started to fail
3 participants