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

cmake: minor correction to support Ubuntu 20.04 #651

Merged
merged 1 commit into from Apr 11, 2020

Conversation

cmitu
Copy link

@cmitu cmitu commented Apr 11, 2020

Looks like libsdl2-dev on Ubuntu 20.04 (and Debian testing at the moment) have relocated the headers to /usr/include/<platform-triplet> 1, so detection for SDL.h fails currently.
Augment PATH_SUFFIXES to search in /usr/include/<triplet> (thanks @hhromic).
We should probably revise the FindSDL2.cmake package with a more modern version.

Minor - remove BOM from FindRapidJSON.cmake (fixes #643) to fix an bug in old CMake versions.

[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=909740

@joolswills
Copy link
Member

Is this complete ? Wanted to check as there looks like some line ending or whitespace change? Thanks for this - Didn't expect this so soon after I tried to offload the work :-)

@joolswills
Copy link
Member

Ah sorry - the other change is the one you referenced with BOM - I wasn't sure what that mean't

@joolswills
Copy link
Member

Will test and merge - sorry for spam.

@joolswills joolswills merged commit 635e50d into RetroPie:master Apr 11, 2020
@joolswills
Copy link
Member

Thanks!

@hhromic
Copy link
Member

hhromic commented Apr 11, 2020

Augment PATH_SUFFIXES to search in /usr/include/<triplet>

Just wanted to clarify that the solution is not described correctly and can be misleading.

Adding SDL2 to PATH_SUFFIXES does NOT make CMake to search in /usr/include/<triplet>. CMake is indeed already searching there.

What the solution in this PR does is to add SDL2 as an additional possible suffix to use when searching in all the configured paths, including /usr/include/<triplet>, by CMake. Part of the list of paths to search is given by the PATH keyword, currently set from SDL2_SEARCH_PATHS:

~/Library/Frameworks
/Library/Frameworks
/usr/local  <-- this and ...
/usr   <- this is why the current version of FindSDL2 works in standard systems
/sw # Fink
/opt/local # DarwinPorts
/opt/csw # Blastwave
/opt

Then, what CMake does with the suffixes is to search like this (before this PR):

/Library/Frameworks + /include/SDL2
/Library/Frameworks + /include
/usr/local + /include/SDL2   <-- we might find SDL2 here
/usr/local + /include
/usr + /include/SDL2   <-- here we find SDL2 in most standard systems
/usr + /include
/usr/include/<triplet> + /include/SDL2   <- won't find SDL2 there
/usr/include/<triplet> + /include   <- won't find SDL2 there
(... and all the other paths in CMake, besides the ones in PATH ...)

This is the reason why SDL2 couldn't be found in the case. Now, with the change in this PR, the path suffix SDL2 is also considered for the same search paths:

/Library/Frameworks + /SDL2   <-- new suffix
/Library/Frameworks + /include/SDL2
/Library/Frameworks + /include
/usr/local + /SDL2   <-- new suffix
/usr/local + /include/SDL2   <-- we might find SDL2 here
/usr/local + /include
/usr + /SDL2
/usr + /include/SDL2   <-- here we find SDL2 in most standard systems
/usr + /include
/usr/include/<triplet> + /SDL2   <- now we can find SDL2 here !
/usr/include/<triplet> + /include/SDL2
/usr/include/<triplet> + /include

As you can notice, most paths to search are obviously wrong but still the right ones are there so "it works". This is the reason why I recommend to use a modern, better programmed, FindSDL2.cmake module.

Sorry to be pedantic but I feel this needs to be understood correctly to know how CMake really works. I hope this is clearer now. Cheers!

@joolswills
Copy link
Member

Appreciated. Thanks.

@joolswills
Copy link
Member

I also agree - and the FindSDL2 code is based on even older FindSDL code. It should imho be using sdl2-config / pkg-config or something rather than just trying lots of paths etc.

@joolswills
Copy link
Member

PRs welcome ;-)

@hhromic
Copy link
Member

hhromic commented Apr 12, 2020

It should imho be using sdl2-config / pkg-config or something rather than just trying lots of paths etc.

The modern way for CMake is to use config modules which are similar to the concept of sdl2-config/pkg-config. Dependencies are responsible for providing these CMake config modules. In fact SDL2 does provide one since ~2017 so is not guaranteed to be provided for older versions. On the other hand pkg-config files provided by dependencies are known to sometimes be wrong therefore CMake doesn't recommend using them (even though SDL2's pkg-config files are correct most of the time).

The modernised FindSDL2.cmake module I suggested before from this repository has been made with a lot of these things in consideration so I think we should adopt it. Worth to mention that the solution on this PR was found by testing with this modernised version.

It does not use sdl2-config, pkg-config nor a config module but relies on CMake itself to do search (which is pretty smart actually) for maximum compatibility across different platforms. It also makes sure to use compiler flags in the correct order and to use SDL2main when necessary.

PRs welcome ;-)

Okay will do!

Yaoh pushed a commit to Yaoh/EmulationStation that referenced this pull request Jul 13, 2020
cmake: minor correction to support Ubuntu 20.04
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.

Error compiling EmulationStation cmake 2.8
3 participants