Skip to content

SDL_compat: set rpath during installation; add tests #405305

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
pbsds merged 3 commits into from
May 10, 2025
Merged

SDL_compat: set rpath during installation; add tests #405305

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fb4b83f
SDL_compat: set rpath during installation
marcin-serwin May 8, 2025
bf1a329
SDL_compat: add basic checkPhase
marcin-serwin May 8, 2025
69948e3
SDL_compat: add passthru.tests
marcin-serwin May 8, 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
64 changes: 46 additions & 18 deletions pkgs/by-name/sd/SDL_compat/package.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@
pkg-config,
pkg-config-unwrapped,
stdenv,
testers,
dosbox,
SDL_image,
SDL_ttf,
SDL_mixer,
SDL_sound,
# Boolean flags
libGLSupported ? lib.elem stdenv.hostPlatform.system mesa.meta.platforms,
openglSupport ? libGLSupported,
Expand Down Expand Up @@ -52,8 +58,33 @@ stdenv.mkDerivation (finalAttrs: {
]
++ lib.optionals openglSupport [ libGLU ];

postPatch = ''
substituteInPlace CMakeLists.txt \
--replace-fail 'set(CMAKE_SKIP_RPATH TRUE)' 'set(CMAKE_SKIP_RPATH FALSE)'
'';

dontPatchELF = true; # don't strip rpath

cmakeFlags =
let
rpath = lib.makeLibraryPath [ sdl2-compat ];
in
[
(lib.cmakeFeature "CMAKE_INSTALL_RPATH" rpath)
(lib.cmakeFeature "CMAKE_BUILD_RPATH" rpath)
(lib.cmakeBool "SDL12TESTS" finalAttrs.finalPackage.doCheck)
];

enableParallelBuilding = true;

# Darwin fails with "Critical error: required built-in appearance SystemAppearance not found"
doCheck = !stdenv.hostPlatform.isDarwin;
checkPhase = ''
runHook preCheck
./testver
runHook postCheck
'';
Comment on lines +82 to +86
Copy link
Contributor

@LordGrimmauld LordGrimmauld May 8, 2025

Choose a reason for hiding this comment

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

Suggested change
checkPhase = ''
runHook preCheck
./testver
runHook postCheck
'';
nativeInstallCheckInputs = [
versionCheckHook
];
doInstallCheck = true;

The version check hook is probably the more-correct way of doing this check

Copy link
Contributor Author

@marcin-serwin marcin-serwin May 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

versionCheckHook runs the installed executable of the package with --version argument, the testver is just one of many example programs that are built alongside the lib and never installed. Most of them are displaying something on the screen or are benchmarking so they're not very useful for us.

Copy link
Contributor

@LordGrimmauld LordGrimmauld May 8, 2025

Choose a reason for hiding this comment

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

Okay fair! Just feels a bit weird: sdl-config has a --version flag you can give it, so versionCheckHook do the same thing at a glance. Might need a comment explaining you want the example program explicitly, not simply a version check.

Copy link
Contributor Author

@marcin-serwin marcin-serwin May 8, 2025

Choose a reason for hiding this comment

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

I forgot that it installs sdl-config. Looking at the code, the --version argument just echoes the hardcoded version so testver, which actually links to the built library, is a better indication that it works. We can still add versionCheckHook as a separate test to check if sdl-config is installed.


postInstall = ''
# allow as a drop in replacement for SDL
# Can be removed after treewide switch from pkg-config to pkgconf
Expand All @@ -66,24 +97,17 @@ stdenv.mkDerivation (finalAttrs: {
patches = [ ./find-headers.patch ];
setupHook = ./setup-hook.sh;

postFixup = ''
for lib in $out/lib/*${stdenv.hostPlatform.extensions.sharedLibrary}* ; do
if [[ -L "$lib" ]]; then
${
if stdenv.hostPlatform.isDarwin then
''
install_name_tool ${
lib.strings.concatMapStrings (x: " -add_rpath ${lib.makeLibraryPath [ x ]} ") finalAttrs.buildInputs
} "$lib"
''
else
''
patchelf --set-rpath "$(patchelf --print-rpath $lib):${lib.makeLibraryPath finalAttrs.buildInputs}" "$lib"
''
}
fi
done
'';
passthru.tests = {
pkg-config = testers.testMetaPkgConfig finalAttrs.finalPackage;

inherit
SDL_image
SDL_ttf
SDL_mixer
SDL_sound
dosbox
;
};

meta = {
homepage = "https://www.libsdl.org/";
Expand All @@ -93,5 +117,9 @@ stdenv.mkDerivation (finalAttrs: {
maintainers = with lib.maintainers; [ peterhoeg ];
teams = [ lib.teams.sdl ];
platforms = lib.platforms.all;
pkgConfigModules = [
"sdl"
"sdl12_compat"
];
};
})