search

minetest / irrlicht

Minetest's fork of Irrlicht
Other
115 stars 87 forks source link

Don't link against SDL2main #265

Closed rollerozxa closed 4 months ago

rollerozxa commented 6 months ago

This PR replaces ${SDL2_LIBRARIES} with just SDL2::SDL2, so that it doesn't link against SDL2main. When building Minetest on Windows (MSYS2) it fails during linking the executable with the following message:

C:/msys64/ucrt64/bin/../lib/gcc/x86_64-w64-mingw32/13.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:/msys64/ucrt64/lib/libSDL2main.a(SDL_windows_main.c.obj):(.text+0x17a): undefined reference to `SDL_main'

Removing the link against SDL2main fixes this and it builds and runs successfully. I'm unsure if there's any side effects to removing it on some platform, but it doesn't seem to be necessary on Windows and Linux at the very least.

sfan5 commented 6 months ago

are you sure that's correct? https://irc.minetest.net/minetest-dev/2023-12-18#i_6140291

numberZero commented 6 months ago

Does it work on MSVC?

Also, you don’t need SDL2_INCLUDE_DIRS, SDL2::SDL2 implies that (unlike SDL2_LIBRARIES).

rollerozxa commented 6 months ago

I don't know if it is correct and doesn't break other platforms, but it does fix building with SDL2 enabled on Windows for me. I have not tested how it works with MSVC, only MSYS2. And I tested building on Linux (native) with SDL2main not linked and that works there.

It would be great if CI could test building Minetest on all platforms when IrrlichtMt is changed, but as long as it's still split like this that's not gonna be possible.

sfan5 commented 5 months ago

I don't know what's special about MSYS2 but at least with our MinGW toolchain this change isn't needed. (Didn't check if it breaks anything.)

rollerozxa commented 4 months ago

I fixed this by including SDL.h in Minetest's main.c file, this appears to be what SDL2 recommends to fix it. Closing this PR, will open another one in Minetest.

rollerozxa commented 4 months ago

Apparently including SDL headers in Minetest is a bad idea so I reopened this PR.

The only platforms we would reasonably support that use SDL2main for initialisation is Windows and Haiku. Windows appears to not require it as it works fine not being wrapped, but no idea about Haiku.

(Also SDL3 does not have this "SDLmain" wrapper library that it links against anymore, as they've moved the init stuff into headers)

appgurueu commented 4 months ago

Two Linux SDL builds are failing in CI; they're fine on master. Why is that?

rollerozxa commented 4 months ago

No idea, they were fine in the previous commit before I resolved merge conflicts. Has anything changed in IrrlichtMt regarding finding SDL2 during the time this PR was opened?

appgurueu commented 4 months ago

No idea, they were fine in the previous commit before I resolved merge conflicts. Has anything changed in IrrlichtMt regarding finding SDL2 during the time this PR was opened?

Yes, something like https://github.com/minetest/irrlicht/commit/8482cc3db8192107b2cfc15d41d5337716bb4349 might be related.

sfan5 commented 4 months ago

Note: before merging this I'd like to check that it doesn't break building MT with our current MinGW setup.

sfan5 commented 4 months ago

result: working.