Created attachment 249962 [details] Patch for sdl20 * Switch to CMake, Autotools is removed upstream and brings us closer to other distros * Use GITHUB as primary site and upstream website as fallback * Use helpers for DISTNAME * Remove i386/AMD optimization logic and rely on CPUTYPE from the framework * Permanently disable NAS * Always use on iconv from libc * Always build with pthread support * Sort _DESC and remove redundant entries * Merge OPENGLES1 and 2 into one option (this is how upstream handles it) * Make use of helpers whenever possible * Drop _m_prefetch patch, merged with LLVM in 2015 Reference: https://github.com/llvm/llvm-project/issues/24022 Note, modeled after multimedia/ffmpeg for readability and consistency Poudriere testport OK 13.2-RELEASE, 14.0-RELEASE amd64 Poudriere testport OK 14.0-RELEASE i386 Poudriere bulk OK (list taken from freshports for devel/sdl20) on 13.2-RELEASE amd64
Nice work, thank you! I'd like to merge it piecewise, starting with options refactoring, _m_prefetch, MASTER_SITES and pthreads bits. Some questions/requests related to other parts: > Use helpers for DISTNAME Please don't, this is helpers overuse and needless cunning. > Remove i386/AMD optimization logic and rely on CPUTYPE from the framework Doesn't this mean unoptimized packages? > Permanently disable NAS Why? Not that I'm against it, but is there a specific reason to remove it from SDL2 and not from the ports completely? > Always use on iconv from libc Is this safe? > Make use of helpers whenever possible Please don't do this for armv[67] hack, I prefer this hack to be obvious and consolidated. > OPTIONS_EXCLUDE_amd64= ALTIVEC Why invert logic while altivec is specific to powerpc as far as I know? > echo -I@includedir@ -I@includedir@/SDL2 @SDL_CFLAGS@ Are you sure this is needed, do many ports fail to build without it? Yes it maintains status quo, but bringing an extra include directory (expecially with -I instead of -isystem) is a bad idea and should be avoided.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=83b0faf494372afd417a9103858b6b1f2b9b48b8 commit 83b0faf494372afd417a9103858b6b1f2b9b48b8 Author: Dmitry Marakasov <amdmi3@FreeBSD.org> AuthorDate: 2024-04-15 16:30:02 +0000 Commit: Dmitry Marakasov <amdmi3@FreeBSD.org> CommitDate: 2024-04-15 16:58:54 +0000 devel/sdl20: always enable pthreads support PR: 278353 Submitted by: diizzy devel/sdl20/Makefile | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=2fb0fe4b746ad8c19574907f815dca14675fb7f4 commit 2fb0fe4b746ad8c19574907f815dca14675fb7f4 Author: Dmitry Marakasov <amdmi3@FreeBSD.org> AuthorDate: 2024-04-15 16:22:23 +0000 Commit: Dmitry Marakasov <amdmi3@FreeBSD.org> CommitDate: 2024-04-15 16:58:53 +0000 devel/sdl20: drop no longer needed _m_prefetch patch Required support aws merged with LLVM in 2015: https://github.com/llvm/llvm-project/issues/24022 The port builds fine, including with CPUTYPE=athlon64-sse3 PR: 194861, 278353 [1] Submitted by: diizzy [1] devel/sdl20/Makefile | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=218f4cff478575e8ae77193d39c871426f2438af commit 218f4cff478575e8ae77193d39c871426f2438af Author: Dmitry Marakasov <amdmi3@FreeBSD.org> AuthorDate: 2024-04-15 16:13:18 +0000 Commit: Dmitry Marakasov <amdmi3@FreeBSD.org> CommitDate: 2024-04-15 16:58:53 +0000 devel/sdl20: refactor options (sort and remove redundant DESCs) PR: 278353 Submitted by: diizzy devel/sdl20/Makefile | 83 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 52 insertions(+), 31 deletions(-)
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=dec1710e77ae9e4cf8b0954fa53afda96a9e1cd9 commit dec1710e77ae9e4cf8b0954fa53afda96a9e1cd9 Author: Dmitry Marakasov <amdmi3@FreeBSD.org> AuthorDate: 2024-04-15 16:27:46 +0000 Commit: Dmitry Marakasov <amdmi3@FreeBSD.org> CommitDate: 2024-04-15 16:58:54 +0000 devel/sdl20: add (and prefer) github MASTER_SITE PR: 278353 Submitted by: diizzy devel/sdl20/Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
(In reply to Dmitry Marakasov from comment #1) > Use helpers for DISTNAME > Please don't, this is helpers overuse and needless cunning. Fine by me, it's a style preferrence > Remove i386/AMD optimization logic and rely on CPUTYPE from the framework > Doesn't this mean unoptimized packages? Unless the port supports runtime detection trying to "outsmart" ports framework causes more issues and inconsistencies than it's worth. We're slowly but steadily working towards moving away from manually defining (SIMD) features and instead relying on CPUTYPE in the tree however some ports are more troublesome than others. As for this specific port amd64 (for example) covers all "stock" optimizations however if it were to use lets say AVX you'd run into issues such as the build box supporting while target might not. There's also for "troublesome" upstream projects https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=272900 but it's wip/being discussed (not related to this one). A few major Linux distros are looking into solving this by proving different package repos depending on baseline of target system. https://en.wikipedia.org/wiki/X86-64#Microarchitecture_levels There are currently no plans of doing this for FreeBSD however we do support using CPUTYPE and Poudriere if you want a "better" optimized repo for your clients. fwiw, we do ship quite a few ports with less than ideal performance characteristics (optimization) because of compatibility reasons. > Permanently disable NAS > Why? Not that I'm against it, but is there a specific reason to remove it from SDL2 and not from the ports completely? While not "dead" we have 2 users of it in tree and it wouldn't hurt if we could reduce the amount of options to make it easier to maintain and for users. If you feel there's a benefit of keeping it feel free to do so. > Always use on iconv from libc > Is this safe? Upstream does this so I would assume yes given it's the default on 2.30 and master branch. https://github.com/libsdl-org/SDL/blob/release-2.30.2/CMakeLists.txt#L429 https://github.com/libsdl-org/SDL/blob/main/CMakeLists.txt#L299 > OPTIONS_EXCLUDE_amd64= ALTIVEC > Why invert logic while altivec is specific to powerpc as far as I know? A "quirk" with the framework, it's not disabled otherwise on other platforms. > echo -I@includedir@ -I@includedir@/SDL2 @SDL_CFLAGS@ This caused about 20 ports or so to fail and mimmicks GNU Autotools. Ideally we should ditch sdl2-config and rely on pkgconfig or cmake but most of these ports are ancient and/or dead upstream. Best regards, Daniel
Regarding CPUTYPE, This probably explains it better https://stackoverflow.com/questions/28939652/how-to-detect-sse-sse2-avx-avx2-avx-512-avx-128-fma-kcvi-availability-at-compile and is how SDL uses it.
>> Doesn't this mean unoptimized packages? > ... I'm aware of build-time CPU features detection issues and consequences, the question is about how it's better to handle these here and not cause a performance regression. However after some investigation I'm taking this question down: - I was afraid that knobs affected not only compiler optimizations but whether corresponding intrinsics could be used, in which case we could not enable all optimizations by relying on CPUTYPE solely. That's turned out not to be the case: cmake knobs only add -m flags, and C code looks at preprocessor defines such __MMX__, which are set by compiler based on -march. - I've assumed that current code enabled some base optimizations and we're causing a regression by disabling these, but looking at [build log](http://beefy11.nyi.freebsd.org/data/140i386-default/ff89313922da/logs/sdl2-2.28.5.log) (Assembly math line) it's not the case either - no optimizations are currently enabled. So your solution is good. I wonder though - should we consider enabling at least mmx on i386? - wouldn't it be simpler to just disable ASSEMBLY? (not unless we want to handle altivec manually I guess) - shouldn't altivec be handled similarly? > A "quirk" with the framework, it's not disabled otherwise on other platforms. I don't understand. It should only be available on power. It is only enabled on power as it is. > While not "dead" we have 2 users of it in tree and it wouldn't hurt if we could reduce the amount of options to make it easier to maintain and for users. If you feel there's a benefit of keeping it feel free to do so. There are 9 consumers in fact (8 of which are optional). As much as I'd love to remove support for ancient garbage this just doesn't feel right to do it out of blue while refactoring the port. Would happily drop it from both sdl1 and 2 if there's general decision to drop nas support. > Upstream does this so I would assume yes given it's the default on 2.30 and master branch. > https://github.com/libsdl-org/SDL/blob/release-2.30.2/CMakeLists.txt#L429 > https://github.com/libsdl-org/SDL/blob/main/CMakeLists.txt#L299 The question was about whether it works as expected when ports iconv is also installed. > This caused about 20 ports or so to fail and mimmicks GNU Autotools. Ideally we should ditch sdl2-config and rely on pkgconfig or cmake but most of these ports are ancient and/or dead upstream. Could you please identify these ports? Would be nice to fix them later and remove the hack of adding extra flags.
I'm actually not sure what baseline we're targetting on i386 but MMX seems reasonable. Just test if CPUTYPE is defined otherwise set -march=pentium-mxx on i386. If you only define ALTIVEC for ppc it'll be unset for every other platform, at least the CMake script figures out it's not applicable but will still report it as requested if you look at logs. While it works it' confusing and may lead to other issues further down the line. Regarding NAS I just looked at freshports.org, there are indeed a few ports offering it as an option. As far as I can tell at least the CMake logic figures out iconv support fine. What I found running my "mini-exp" of ports not working, games/jfsw (fails indirectly because of missing GL/gl.h) emulators/mesen audio/ft2play games/el (fails indirectly because of missing GL/gl.h) games/gigalomania games/uqm (fails indirectly because of missing png.h) games/neverball (fails indirectly because of missing GL/gl.h) emulators/jzintv games/abbayedesmorts games/trigger-rally games/OpenLara games/blues (fails indirectly because of missing libmodplug/modplug.h) games/legend-of-edgar emulators/vt100 (fails indirectly because of missing GL/gl.h) Best regards, Daniel
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=c14f535ae56adee7545d94077b4e04f2884c2137 commit c14f535ae56adee7545d94077b4e04f2884c2137 Author: Dmitry Marakasov <amdmi3@FreeBSD.org> AuthorDate: 2024-05-07 18:22:18 +0000 Commit: Dmitry Marakasov <amdmi3@FreeBSD.org> CommitDate: 2024-05-14 01:44:10 +0000 devel/sdl20: switch to cmake PR: 278353 Submitted by: diizzy devel/sdl20/Makefile | 194 ++++++++++----------------- devel/sdl20/files/patch-CMakeLists.txt (new) | 49 +++++++ devel/sdl20/files/patch-sdl2-config.in (new) | 12 ++ devel/sdl20/pkg-plist | 15 ++- 4 files changed, 148 insertions(+), 122 deletions(-)