Skip to content

Commit

Permalink
Remove FindSDL2 find-module, use sdl2-config.cmake instead
Browse files Browse the repository at this point in the history 
This requires SDL >= 2.0.4.

Since <https://bugzilla.libsdl.org/show_bug.cgi?id=2464> was fixed in
SDL 2.0.4, SDL behaves as a CMake "config-file package", even if it was
not itself built using CMake: it installs a sdl2-config.cmake file to
${libdir}/cmake/SDL2, which tells CMake where to find SDL's headers and
library, analogous to a pkg-config .pc file.

As a result, we no longer need to copy/paste a "find-module package"
to be able to find a system copy of SDL >= 2.0.4 with find_package(SDL2).
Find-module packages are now discouraged by the CMake developers, in
favour of having upstream projects behave as config-file packages.

This results in a small API change: FindSDL2 used to set SDL2_INCLUDE_DIR
and SDL2_LIBRARY, but the standard behaviour for config-file packages is
to set <name>_INCLUDE_DIRS and <name>_LIBRARIES. Use the CONFIG keyword
to make sure we search in config-file package mode, and will not find a
FindSDL2.cmake in some other directory that implements the old interface.

In addition to deleting redundant code, this avoids some assumptions in
FindSDL2 about the layout of a SDL installation. The current libsdl2-dev
package in Debian breaks those assumptions; this is considered a bug
and will hopefully be fixed soon, but it illustrates how fragile these
assumptions can be. We can be more robust against different installation
layouts by relying on SDL's own CMake integration.

When linking to a copy of CMake in a non-standard location, users can
now set the SDL2_DIR or CMAKE_PREFIX_PATH environment variable to point
to it; previously, these users would have used the SDL2DIR environment
variable. This continues to be unnecessary if using matching system-wide
installations of CMake and SDL2, for example both from Debian.

Mitigates: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=951087
Signed-off-by: Simon McVittie <smcv@debian.org>
  • Loading branch information
@smcv @Razish
smcv authored and Razish committed Mar 9, 2020
1 parent 24d38be commit 5203023
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 188 deletions.
182 changes: 0 additions & 182 deletions CMakeModules/FindSDL2.cmake
View file

This file was deleted.

6 changes: 3 additions & 3 deletions code/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,9 @@ if(BuildSPEngine OR BuildJK2SPEngine)
${OpenJKLibDir}/SDL2/include
)
else()
find_package(SDL2 REQUIRED)
set(SPEngineIncludeDirectories ${SPEngineIncludeDirectories} ${SDL2_INCLUDE_DIR})
set(SPEngineLibraries ${SPEngineLibraries} ${SDL2_LIBRARY})
find_package(SDL2 REQUIRED CONFIG)
set(SPEngineIncludeDirectories ${SPEngineIncludeDirectories} ${SDL2_INCLUDE_DIRS})
set(SPEngineLibraries ${SPEngineLibraries} ${SDL2_LIBRARIES})
endif()

# Source Files
Expand Down
6 changes: 3 additions & 3 deletions codemp/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -396,9 +396,9 @@ if(BuildMPEngine)
${OpenJKLibDir}/SDL2/include
)
else()
find_package(SDL2 REQUIRED)
set(MPEngineIncludeDirectories ${MPEngineIncludeDirectories} ${SDL2_INCLUDE_DIR})
set(MPEngineLibraries ${MPEngineLibraries} ${SDL2_LIBRARY})
find_package(SDL2 REQUIRED CONFIG)
set(MPEngineIncludeDirectories ${MPEngineIncludeDirectories} ${SDL2_INCLUDE_DIRS})
set(MPEngineLibraries ${MPEngineLibraries} ${SDL2_LIBRARIES})
endif()

# EAX is Windows-Only (right?)
Expand Down

8 comments on commit 5203023

@ensiform
Copy link
Member

@ensiform ensiform commented on 5203023 Jul 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@smcv This doesn't work out of the box on Arch/Manjaro. (SDL 2.0.12) CMake 3.17.3

SDL2_INCLUDE_DIRS not set SDL2_LIBRARIES not set. Using SDL2::SDL2 works but this is not backwards compatible with most travis-ci versions of CMake.

What steps are required for making it detect the SDL2 installation with sdl2-config. There is no --help only vague usage.

SDL2_DIR is set to /usr/lib32or64/cmake/SDL2 and contains

SDL2Config.cmake SDL2ConfigVersion.cmake SDL2Targets-noconfig.cmake SDL2Targets.cmake

@smcv
Copy link
Contributor Author

@smcv smcv commented on 5203023 Jul 5, 2020

Choose a reason for hiding this comment

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

@ensiform Is this the distro's CMake and the distro's SDL package, or your own build of one or both of them?

If /usr/lib32or64/cmake is in the distro's CMake's search path, I would have expected that find_package(SDL2 REQUIRED CONFIG) should work without setting special environment variables.

Upstream SDL2 seems to install different files depending whether it was built with Autotools or CMake. Debian uses an Autotools build, which installs .../cmake/sdl2-config.cmake; yours seems to be a CMake build, which installs .../cmake/SDL2Config.cmake. However, find_package(SDL2) should find either of those interchangeably, so it should still work.

Maybe SDL2Config.cmake, and the files it includes, don't define the same variables as sdl2-config.cmake? If true, that seems like a bug, either in the packaging you're using or in SDL2 upstream.

SDL2_DIR is set to /usr/lib32or64/cmake/SDL2

If I'm reading the documentation correctly, I think you're meant to set SDL2_DIR to /usr? But I don't know whether it will understand lib32or64 unless your CMake has been specially patched to do so.

What steps are required for making it detect the SDL2 installation with sdl2-config

Current best-practice seems to be "don't"; pkg-config or the CMake module is preferred now. I think you'd have to reinstate a FindSDL2.cmake cargo-culted from other projects, which is now discouraged, and revert some or all of the changes I made in #1033.

@ensiform
Copy link
Member

@ensiform ensiform commented on 5203023 Jul 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's the official distro SDL2.

SDL2_DIR is correctly set to /usr/lib32/cmake/SDL2 if 32bit and lib64 if 64.

SDL2Config is just a one liner to include the targets file mentioned above and it does not seem to do anything with those variables. I'm not at home to paste the whole thing at this moment.

SDL2config.cmake:

include("${CMAKE_CURRENT_LIST_DIR}/SDL2Targets.cmake")

SDL2Targets.cmake:

# Generated by CMake

if("${CMAKE_MAJOR_VERSION}.${CMAKE_MINOR_VERSION}" LESS 2.5)
   message(FATAL_ERROR "CMake >= 2.6.0 required")
endif()
cmake_policy(PUSH)
cmake_policy(VERSION 2.6)
#----------------------------------------------------------------
# Generated CMake target import file.
#----------------------------------------------------------------

# Commands may need to know the format version.
set(CMAKE_IMPORT_FILE_VERSION 1)

# Protect against multiple inclusion, which would fail when already imported targets are added once more.
set(_targetsDefined)
set(_targetsNotDefined)
set(_expectedTargets)
foreach(_expectedTarget SDL2::SDL2 SDL2::SDL2main)
  list(APPEND _expectedTargets ${_expectedTarget})
  if(NOT TARGET ${_expectedTarget})
    list(APPEND _targetsNotDefined ${_expectedTarget})
  endif()
  if(TARGET ${_expectedTarget})
    list(APPEND _targetsDefined ${_expectedTarget})
  endif()
endforeach()
if("${_targetsDefined}" STREQUAL "${_expectedTargets}")
  unset(_targetsDefined)
  unset(_targetsNotDefined)
  unset(_expectedTargets)
  set(CMAKE_IMPORT_FILE_VERSION)
  cmake_policy(POP)
  return()
endif()
if(NOT "${_targetsDefined}" STREQUAL "")
  message(FATAL_ERROR "Some (but not all) targets in this export set were already defined.\nTargets Defined: ${_targetsDefined}\nTargets not yet defined: ${_targetsNotDefined}\n")
endif()
unset(_targetsDefined)
unset(_targetsNotDefined)
unset(_expectedTargets)


# Compute the installation prefix relative to this file.
get_filename_component(_IMPORT_PREFIX "${CMAKE_CURRENT_LIST_FILE}" PATH)
# Use original install prefix when loaded through a
# cross-prefix symbolic link such as /lib -> /usr/lib.
get_filename_component(_realCurr "${_IMPORT_PREFIX}" REALPATH)
get_filename_component(_realOrig "/usr/lib/cmake/SDL2" REALPATH)
if(_realCurr STREQUAL _realOrig)
  set(_IMPORT_PREFIX "/usr/lib/cmake/SDL2")
endif()
unset(_realOrig)
unset(_realCurr)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
if(_IMPORT_PREFIX STREQUAL "/")
  set(_IMPORT_PREFIX "")
endif()

# Create imported target SDL2::SDL2
add_library(SDL2::SDL2 SHARED IMPORTED)

set_target_properties(SDL2::SDL2 PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include;${_IMPORT_PREFIX}/include/SDL2"
)

# Create imported target SDL2::SDL2main
add_library(SDL2::SDL2main STATIC IMPORTED)

set_target_properties(SDL2::SDL2main PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include;${_IMPORT_PREFIX}/include/SDL2"
)

# Load information for each installed configuration.
get_filename_component(_DIR "${CMAKE_CURRENT_LIST_FILE}" PATH)
file(GLOB CONFIG_FILES "${_DIR}/SDL2Targets-*.cmake")
foreach(f ${CONFIG_FILES})
  include(${f})
endforeach()

# Cleanup temporary variables.
set(_IMPORT_PREFIX)

# Loop over all imported files and verify that they actually exist
foreach(target ${_IMPORT_CHECK_TARGETS} )
  foreach(file ${_IMPORT_CHECK_FILES_FOR_${target}} )
    if(NOT EXISTS "${file}" )
      message(FATAL_ERROR "The imported target \"${target}\" references the file
   \"${file}\"
but this file does not exist.  Possible reasons include:
* The file was deleted, renamed, or moved to another location.
* An install or uninstall procedure did not complete successfully.
* The installation package was faulty and contained
   \"${CMAKE_CURRENT_LIST_FILE}\"
but not all the files it references.
")
    endif()
  endforeach()
  unset(_IMPORT_CHECK_FILES_FOR_${target})
endforeach()
unset(_IMPORT_CHECK_TARGETS)

# This file does not depend on other imported targets which have
# been exported from the same project but in a separate export set.

# Commands beyond this point should not need to know the version.
set(CMAKE_IMPORT_FILE_VERSION)
cmake_policy(POP)

@xycaleth
Copy link
Member

Choose a reason for hiding this comment

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

On the Mac, it seems like the SDL2 config file was generated using Autotools. I don't think the CMake-generated SDL2 config defines the SDL2_* variables ironically...

@smcv
Copy link
Contributor Author

@smcv smcv commented on 5203023 Jul 6, 2020

Choose a reason for hiding this comment

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

That seems like a bug in SDL? When built with CMake, it generates a config-module with less information.

@smcv
Copy link
Contributor Author

@smcv smcv commented on 5203023 Jul 6, 2020

Choose a reason for hiding this comment

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

Using SDL2::SDL2 works but this is not backwards compatible with most travis-ci versions of CMake.

What OS are you using for the Travis-CI build? Still bionic?

Ideally SDL would document which parts of the config-modules are "API" and which are implementation details, but at the moment it isn't clear. Perhaps SDL::SDL is the part you're meant to use, I don't know... since it isn't documented, I was going by what worked (when built with Autotools, but apparently not when built with CMake).

@smcv
Copy link
Contributor Author

@smcv smcv commented on 5203023 Jul 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It looks as though SDL2::SDL2 is the part that is meant to be "API", but unfortunately that was only added to SDL Autotools builds in SDL 2.0.12.

If you need to stay backwards-compatible with SDL 2.0.8 (Ubuntu 18.04), perhaps try something like this:

find_package(SDL2 REQUIRED CONFIG)

if (TARGET SDL2::SDL2)
  set(SDL2_INCLUDE_DIRS "")
  set(SDL2_LIBRARIES SDL2::SDL2)
endif()

and then continue to use SDL2_INCLUDE_DIRS and SDL2_LIBRARIES.

@ensiform
Copy link
Member

@ensiform ensiform commented on 5203023 Jul 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Bionic is too new. 16 and 14 are preferred for most backwards compatibility with dedicated server hosts.

Note: I realize this repo's is set to bionic but it probably shouldn't be. However xenial has 3.12.4 in it's Travis updates unless using docker, then it's the ancient version.

Please sign in to comment.