Patches to potentially upstream from *BSD packages

[ccawley2011]

It might be worth incorporating some of the patches and workarounds included in the downstream packages for FreeBSD, DragonFly BSD, NetBSD and OpenBSD.

FreeBSD: https://cgit.freebsd.org/ports/tree/devel/sdl20/Makefile#n160 DragonFly BSD: https://github.com/DragonFlyBSD/DeltaPorts/tree/master/ports/devel/sdl20/dragonfly NetBSD: http://cvsweb.netbsd.org/bsdweb.cgi/pkgsrc/devel/SDL2/patches/ OpenBSD (provides version 2.0.20): http://cvsweb.openbsd.org/cgi-bin/cvsweb/ports/devel/sdl2/patches/

[slouken]

I'll review these, with a comment for each platform.

[slouken]

OpenBSD: http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-Makefile_in?rev=1.12&content-type=text/plain&hideattic=1

• Needs review (possibly by @smcv?) to understand the implication of this change. Definitely not desired at this point for all platforms.

http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-sdl2-config_cmake_in?rev=1.4&content-type=text/plain&hideattic=1

• Needs review (possibly by @madebr?) to know whether this is valid for current SDL. It's definitely not something we want for all platforms.

http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-sdl2-config_in?rev=1.3&content-type=text/plain&hideattic=1

• I'm not sure why the application should be linking with X libraries - it should be a hidden dependency of the SDL library, either dynamically linked or dynamically loaded at runtime.

http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-sdl2_pc_in?rev=1.5&content-type=text/plain&hideattic=1

• Again, I'm not sure why the application needs the X compiler flags and libraries when building with SDL.

http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-src_SDL_c?rev=1.9&content-type=text/plain&hideattic=1

• Trying to build without joystick support and then trying to initialize game controller support is intended to be a failure. I'm not sure what the use case is?
• Why would anyone override the platform with an environment variable?

http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-src_filesystem_unix_SDL_sysfilesystem_c?rev=1.4&content-type=text/plain&hideattic=1

• This should be replaced with something functional rather than just deleted. It would be helpful to know what's wrong here, possibly with an upstream bug report?

http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-src_joystick_SDL_gamecontrollerdb_h?rev=1.4&content-type=text/plain&hideattic=1

• The Linux entries make sense here, assuming OpenBSD's joystick stack is similar enough that they work across the board. The macOS entries definitely do not, as they're often different from the Linux ones for the same GUIDs. I would feel more comfortable with this patch if someone with OpenBSD tested a wide variety of controllers and vetted the entries.

http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-src_joystick_bsd_SDL_bsdjoystick_c?rev=1.5&content-type=text/plain&hideattic=1

• Switching from uhid* to ujoy/* shouldn't be done for all platforms. The rest of the changes seem reasonable, but the fact that the button being delivered is based on HID_USAGE definitely means the patch for SDL_gamecontrollerdb.h above won't work in general.

[slouken]

NetBSD: http://cvsweb.netbsd.org/bsdweb.cgi/~checkout~/pkgsrc/devel/SDL2/patches/patch-src_video_wayland_SDL__waylandmessagebox.c?rev=1.1&content-type=text/plain

• Yeah, that was silly. Fixed in https://github.com/libsdl-org/SDL/commit/21100006ad41876db53060dc754423e369cc5a9c

[slouken]

FreeBSD: https://cgit.freebsd.org/ports/tree/devel/sdl20/Makefile

• I'm not sure what's actionable here. The change from _m_prefetch to __builtin_prefetch seems like a good change, but should be guarded by preprocessor check for clang, and maybe some additional research should be done there. I'm curious if there's a way we should be improving our iconv check on FreeBSD - maybe we should be using something like https://github.com/gcc-mirror/gcc/blob/master/config/iconv.m4 ?

[slouken]

DragonFly BSD: https://github.com/DragonFlyBSD/DeltaPorts/blob/master/ports/devel/sdl20/dragonfly/patch-configure

• Yup, applied as https://github.com/libsdl-org/SDL/commit/6926ff3cd83c9fea544f1cc574df267b8a069a48

https://github.com/DragonFlyBSD/DeltaPorts/blob/master/ports/devel/sdl20/dragonfly/patch-include_SDL__endian.h

• Yup, applied as https://github.com/libsdl-org/SDL/commit/ce5a23bd57f0393d39c48a3cffcb90c24bc56dae

https://github.com/DragonFlyBSD/DeltaPorts/blob/master/ports/devel/sdl20/dragonfly/patch-src_joystick_linux_SDL__sysjoystick.c

• Already fixed in main

https://github.com/DragonFlyBSD/DeltaPorts/blob/master/ports/devel/sdl20/dragonfly/patch-src_stdlib_SDL__malloc.c

• Yep, applied as https://github.com/libsdl-org/SDL/commit/7f42fb54adbb9fa86bbdbd09abac628ccfaef648

[madebr]

Replying to the patches related to the build systems:

• http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-sdl2-config_cmake_in?rev=1.4&content-type=text/plain&hideattic=1

  Totally unwanted. They removed the creation of the imported targets, which I rewrote last May..

• http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-sdl2-config_in?rev=1.3&content-type=text/plain&hideattic=1

  The include part might be useful (-I@includedir@), as it aligns with this recent patch: https://github.com/libsdl-org/SDL/commit/45c1cc81771123a3a3a632e945dd4fd7a428c92b But I would be cautious with these patches, as it encourages people to include SDL using #include "SDL2/SDL.h instead of #include "SDL.h".

  As you've written, the X_LIBS part is unwanted.

[ccawley2011]

• Why would anyone override the platform with an environment variable?

The commit history for the patch suggests that it's needed for specific FNA games that check the current platform using this function but don't recognize OpenBSD, so this allows those games to work correctly by setting SDL_PLATFORM to Linux.

[sezero]

* > http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-sdl2-config_in?rev=1.3&content-type=text/plain&hideattic=1
  
  The include part might be useful (`-I@includedir@`), as it aligns with this recent patch: [45c1cc8](https://github.com/libsdl-org/SDL/commit/45c1cc81771123a3a3a632e945dd4fd7a428c92b)
  But I would be cautious with these patches, as it encourages people to include SDL using `#include "SDL2/SDL.h` instead of `#include "SDL.h"`.

Indeed: Agreed.

  As you've written, the `X_LIBS` part is unwanted.

Agreed.

[sezero]

• Why would anyone override the platform with an environment variable?

The commit history for the patch suggests that it's needed for specific FNA games that check the current platform using this function but don't recognize OpenBSD, so this allows those games to work correctly by setting SDL_PLATFORM to Linux.

Then it is not needed here in mainstream, yes?

[ccawley2011]

It might be worth asking @rfht as the OpenBSD package maintainer about the relevant patches.

[icculus]

• Why would anyone override the platform with an environment variable?

The commit history for the patch suggests that it's needed for specific FNA games that check the current platform using this function but don't recognize OpenBSD, so this allows those games to work correctly by setting SDL_PLATFORM to Linux.

Mentioning @flibitijibibo in case he wants to know about this.

[flibitijibibo]

Yep, this is part of fnaify: https://github.com/rfht/fnaify

FNA recognizes the BSDs but some older builds don't, so that's why that's in there. This is something you'll only see with managed code where we detect at runtime rather than build time, so this call ends up getting used for more than just logging.

[smcv]

OpenBSD: http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-Makefile_in?rev=1.12&content-type=text/plain&hideattic=1

• Needs review (possibly by @smcv?) to understand the implication of this change. Definitely not desired at this point for all platforms.

This is an ABI break and should not be applied upstream. It changes the SONAME from libSDL2-2.0.so.0 to libSDL2.so.0, and the realpath() of the actual library from something like libSDL2-2.0.so.0.2301.0 to libSDL2.so.0.2301.0. Downstreams should not change this sort of thing without talking to upstream first.

Removing -release is sort-of-correct in principle, and we should do that in SDL 3 (#5626), but it's years too late to do this in SDL 2.

If OpenBSD has always been making this change, then maybe it would be OK to do this conditionally (for OpenBSD but no other platforms), but even that is risky - it would be an ABI break for OpenBSD users who have been compiling SDL2 themselves (from the unpatched upstream version) rather than from ports (the patched downstream version).

As a general principle, I would encourage downstreams to send their changes upstream (as I do for things I fix in Debian), or if their changes are unsuitable for upstream, keep a record of why the change was made and why it isn't upstreamable (as I've done for the one non-upstreamable change in Debian).

[smcv]

I'm not sure why the application needs the X compiler flags and libraries when building with SDL

The better runtime dynamic linkers (such as glibc's) understand that shared libraries can have dependencies, so you only need to list the executable's direct dependencies for dynamic linking, with knowledge of its dependencies' dependencies (recursively) only required when linking statically. However, historically this was not true for all dynamic linkers: some needed the recursive dependencies (similar to glibc static linking) even when linking dynamically.

The libtool manual describes this problem with a note that "most shared library systems are restricted in that they only allow a single level of dependencies", which I'm sure was true in the era of 1990s proprietary Unix, but I hope is very much false in 2022.

I had hoped that all the modern BSDs behaved like glibc in this respect, but perhaps some of them still have the old/bad behaviour when linking dynamically?

[rfht]

It might be worth asking @rfht as the OpenBSD package maintainer about the relevant patches.

@flibitijibibo has already provided the rationale. Question has been discussed among OpenBSD fnaify gamers if there might be upstream interest in having an env var that can be used to work around these (often artificial and overly strict) platform limitations. fnaify has already been used a little on FreeBSD. Other less mainstream OS like Haiku could also run into this.

[brad0]

OpenBSD: http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-Makefile_in?rev=1.12&content-type=text/plain&hideattic=1

• Needs review (possibly by @smcv?) to understand the implication of this change. Definitely not desired at this point for all platforms.

http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-sdl2-config_cmake_in?rev=1.4&content-type=text/plain&hideattic=1

• Needs review (possibly by @madebr?) to know whether this is valid for current SDL. It's definitely not something we want for all platforms.

http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-sdl2-config_in?rev=1.3&content-type=text/plain&hideattic=1

• I'm not sure why the application should be linking with X libraries - it should be a hidden dependency of the SDL library, either dynamically linked or dynamically loaded at runtime.

http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-sdl2_pc_in?rev=1.5&content-type=text/plain&hideattic=1

• Again, I'm not sure why the application needs the X compiler flags and libraries when building with SDL.

http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-src_SDL_c?rev=1.9&content-type=text/plain&hideattic=1

• Trying to build without joystick support and then trying to initialize game controller support is intended to be a failure. I'm not sure what the use case is?
• Why would anyone override the platform with an environment variable?

http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-src_filesystem_unix_SDL_sysfilesystem_c?rev=1.4&content-type=text/plain&hideattic=1

• This should be replaced with something functional rather than just deleted. It would be helpful to know what's wrong here, possibly with an upstream bug report?

http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-src_joystick_SDL_gamecontrollerdb_h?rev=1.4&content-type=text/plain&hideattic=1

• The Linux entries make sense here, assuming OpenBSD's joystick stack is similar enough that they work across the board. The macOS entries definitely do not, as they're often different from the Linux ones for the same GUIDs. I would feel more comfortable with this patch if someone with OpenBSD tested a wide variety of controllers and vetted the entries.

http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/sdl2/patches/patch-src_joystick_bsd_SDL_bsdjoystick_c?rev=1.5&content-type=text/plain&hideattic=1

• Switching from uhid* to ujoy/* shouldn't be done for all platforms. The rest of the changes seem reasonable, but the fact that the button being delivered is based on HID_USAGE definitely means the patch for SDL_gamecontrollerdb.h above won't work in general.

Ignore almost all of those patches other than the gamecontroller and joystick bits. The rest we don't care about going upstream in any manner.

[brad0]

If OpenBSD has always been making this change, then maybe it would be OK to do this conditionally (for OpenBSD but no other platforms), but even that is risky - it would be an ABI break for OpenBSD users who have been compiling SDL2 themselves (from the unpatched upstream version) rather than from ports (the patched downstream version).

Don't worry about this part.

As a general principle, I would encourage downstreams to send their changes upstream (as I do for things I fix in Debian), or if their changes are unsuitable for upstream, keep a record of why the change was made and why it isn't upstreamable (as I've done for the one non-upstreamable change in Debian).

Personally I do try, especially more so in the last few years. But not all developers take the same initiative or have the same drive.

[slouken]

Ignore almost all of those patches other than the gamecontroller and joystick bits. The rest we don't care about going upstream in any manner.

Okay, I pulled in the game controller patches and did a bunch of cleanup, so the latest code should work much better. Please let me know if I broke anything!

[brad0]

I'm curious if there's a way we should be improving our iconv check on FreeBSD - maybe we should be using something like https://github.com/gcc-mirror/gcc/blob/master/config/iconv.m4 ?

I just noticed this affects us too. The autoconf macros pointed to look like they should resolve the issue.