Fix FindSDL to work with SDL built with MSVC and CMake

[sthalik]

Autodetection of GL context type in src/Magnum/Platform/CMakeLists.txt fails due to OS flags not being set. This should be hit, but isn't:

        elseif(CORRADE_TARGET_WINDOWS AND (NOT MAGNUM_TARGET_GLES OR MAGNUM_TARGET_DESKTOP_GLES))
            set(NEED_WGLCONTEXT 1)
            set(MagnumSomeContext_OBJECTS $<TARGET_OBJECTS:MagnumWglContextObjects>)

This results in a build error:

MagnumSdl2Application.lib(Sdl2Application.cpp.obj) : error LNK2001: unresolved external symbol flextGLInit

The solution is to manually enable WITH_WGLCONTEXT at build time for Magnum. But this shouldn't happen, as context type is an implementation detail for Magnum and isn't mentioned anywhere inside the documentation?

[mosra]

Hi,

This is strange. Yes, you're right, this is an implementation detail and as such you should only need to enable WITH_SDL2APPLICATION to get the Sdl2Application to compile and link correctly.

Is there something special about your setup? What CMake flags do you use to build Corrade and Magnum? Is #define CORRADE_TARGET_WINDOWS present in the generated/installed Corrade/configure.h?

[sthalik]

Found it. I've been setting up all the _CORRADE internal variables manually, and set _CORRADE_CONFIGURE_FILE to the wrong value. The FindCorrade script actually uses CORRADE_DIR, but doesn't create it as a cache variable on its own. I see that FindSDL2 bundled with Magnum has the same approach.

The FindSDL2 script also has the issue that when SDL2_DIR is set, SDL2_DLL_{DEBUG,RELEASE} doesn't get set even on shared SDL builds.

Please expose set(CORRADE_DIR "" CACHE PATH "") at the beginning of the FindCorrade script for the next guy who encounters the same problem.

[mosra]

If you set CMAKE_PREFIX_PATH to where Corrade (or SDL, or any other dependency) is installed, that should work too -- this is the standard variable that works everywhere without the Find module having to do anything extra.

[mosra]

Since CMake 3.12, <PpackageName>_ROOT seems to be the new designated builtin way to specify per-package prefixes.

It needs a CMake policy enabled to work, I can push the needed changes. Then you should get the expected behavior either by setting CMAKE_PREFIX_PATH to where Corrade/SDL/... is installed, or by setting separate Corrade_ROOT, SDL2_ROOT, ... variables instead. Does that sound good?

[sthalik]

It's fine with _ROOT, as long as these cache variables initialize themselves to empty values and thus are shown in cmake-gui in the first place.

Using -DCMAKE_PREFIX_PATH:PATH=f:/dev/magnum/install works with both Corrade and SDL2, except that SDL2_DLL_RELEASE doesn't get set.

[mosra]

as long as these cache variables initialize themselves to empty values and thus are shown in cmake-gui in the first place

Actually, that's what I wanted to avoid by using a builtin CMake feature -- to not have to update twenty or how many Find modules :sweat_smile: But maybe CMake can do that on its own, I'll investigate. For Config modules CMake auto-creates cache <PackageName>_DIR entries so maybe it could do that here too.

except that SDL2_DLL_RELEASE doesn't get set.

If you tell me how the SDL directory you have is organized, I can fix that. Currently the Find module is adapted to layout of the official MSVC and MinGW binary distribution (the ZIP file from the website), so if you have something else it may not work.

[sthalik]

It doesn't create FOODIR. My normal workflow is repeatedly generating, looking for errors and filling up `FOO{DIR,ROOT}` in ungrouped entries. Hence the insistence upon exposing the cache variable.

If you tell me how the SDL directory you have is organized, I can fix that.

This is a shared MSVC build of SDL2 straight from their repo:

dev/magnum/install % ls -ld cmake/SDL2Config.cmake bin/SDL2.dll lib/SDL2*
-rwxr-xr-x 1 sthalik None 1956352 Feb 16 13:40 bin/SDL2.dll
-rw-r--r-- 1 sthalik None    5385 Feb 16 13:30 cmake/SDL2Config.cmake
-rw-r--r-- 1 sthalik None  174356 Feb 16 13:39 lib/SDL2.lib
-rw-r--r-- 1 sthalik None  174678 Feb 16 13:40 lib/SDL2main.lib

[mosra]

It doesn't

It does, as the documentation says, when find_package() goes through a CMake Config file instead of a Find module. Which for various reasons isn't the case for Corrade or Magnum, so you don't get Corrade_DIR.

For SDL, would this simple patch make the DLL found?

diff --git a/modules/FindSDL2.cmake b/modules/FindSDL2.cmake
index 800d26457..311b05211 100644
--- a/modules/FindSDL2.cmake
+++ b/modules/FindSDL2.cmake
@@ -168,10 +168,10 @@ find_path(SDL2_INCLUDE_DIR
 if(CORRADE_TARGET_WINDOWS)
     find_file(SDL2_DLL_RELEASE
         NAMES SDL2.dll
-        PATH_SUFFIXES ${_SDL2_RUNTIME_PATH_SUFFIX} ${_SDL2_LIBRARY_PATH_SUFFIX})
+        PATH_SUFFIXES bin ${_SDL2_RUNTIME_PATH_SUFFIX} ${_SDL2_LIBRARY_PATH_SUFFIX})
     find_file(SDL2_DLL_DEBUG
         NAMES SDL2d.dll # not sure?
-        PATH_SUFFIXES ${_SDL2_RUNTIME_PATH_SUFFIX} ${_SDL2_LIBRARY_PATH_SUFFIX})
+        PATH_SUFFIXES bin ${_SDL2_RUNTIME_PATH_SUFFIX} ${_SDL2_LIBRARY_PATH_SUFFIX})
 endif()

 # (Static) macOS / iOS dependencies. On macOS these were mainly needed when

Oh and I see SDL2Config.cmake there, that must be rather new -- their CMake support improved only quite recently. I'll check if I can support that as well (and then you get the SDL2_DIR).

[sthalik]

The patch is correct. Also, the SDL2d.dll name seems to be correct for MSVC.

[mosra]

Just for the record (forgot to update this issue), the patch was merged as 317d67fca6ce186034caaa98ee464c11a3243d9c. I still need to look into using the SDL2Config.cmake inside FindSDL.