Skip to content

SDL_compat: use SDL3 through sdl2-compat #386495

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.

Sign up for GitHub

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

Merged
K900 merged 2 commits into from
Mar 15, 2025
Merged

SDL_compat: use SDL3 through sdl2-compat #386495

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
abaf2bd
SDL_compat: use SDL3 through sdl2-compat
marcin-serwin Mar 2, 2025
7aeac03
SDL_compat: don't propagate build inputs
marcin-serwin Mar 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions pkgs/by-name/sd/SDL_compat/package.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
{
lib,
SDL2,
sdl2-compat,
cmake,
darwin,
fetchFromGitHub,
libGLU,
libiconv,
libX11,
mesa,
pkg-config,
stdenv,
Expand Down Expand Up @@ -38,8 +39,11 @@ stdenv.mkDerivation (finalAttrs: {
autoSignDarwinBinariesHook
];

propagatedBuildInputs =
[ SDL2 ]
buildInputs =
[
libX11
Copy link
Contributor

@K900 K900 Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does libx11 come from here? Was it propagated by normal SDL2 or something?

Copy link
Contributor

@LordGrimmauld LordGrimmauld Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was, see

nixpkgs/pkgs/development/libraries/SDL2/default.nix

Lines 114 to 119 in e3e32b6

dlopenPropagatedBuildInputs =
[ ]
# Propagated for #include <GLES/gl.h> in SDL_opengles.h.
++ lib.optional (openglSupport && !stdenv.hostPlatform.isDarwin) libGL
# Propagated for #include <X11/Xlib.h> and <X11/Xatom.h> in SDL_syswm.h.
++ lib.optionals x11Support [ libX11 ];

It might however make sense to do the following:

# SDL_compat
propagatedBuildInputs = sdl2-compat.propagatedBuildInputs ++ [ ... ];
# sdl2-compat
propagatedBuildInputs = sdl3.propagatedBuildInputs ++ [ ... ];

This adds extra complexity, but would allow us to not maintain multiple lists of the same thing.

Copy link
Contributor

@LordGrimmauld LordGrimmauld Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not quite familiar with how propagatedBuildInputs works, there might be better ways.)

Copy link
Contributor

@K900 K900 Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be necessary though, that's what propagation does already - if sdl2-compat is in sdl-compat's propagatedBuildInputs, sdl2's propagatedBuildInputs will also be propagated.

Copy link
Contributor

@LordGrimmauld LordGrimmauld Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thats the issue!!
See

nixpkgs/pkgs/by-name/sd/sdl2-compat/package.nix

Lines 36 to 38 in 74b648b

buildInputs = [
sdl3
];

sdl2-compat is not listing sdl3 as propagated build input. THAT is the issue and should be fixed. Then explicit libX11 can be dropped here.

Copy link
Contributor

@K900 K900 Mar 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should, however, propagate libX11, because include/SDL2/SDL_syswm.h refers to xlib includes.

Copy link
Contributor

@LordGrimmauld LordGrimmauld Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sdl2-compat also does a dlopen on sdl3 at runtime, see #388079. The fact that is currently missing is more a mistake/bug than anything.
(these two PRs really should have been one, huh?)

Copy link
Contributor

@LordGrimmauld LordGrimmauld Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i need to do some testing. But either way, libX11 being in SDL_compat explicitly seems to not be the correct solution.

Copy link
Contributor Author

@marcin-serwin marcin-serwin Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sdl2-compat also does a dlopen on sdl3 at runtime, see #388079. The fact that is currently missing is more a mistake/bug than anything.

But that is handled by rpath and doesn't really have to do with propagated build inputs, or am I missing something?

But either way, libX11 being in SDL_compat explicitly seems to not be the correct solution.

I'm still a bit fuzzy on how exactly propagated build inputs work but I don't understand why that wouldn't be the correct approach. It seems like sdl2-compat doesn't need to propagate libX11, since I've managed to build some games without it, but SDL1 does so that's why it appears here. It's a bit weird that SDL2 propagated libX11 but maybe that was the issue in the first place and now it got accidentally corrected in sdl2-compat?

Copy link
Contributor

@LordGrimmauld LordGrimmauld Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SDL2 propagated libX11. If sdl2-compat is supposed to be a drop-in replacement, it should do the same imo.

sdl2-compat
]
++ lib.optionals stdenv.hostPlatform.isDarwin [
libiconv
Cocoa
Expand Down Expand Up @@ -67,14 +71,12 @@ stdenv.mkDerivation (finalAttrs: {
if stdenv.hostPlatform.isDarwin then
''
install_name_tool ${
lib.strings.concatMapStrings (
x: " -add_rpath ${lib.makeLibraryPath [ x ]} "
) finalAttrs.propagatedBuildInputs
lib.strings.concatMapStrings (x: " -add_rpath ${lib.makeLibraryPath [ x ]} ") finalAttrs.buildInputs
} "$lib"
''
else
''
patchelf --set-rpath "$(patchelf --print-rpath $lib):${lib.makeLibraryPath finalAttrs.propagatedBuildInputs}" "$lib"
patchelf --set-rpath "$(patchelf --print-rpath $lib):${lib.makeLibraryPath finalAttrs.buildInputs}" "$lib"
''
}
fi
Expand Down