Page MenuHome

Ensure CMake finds SDL 2.0
ClosedPublic

Authored by Sybren A. Stüvel (sybren) on Feb 15 2015, 7:48 PM.

Details

Summary

CMake doesn't search /usr/include/SDL2, which is the include directory for SDL 2.x on Ubuntu Linux (and possibly others). This results in SDL 1.2 headers being found when WITH_SDL_DYNLOAD=OFF (or none, if SDL 1.2 is not installed). Our shipped SDL 2.0 headers are used when WITH_SDL_DYNLOAD=ON. This patch ensures that in both
cases the correct SDL headers are used.

Both CMake 2.8 and the latest 3.2.0-rc1 skip /usr/include/SDL2, so increasing the minimum required CMake version won't help.

Diff Detail

Event Timeline

Where is this FindSDL2.cmake file from? its good to note where the upstream is if you didn't write yourself.

We could use as-is, assuming upstream are maintaining it. - but in that case best to add some comment so we know who upstream is.

Or we could modify for our own use.

  • Remove unused checks - mingw, references to solaris etc...
  • add SDL2_ROOT_DIR (instead of SDL2DIR, see how FindOpenEXR.cmake uses OPENEXR_ROOT_DIR)

While using someone elses module finders seems reasonable, we have an add-hoc template for our own (when CMake doesn't provide). and its quite lite (low maintenance). see build_files/cmake/Modules/FindSpacenav.cmake

This seems to bump SDL requirement to 2.0. Is there really a good reason for that?

@Campbell Barton (campbellbarton) the CMake module comes from https://dolphin-emu.googlecode.com/git/CMakeTests/FindSDL2.cmake. However, I'm also fine with a more light-weight, home-grown version. I'll address this in an updated patch.

@Sergey Sharybin (sergey) as far as I can see, the code already requires 2.0 when WITH_SDL_DYNLOAD=ON (which is the default for official releases). For example, sldew.c:1103 only accepts >= 2.0. I hadn't guessed that WITH_SDL_DYNLOAD would change the required SDL version.

@Sybren A. Stüvel (sybren), if this were from SDL or CMake projects - would be worth considering, but in this case would rather have own.

We can keep them fairly simple since they dont need to work on win32/osx.

Also re: REQUIRED, wouldnt it be better to omit that, and disable SDL2 if its not found? (as we do for many other libs).

Sybren A. Stüvel (sybren) updated this revision to Diff 3544.
  • Created new FindSDL2.cmake module based on FindSpacenav.cmake
  • Ensured that SDL 1.2 is used as before (i.e. when WITH_SDL_DYNLOAD=OFF and 2.0 is not available).
Campbell Barton (campbellbarton) accepted this revision.

LGTM (probably good to get Sergey's approval too)

CMakeLists.txt
847–850

not sure this comment is needed? - seems pretty obvious whats going on without it.

maybe something like...

# use same names for both versions of SDL until we move to 2.x

This revision is now accepted and ready to land.Feb 16 2015, 12:04 AM

@Campbell Barton (campbellbarton) re: REQUIRED: personally I think REQUIRED would be suitable, as CMake needs to be configured with WITH_SDL to hit that statement anyway. However, in the latest version of this patch I try to find both SDL and SDL2 packages to remain compatible with SDL 1.2, so neither gets the REQUIRED keyword.

Sybren A. Stüvel (sybren) updated this revision to Diff 3547.

Changed comment.

CMakeLists.txt
846

I guess it should be instead:

find_package_wrapper(SDL2)
if(SDL2_FOUND)
  # Here goes your code doe SDL2_FOO vs. SDL_FOO
else()
  find_package_wrapper(SDL)
endif()
Sergey Sharybin (sergey) accepted this revision.

Closed by commit 9a10b208bacd425