Ensure CMake finds SDL 2.0
ClosedPublic
Actions

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

Details

Reviewers

Campbell Barton (campbellbarton)
Sergey Sharybin (sergey)

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

Sybren A. Stüvel (sybren) updated this revision to Diff 3540.Feb 15 2015, 7:48 PM

Sybren A. Stüvel (sybren) retitled this revision from to Ensure CMake finds SDL 2.0.

Sybren A. Stüvel (sybren) updated this object.

Sybren A. Stüvel (sybren) added reviewers: Sergey Sharybin (sergey), Campbell Barton (campbellbarton).

Sybren A. Stüvel (sybren) added a project: BF Blender.

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).

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).

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.

Changed comment.

Sergey Sharybin (sergey) added inline comments.Feb 16 2015, 7:33 AM

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()