sdl{2-compat,3}: clean up dependencies #414472
Conversation
The `installedTests` contains various test binaries, some of which only useful on a running system. Motivation for providing this output is to eventually test tray support in a VM test, which needs a running dbus session.
There was a problem hiding this comment.
I'm generally onboard with the goal and approach 👍
The VM test could be done here, in a followup or maybe not at all. I don't think dropping libdecor from the closure is feasible on linux given the stance of gtk, unless of course we resort to patching it to dlopen a magical /run path like graphics does. That however makes it way less usable on FHS distros
pkgs/by-name/sd/sdl3/package.nix
Outdated
| waylandSupport ? stdenv.hostPlatform.isLinux && !stdenv.hostPlatform.isAndroid, | ||
| x11Support ? !stdenv.hostPlatform.isAndroid && !stdenv.hostPlatform.isWindows, | ||
| zenitySupport ? false, # sdl3 will default to dbus and fail gracefully if zenity is not available. |
There was a problem hiding this comment.
Maybe provide an alternative for those who consider using an override
| zenitySupport ? false, # sdl3 will default to dbus and fail gracefully if zenity is not available. | |
| zenitySupport ? false, # sdl3 will default to dbus and fail gracefully if zenity is not available. sdl3 can still use zenity if available in PATH. |
There was a problem hiding this comment.
perhaps also this, considering this is about hermeticity and not support
| zenitySupport ? false, # sdl3 will defa 8000 ult to dbus and fail gracefully if zenity is not available. | |
| withZenity ? false, # sdl3 will default to dbus and fail gracefully if zenity is not available. |
discussed in NixOS/rfcs#169 although not merged or active
There was a problem hiding this comment.
valid argument, done.
|
|
351c266 to
aa521da
Compare
There was a problem hiding this comment.
The original motivation behind the zenity dependency was not the general dialog handler but the Wayland_ShowMessageBox function. It is called when the library initialization failed for some reason and it always uses zenity, it can't use dbus since dbus may not be initialized. If the zenity fails to launch the user will just see nothing in such cases. That being said, I'm still in favor of removing it since the closure size reduction is significant.
As for the libdecor, after skimming the source I think we could build the gtk plugin in a separate derivation and then point LIBDECOR_PLUGIN_DIR to it in the wrapGAppsHook. That way gtk apps and sessions could still rely on it. The downside is that the plugin will be added as a dependency even to apps that don't use libdecor but I think it's preferable compared to every libdecor dependent having gtk3 in its closure.
pkgs/by-name/sd/sdl3/package.nix
Outdated
| # sdl3 will default to dbus and fail gracefully if zenity is not available. | ||
| # sdl3 can still use zenity if available in $PATH | ||
| withZenity ? false, |
There was a problem hiding this comment.
If we are defaulting it to false then I would drop this entirely: It is much easier to provide it via PATH than to recompile the whole package just to replace some strings.
|
Ah, indeed, simple message box now fails, and adding zenity to path makes it work: the relevant code is in https://github.com/libsdl-org/SDL/blob/7dd5e765df239986f78c9b0016e3f3023d885084/src/video/wayland/SDL_waylandvideo.c#L681-L693 for the wayland driver. However: About |
|
I suppose we should then keep zenity default-enabled then... But then the actual question is, does
Is this just bad maintenance of SDL before we picked it up, or is this actually not needed? |
|
Apparently SDL2-era we just had programs do wrapping for zenity: nixpkgs/pkgs/by-name/me/mepo/package.nix Line 76 in f811655 Not sure this is the ideal solution... |
|
only |
Not sure, I'm not very familiar with these hooks.
I'll take a stab at it over the weekend.
Well, we have the option to just ignore it and hope the user has it in PATH. Arch doesn't list zenity in sdl3 deps, neither does Debian. It's also not mentioned anywhere in the SDL3 docs as a required dependency, you have to read the code to realize that it's actually required for this functionality. It's a subpar experience for Wayland users but it's an option.
If I recall correctly it'll call only zenity if the video driver is wayland. The issue with this interface is that it assumes that the OS provides some generic message box that can be used for catastrophic app failures. This is true for Windows, macOS or Android but on Linux this assumption fails. On X11 SDL3 draws a simple message box itself, on Wayland it hopes zenity is in PATH and on KMSDRM just does nothing. Not really sure if there's a principled fix for this. |
|
Possible long term solution: convince Freedesktop Group to add something like |
|
Okay i just checked, i also missed the fact ibus has quite the long gtk dep chain. |
+1
IMO this override just introduces needless complexity. |
zenity is used not only for error reporting, but also for file dialogs if dbus is disabled/unavailable. This needs a patched path too.
aa521da to
9149a3a
Compare
|
Alright, then there is not much more to be done here. VM test improvements is still on my list of things to do, but does not need to happen here. With zenity kept in the closure, these changes are pretty safe i believe. |
| lib.optionalString testSupport '' | ||
| substituteInPlace test/CMakeLists.txt \ | ||
| --replace-fail 'set(noninteractive_timeout 10)' 'set(noninteractive_timeout 30)' | ||
|
|
||
| substituteInPlace test/testtray.c \ | ||
| --replace-warn '../test/' '${placeholder "installedTests"}/share/assets/' |
There was a problem hiding this comment.
What's the reason for using --replace-warn instead of --replace-fail here?
There was a problem hiding this comment.
Because its a test output. I believe the package shouldn't hard-fail if a test binary might not have built successfully, that is my reasoning. It should be loud enough to find if something goes wrong, but not be annoying when it breaks.
There was a problem hiding this comment.
testtray is not part of our CI, so that one failing to replace doesn't actually affect anything.
|
builds on linux and darwin |
To follow up on this because I got distracted: this approach is unfortunately not viable because sdl3 apps built without using |
Tray support in sdl3 is nice, but completely unsupported by SDL2. Thus, it makes little sense to pull in all the dependencies needed for tray into sdl2-compat.
Similarly, both sdl2 and sdl3 support
zenityas a dialog backend. This is optional, and will by default only be used if dbus/portal backend fails. The classic SDL2 technically supported zenity upstream, but we never enabled that support. Similarly, sdl3 supports zenity, but our implementation was broken, always returning zenity was not available. Zenity can still be made available, either by setting the override or addingzenityto$PATHin a wrapper of a consumer package.While testing these, i needed the
installed-testsoutput to execute the test binaries in an interactive session.It might make sense to use the
installedTestsoutput e.g. in a VM test to make sure these override interfaces don't break.I also found ibus support requires dbus support and added an assertion for that.
This is currently a draft: I am not yet entirely decided on some aspects, and i would like to write VM tests for these things. sdl3 also still depends on
libdecorwhich in turn pulls in gtk3 into the closure. Arguably this is fine: SDL2_classic also depended on libdecor, and the closure size was still acceptable. But, if there is a reasonable way to drop it, we wouldn't need any gtk for the sdl going into ffmpeg, which could be nice!cc @NixOS/sdl, feedback welcome
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.