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

sdl3: init at 3.1.8 #326699

Merged
merged 1 commit into from
Jan 20, 2025
Merged

sdl3: init at 3.1.8 #326699

merged 1 commit into from
Jan 20, 2025

Conversation

getchoo
Copy link
Member

@getchoo getchoo commented Jul 13, 2024

Description of changes

Closes #324694

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jul 13, 2024
@pbsds
Copy link
Member

pbsds commented Jul 30, 2024

freebsd has the version at 3.1.2, and that version seems to be the case in te rev you've selected as well:

https://github.com/libsdl-org/SDL/blob/730d5cf2f889b553852bd02b2d56dedf8690872a/include/SDL3/SDL_version.h#L41-L66

@getchoo
Copy link
Member Author

getchoo commented Aug 2, 2024

Looks like these tags were yanked. See libsdl-org/SDL#10367

Quoting from upstream:

Because the API is changing daily and we don't want people to freeze on a specific version. We have an ABI stable release coming soon, and we'll tag that for people to play with.

Based on this, the package here could be merged as is (though i'll give it a commit rev bump) IMO. An equally good alternative would be waiting for that stable ABI release soon ™️. Feedback welcome :)

@getchoo getchoo changed the title sdl3: init at 3.1.1-unstable-2024-07-12 sdl3: init at 3.1.2-unstable-2024-08-01 Aug 2, 2024
@pbsbot
Copy link

pbsbot commented Sep 13, 2024

Result of nixpkgs-review pr 326699 run on x86_64-linux 1

3 packages built:
  • sdl3
  • sdl3.dev
  • sdl3.lib

@pbsds
Copy link
Member

pbsds commented Sep 13, 2024

sorry for the delay. This LGTM for the most part, but i think this expression risks being bumped down to SDL2 by nix-update or r-ryantm. Could you set updateScript?

Copy link
Contributor

@OPNA2608 OPNA2608 left a comment

Choose a reason for hiding this comment

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

Configure warns about some things that are still missing:

-- Could NOT find OpenGL (missing: OPENGL_opengl_LIBRARY OPENGL_glx_LIBRARY OPENGL_INCLUDE_DIR)
[...]
-- Checking for modules 'wayland-client>=1.18;wayland-egl;wayland-cursor;egl;xkbcommon>=0.5.0'
--   No package 'egl' found
[...]
-- Checking for modules 'libunwind;libunwind-generic'
--   No package 'libunwind' found
--   No package 'libunwind-generic' found
-- Checking for module 'libusb-1.0>=1.0.16'
--   No package 'libusb-1.0' found
-- Could NOT find LibUSB (missing: LibUSB_LIBRARY LibUSB_INCLUDE_PATH) (found version "LibUSB_VERSION-NOTFOUND")

I think this also needs the dlopen workaround from our SDL2 packaging, so sdl3 users don't need to worry about adding all the dlopened dependencies to their LD_LIBRARY_PATH / rpath?

There's also a test subdirectory with tests that we could run via

  cmakeFlags = [
    (lib.cmakeBool "SDL_TESTS" finalAttrs.finalPackage.doCheck)
  ];

  doCheck = stdenv.buildPlatform.canExecute stdenv.hostPlatform;

Can we get them working? (Though I don't think sdl2 runs them either, so 🤷‍♀️)

pkgs/by-name/sd/sdl3/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/sd/sdl3/package.nix Show resolved Hide resolved
pkgs/by-name/sd/sdl3/package.nix Outdated Show resolved Hide resolved
@getchoo getchoo changed the title sdl3: init at 3.1.2-unstable-2024-08-01 sdl3: init at 3.1.6 Nov 3, 2024
@getchoo
Copy link
Member Author

getchoo commented Nov 3, 2024

Sorry for the wait. I've updated upstream's latest preview tag, (hopefully) applied the correct RPATH changes, and made a ton of overrides for build options similar to the current SDL2 package

Configure warns about some things that are still missing:

-- Could NOT find OpenGL (missing: OPENGL_opengl_LIBRARY OPENGL_glx_LIBRARY OPENGL_INCLUDE_DIR)
[...]
-- Checking for modules 'wayland-client>=1.18;wayland-egl;wayland-cursor;egl;xkbcommon>=0.5.0'
--   No package 'egl' found
[...]
-- Checking for modules 'libunwind;libunwind-generic'
--   No package 'libunwind' found
--   No package 'libunwind-generic' found
-- Checking for module 'libusb-1.0>=1.0.16'
--   No package 'libusb-1.0' found
-- Could NOT find LibUSB (missing: LibUSB_LIBRARY LibUSB_INCLUDE_PATH) (found version "LibUSB_VERSION-NOTFOUND")

Is there any example project similar to what you're compiling here we could add to passthru.tests? Would probably help in not tripping this for any future dependency changes

Can we get them working? (Though I don't think sdl2 runs them either, so 🤷‍♀️)

Two are still broken, but I've left the option there for when we eventually figure out how to

@getchoo getchoo requested a review from OPNA2608 November 3, 2024 04:04
@OPNA2608
Copy link
Contributor

OPNA2608 commented Nov 3, 2024

Is there any example project similar to what you're compiling here

What I was compiling there was the SDL3 package.

any […] project […] we could add to passthru.tests? Would probably help in not tripping this for any future dependency changes

I know that shadps4 needs SDL3, but it's pinning 3.1.2. With the "daily-changing API", not sure if removing the pin would break something. It doesn't have a VM test to check working SDL3 functionality, but one could be written to install & run an open toolchain's hello world example and wait for the text output.

SDL_EXAMPLES should build the examples, and SDL_INSTALL_EXAMPLES (this is missing an option() declaration?, but is being used here) should install them to $out/libexec/installed-examples/SDL3. Some of them might have on-screen text, maybe a basic VM test could be written that tries to run one of them & waits for working text rendering? Better than nothing maybe.

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Nov 3, 2024
@marcin-serwin
Copy link
Contributor

marcin-serwin commented Nov 3, 2024

There are some changes needed to make it work with wayland:

  • wayland-scanner is a build time dependency, it's not needed at runtime
  • libxkbcommon is not needed by X11 since it has its own XKB implementation, but it's needed by wayland
  • SDL_ShowMessageBox fails with posix_spawn() failed: No such file or directory if it doesn't find zenity in the PATH, we should patch it to point to the store path
Patch with the above changes
diff --git a/pkgs/by-name/sd/sdl3/package.nix b/pkgs/by-name/sd/sdl3/package.nix
index cff003e1802f..505272cca80e 100644
--- a/pkgs/by-name/sd/sdl3/package.nix
+++ b/pkgs/by-name/sd/sdl3/package.nix
@@ -28,6 +28,7 @@
   wayland,
   wayland-scanner,
   xorg,
+  zenity,
   alsaSupport ? stdenv.hostPlatform.isLinux && !stdenv.hostPlatform.isAndroid,
   dbusSupport ? stdenv.hostPlatform.isLinux && !stdenv.hostPlatform.isAndroid,
   drmSupport ? stdenv.hostPlatform.isLinux && !stdenv.hostPlatform.isAndroid,
@@ -68,10 +69,11 @@ stdenv.mkDerivation (finalAttrs: {
     cmake
     ninja
     validatePkgConfig
-  ];
+  ] ++ lib.optional waylandSupport wayland-scanner;
 
   buildInputs =
     finalAttrs.dlopenBuildInputs
+    ++ lib.optional waylandSupport zenity
     ++ lib.optionals ibusSupport [
       fcitx5
       ibus
@@ -106,10 +108,9 @@ stdenv.mkDerivation (finalAttrs: {
     ++ lib.optional sndioSupport sndio
     ++ lib.optionals waylandSupport [
       wayland
-      wayland-scanner
+      libxkbcommon
     ]
     ++ lib.optionals x11Support [
-      libxkbcommon
       xorg.libX11
       xorg.libXScrnSaver
       xorg.libXcursor
@@ -125,6 +126,11 @@ stdenv.mkDerivation (finalAttrs: {
     lib.optional (openglSupport && !stdenv.hostPlatform.isDarwin) libGL
     ++ lib.optional x11Support xorg.libX11;
 
+  postPatch = lib.optionalString waylandSupport ''
+    substituteInPlace src/video/wayland/SDL_waylandmessagebox.c \
+      --replace-fail '"zenity"' '"${zenity}/bin/zenity"'
+  '';
+
   cmakeFlags = [
     (lib.cmakeBool "SDL_ALSA" alsaSupport)
     (lib.cmakeBool "SDL_DBUS" dbusSupport)

@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Nov 4, 2024
@getchoo getchoo requested a review from OPNA2608 November 4, 2024 05:47
@getchoo
Copy link
Member Author

getchoo commented Nov 4, 2024

The snake game example is now built as a separate derivation to test our RPATH hacks, and then run in a VM test

@getchoo
Copy link
Member Author

getchoo commented Jan 2, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 326699


aarch64-linux

✅ 3 packages built:
  • sdl3
  • sdl3.dev
  • sdl3.lib

aarch64-darwin

✅ 3 packages built:
  • sdl3
  • sdl3.dev
  • sdl3.lib

@getchoo getchoo added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 2, 2025
@Green-Sky
Copy link
Contributor

They cut a new preview release https://github.com/libsdl-org/SDL/releases/tag/preview-3.1.8 which adds tray icon support. It currently requires libayatana-appindicator or libappindicator-gtk3 on linux.

@getchoo getchoo removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 8, 2025
@getchoo getchoo requested a review from marcin-serwin January 8, 2025 16:58
@getchoo getchoo changed the title sdl3: init at 3.1.6 sdl3: init at 3.1.8 Jan 8, 2025
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1-10 labels Jan 8, 2025
@github-actions github-actions bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Jan 8, 2025
@getchoo
Copy link
Member Author

getchoo commented Jan 8, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 326699


aarch64-linux

✅ 3 packages built:
  • sdl3
  • sdl3.dev
  • sdl3.lib

x86_64-darwin

✅ 3 packages built:
  • sdl3
  • sdl3.dev
  • sdl3.lib

aarch64-darwin

✅ 3 packages built:
  • sdl3
  • sdl3.dev
  • sdl3.lib

Copy link
Contributor

@OPNA2608 OPNA2608 left a comment

Choose a reason for hiding this comment

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

Let's complete the list of checked platforms:

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 326699


x86_64-linux

✅ 3 packages built:
  • sdl3
  • sdl3.dev
  • sdl3.lib

Also built sdl3.passthru.tests.

At this point, I think we should just send this.

@getchoo getchoo added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 12, 2025
@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Jan 12, 2025
@Green-Sky
Copy link
Contributor

And another preview release 😄 . This time they are calling it the first release candidate.
https://github.com/libsdl-org/SDL/releases/tag/prerelease-3.1.10

@getchoo
Copy link
Member Author

getchoo commented Jan 20, 2025

At this point it's been almost 6 months, so honestly I'm fine landing this without being on the latest tag and making a follow up, as we've already tested the current rev. Committers feel free to merge :)

If no one does, I'll try to bump it in the next couple days and go through the testing/nixpkgs-review process again

@pbsds
Copy link
Member

pbsds commented Jan 20, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 326699


x86_64-linux

✅ 3 packages built:
  • sdl3
  • sdl3.dev
  • sdl3.lib

aarch64-linux

✅ 3 packages built:
  • sdl3
  • sdl3.dev
  • sdl3.lib

x86_64-darwin

✅ 3 packages built:
  • sdl3
  • sdl3.dev
  • sdl3.lib

aarch64-darwin

✅ 3 packages built:
  • sdl3
  • sdl3.dev
  • sdl3.lib

Copy link
Contributor

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

Let's move on with this. We will be able to fix eventual issues in follow-up PRs.

@GaetanLepage GaetanLepage merged commit 0cda476 into NixOS:master Jan 20, 2025
25 checks passed
@nixpkgs-ci
Copy link
Contributor

nixpkgs-ci bot commented Jan 20, 2025

Successfully created backport PR for release-24.11:

@getchoo getchoo deleted the pkgs/sdl3/init branch January 20, 2025 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 2 This PR was reviewed and approved by two reputable people backport release-24.11 Backport PR automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package request: libSDL3
9 participants