Skip to content
This repository has been archived by the owner on May 13, 2024. It is now read-only.

Don't link against SDL2main #265

Merged
merged 3 commits into from
Feb 23, 2024
Merged

Don't link against SDL2main #265

merged 3 commits into from
Feb 23, 2024

Conversation

rollerozxa
Copy link
Member

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
Copy link
Member

sfan5 commented Dec 19, 2023

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

@numberZero
Copy link
Contributor

Does it work on MSVC?

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

@rollerozxa
Copy link
Member Author

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
Copy link
Member

sfan5 commented Jan 17, 2024

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
Copy link
Member Author

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 rollerozxa closed this Feb 8, 2024
@rollerozxa rollerozxa reopened this Feb 10, 2024
@rollerozxa
Copy link
Member Author

rollerozxa commented Feb 10, 2024

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
Copy link
Contributor

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

@rollerozxa
Copy link
Member Author

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
Copy link
Contributor

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 8482cc3 might be related.

@sfan5
Copy link
Member

sfan5 commented Feb 18, 2024

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

@sfan5
Copy link
Member

sfan5 commented Feb 23, 2024

result: working.

@sfan5 sfan5 merged commit 2bbfa17 into minetest:master Feb 23, 2024
19 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants