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

Resolve WinMain linking issues with CMake #83

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ccawley2011
Copy link
Contributor

@@ -1,4 +1,5 @@
#ifdef _WIN32
#define SDL_MAIN_HANDLED
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to add a call to SDL_SetMainReady in main/WinMain.
See https://wiki.libsdl.org/SDL_SetMainReady

Copy link
Contributor

@madebr madebr Jul 5, 2021

Choose a reason for hiding this comment

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

I just checked the implementation of SDL_SetMainReady.
It looks like SDL2main is needed (for some platforms).
On e.g. Windows, it does something: https://github.com/libsdl-org/SDL/blob/main/src/main/windows/SDL_windows_main.c

Other platforms also use it: https://github.com/libsdl-org/SDL/tree/main/src/main

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only seems to be needed on platforms that define SDL_MAIN_NEEDED, while Windows defines SDL_MAIN_AVAILABLE instead. For Windows, what SDL2main provides is equivalent to what both the skeleton and MinGW already provide, so calling SDL_SetMainReady shouldn't be necessary on Windows.

@@ -29,6 +28,8 @@ if(NOT SDL2_FOUND)
endif()
endif()

find_library(SDL2main_LIBRARY SDL2main)
Copy link
Contributor

@madebr madebr Jul 5, 2021

Choose a reason for hiding this comment

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

We don't use the SDL2main library, so perhaps all logic related to SDL2main can be removed from this cmake script?

premake5.lua Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/d3d/rwd3d.h Outdated Show resolved Hide resolved
@ccawley2011 ccawley2011 marked this pull request as ready for review July 5, 2021 21:09
@ccawley2011 ccawley2011 changed the title Attempt to resolve WinMain linking issues with CMake Resolve WinMain linking issues with CMake Jul 5, 2021
@madebr
Copy link
Contributor

madebr commented Jul 5, 2021

I created a branch on my fork that adds mingw CI with github actions: https://github.com/madebr/librw/tree/ga_mingw
Github actions result at https://github.com/madebr/librw/actions/runs/1002476846
It has the behavior described in your initial comment: glfw/SDL2 works, d3d9 does not.

I find it weird that glfw works while d3d9 does not.
Their respective librw_skeletonlibraries are both providing a WinMain symbol in a static library.
What is different between them?

@ccawley2011
Copy link
Contributor Author

I find it weird that glfw works while d3d9 does not.
Their respective librw_skeletonlibraries are both providing a WinMain symbol in a static library.
What is different between them?

I think it's because the GLFW backend has a standard main() function that can be used with MinGW's default WinMain implementation, while the Win32 backend doesn't have one and the one in the SDL backend gets renamed when SDL_MAIN_HANDLED is not defined.

@madebr
Copy link
Contributor

madebr commented Jul 5, 2021

@ccawley2011
Please re-check my ga_mingw branch at https://github.com/madebr/librw/tree/ga_mingw
I got it working in a (so I believe) cleaner way by using linker options.
github action results at https://github.com/madebr/librw/actions/runs/1002584126

@ccawley2011
Copy link
Contributor Author

@ccawley2011
Please re-check my ga_mingw branch at https://github.com/madebr/librw/tree/ga_mingw
I got it working in a (so I believe) cleaner way by using linker options.
github action results at https://github.com/madebr/librw/actions/runs/1002584126

This seems to work with the 64-bit mingw-w64 toolchain, but not with MinGW32 or the 32-bit mingw-w64 toolchain.

@madebr
Copy link
Contributor

madebr commented Jul 7, 2021

@ccawley2011
Please re-check my ga_mingw branch at https://github.com/madebr/librw/tree/ga_mingw
I got it working in a (so I believe) cleaner way by using linker options.
github action results at https://github.com/madebr/librw/actions/runs/1002584126

This seems to work with the 64-bit mingw-w64 toolchain, but not with MinGW32 or the 32-bit mingw-w64 toolchain.

I got it working for all configs.
See https://github.com/madebr/librw/actions/runs/1006420251 for the logs.
I also added the ability to switch between static and shared runtime (this also includes static SDL2/glfw).
Is it okay for you if I open a pr with these commits (I have to rewrite the commit history)?

@ccawley2011
Copy link
Contributor Author

I got it working for all configs.
See https://github.com/madebr/librw/actions/runs/1006420251 for the logs.
I also added the ability to switch between static and shared runtime (this also includes static SDL2/glfw).

Thanks, I've tested the new changes and it works for me as well. I just have one additional suggested change for building with clang:

index 7c7c27c..572e4b2 100644
--- a/cmake/UseStaticRuntime.cmake
+++ b/cmake/UseStaticRuntime.cmake
@@ -6,6 +6,8 @@ endif()

 if(CMAKE_C_COMPILER_ID MATCHES "^GNU$")
     set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -static-libgcc -static-libstdc++ -static")
+elseif (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
+    set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -static")
 endif()

 if(WIN32)

Is it okay for you if I open a pr with these commits (I have to rewrite the commit history)?

Sure, go ahead.

@madebr
Copy link
Contributor

madebr commented Jul 7, 2021

+    set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -static")

Thanks. I wasn't sure whether -static also meant static libgcc and libstdc++

Is it okay for you if I open a pr with these commits (I have to rewrite the commit history)?

Sure, go ahead.

I will post it in the coming days and tag you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants