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

ci: use SDL2 from head #431

Closed
wants to merge 1 commit into from
Closed

Conversation

madebr
Copy link
Collaborator

@madebr madebr commented Jan 5, 2025

SDL2 from head includes a patch that does not fail CMake configuration when libX11 can be found, but no xext.h header.

@madebr
Copy link
Collaborator Author

madebr commented Jan 5, 2025

All 2.x releases from the SDL2 are ABI compatible, so breakage is no concern.

@Squall-Leonhart
Copy link

All 2.x releases from the SDL2 are ABI compatible, so breakage is no concern.

breakage is always a concern,

bind to a release tag, and not get surprised by accidental regressions you can't track for.

@madebr
Copy link
Collaborator Author

madebr commented Jan 6, 2025

All 2.x releases from the SDL2 are ABI compatible, so breakage is no concern.

breakage is always a concern,

bind to a release tag, and not get surprised by accidental regressions you can't track for.

True, but I trust the SDL devs on this one. ;)

@b-kurczynski
Copy link
Contributor

b-kurczynski commented Jan 6, 2025

So what exactly happen? If I see correctly the runner switched from Ubuntu 22.x to 24.x, right? If there is a risk of regression due to usage of SDL2's bleeding-edge, and the issue is that 24.x is missing certain header files (missing packages?) compared to 22.x, then maybe it's better to revert back to use latest SDL2 and just install the missing package to 24.x like I did attempt in my PR?

Edit: I dig a bit more.

The last 22.x runner which succeeded used this version:
https://github.com/actions/runner-images/blob/ubuntu22/20241201.1/images/ubuntu/Ubuntu2204-Readme.md

The first 24.x runner which failed used this version:
https://github.com/actions/runner-images/blob/ubuntu24/20241208.1/images/ubuntu/Ubuntu2404-Readme.md

I spot that 22.x contains this package:
https://packages.debian.org/buster/libxss1 which depends on https://packages.debian.org/buster/libxext6

24.x does not contain libxss1 thus no libxext6 thus no Xext.h header files? There is even a suggestion to install missing package here: https://github.com/dethrace-labs/dethrace/actions/runs/12359607398/job/34492714851?pr=428#step:6:421

@Squall-Leonhart
Copy link

True, but I trust the SDL devs on this one. ;)

you should probably look at the SDL 2.30.6 regressions then.

@madebr
Copy link
Collaborator Author

madebr commented Jan 6, 2025

True, but I trust the SDL devs on this one. ;)

you should probably look at the SDL 2.30.6 regressions then.

What regressions? Can you link a SDL GitHub issue?

For dethrace on Linux ci, since we don't ship a SDL2 elf library in the artifacts, we only care about the API/ABI interface, which is frozen.

@madebr
Copy link
Collaborator Author

madebr commented Jan 6, 2025

24.x does not contain libxss1 thus no libxext6 thus no Xext.h header files? There is even a suggestion to install missing package here: https://github.com/dethrace-labs/dethrace/actions/runs/12359607398/job/34492714851?pr=428#step:6:421

For Linux ci purposes, we don't really need x11 or wayland support. The only thing we care about on Linux is running the ci headless tests, and the dethrace executable correctly linked against a valid SDL2 library.
If we really want wayland/x11 support, we'd need a lot of extra development packages, of which libxext-dev is only one.
The libsdl-org/setup-sdl has a install-linux-dependencies option which should automatically install all dependencies.

@b-kurczynski
Copy link
Contributor

24.x does not contain libxss1 thus no libxext6 thus no Xext.h header files? There is even a suggestion to install missing package here: https://github.com/dethrace-labs/dethrace/actions/runs/12359607398/job/34492714851?pr=428#step:6:421

For Linux ci purposes, we don't really need x11 or wayland support. The only thing we care about on Linux is running the ci headless tests, and the dethrace executable correctly linked against a valid SDL2 library. If we really want wayland/x11 support, we'd need a lot of extra development packages, of which libxext-dev is only one. The libsdl-org/setup-sdl has a install-linux-dependencies option which should automatically install all dependencies.

Correct. However, please notice that until the runner was on 22.x the x11-ext packages where there and everything worked ok with no side effects. What is proposed now is: 1) use SDL2's head instead of official version; 2) SDL2 head IMO contains questionable fix to make Dethrace CI working - it alters CMake checks behavior and might create regression for the others and no longer provides valuable error/warning message.

@madebr
Copy link
Collaborator Author

madebr commented Jan 6, 2025

  1. use SDL2's head instead of official version;

Correct. That is the C in CI.

  1. SDL2 head IMO contains questionable fix to make Dethrace CI working - it alters CMake checks behavior and might create regression for the others and no longer provides valuable error/warning message.

Right. SDL3 fails when there is no X11 or no Wayland support (there is a CMake option to allow this). Perhaps SDL2 should do the same.

@b-kurczynski
Copy link
Contributor

  1. use SDL2's head instead of official version;

Correct. That is the C in CI.

  1. SDL2 head IMO contains questionable fix to make Dethrace CI working - it alters CMake checks behavior and might create regression for the others and no longer provides valuable error/warning message.

Right. SDL3 fails when there is no X11 or no Wayland support (there is a CMake option to allow this). Perhaps SDL2 should do the same.

The issue seems to be solely on Dethrace ground. CI/CD environment has changed because Ubuntu 24.x offers different set of pre-installed packages than 22.x. To me it means that the fix should be applied in Dethrace repo only. What is proposed is >>this<< change which might spawn a wave of new issues to the others - I would call it potential domino effect. I do understand that Dethrace does not use X11, but that's the prerequisite for SDL2.

It's all I can tell to convince you to change the proposed solution and more importantly to revert the change pushed to SDL2 repo. It's up to you which way you pick.

@madebr madebr marked this pull request as draft January 6, 2025 19:35
@madebr madebr closed this Jan 6, 2025
@madebr madebr deleted the ci-use-latest-sdl2 branch January 6, 2025 20:32
@dethrace-labs
Copy link
Owner

I added #432 as the smallest change required to fix the github build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants