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

Use modern CMake targets for SDL2 #6132

Merged
merged 3 commits into from Oct 5, 2021
Merged

Conversation

DomClark
Copy link
Member

This PR changes FindSDL2.cmake to provide an imported target SDL2::SDL2 to link to, rather than SDL2_LIBRARY and SDL2_INCLUDE_DIR variables to manually target. This is the same as provided by SDL2's own CMake configuration script, which means that that script can be used in place of ours if available. Indeed, that is the motivation - our script only imports a single version of SDL2, which means that MSVC debug builds of LMMS still use the release version of SDL2. With SDL2's own multiple imported configurations, CMake can select the correct version of the library to use with a particular build configuration.

This is a step towards solving #5683 - it fixes the problem for Debug builds, and simplifies the situation for RelWithDebInfo builds, which should make a full fix easier in the future.

Other changes made:

  • SDL2's own configuration script is now used if available, falling back to ours if no SDL2::SDL2 target is defined (either because the script is missing, or because it's a custom one that still uses old CMake variables).
  • Removed the SDL2_LIBRARY_TEMP variable. Previously this held the path to the SDL2 library, which was used to construct the SDL2_LIBRARY variable, which contained other flags and libraries required for linking to SDL2. These other flags and libraries are now in the appropriate properties of the imported targets, so SDL2_LIBRARY can be the library itself again.
  • SDL2main is no longer linked on the basis of the variable SDL2_BUILDING_LIBRARY; it is its own target SDL2::SDL2main, which can optionally be linked to.
  • The addition of the -mwindows flag that selects the Windows GUI subsystem for MinGW has been moved to src/CMakeLists.txt. It used to be added in FindSDL2.cmake due to some faulty logic - it is required only for SDL2main, not SDL2, but was nevertheless added unconditionally. Now that SDL2main is a separate target, I decided we shouldn't be adding this flag as a side effect of using a library that doesn't need it, and instead add it explicitly in the relevant CMake file. (I'm not using CMake's WIN32_EXECUTABLE property yet, since that requires extra jiggery-pokery for MSVC which is out of scope here.)

Although I have given SDL2main its own target, it may be preferable to remove it entirely, since we don't use it and so I haven't tested that bit of code.

Only tested with MSVC and MinGW so far, let's see what happens elsewhere...

@DomClark
Copy link
Member Author

It seems we only use SDL2 with MSVC in CI. If anyone uses SDL2 on Linux or Mac, it would be great if you could build and test this PR yourself.

The MSVC build is succeeding, but fails to find SDL2d.dll when packaging. I'm trying to figure out how to debug this.

@Veratil
Copy link
Contributor

Veratil commented Aug 15, 2021

I believe my VM has SDL2, I'll test this next chance I get.

@rdrpenguin04
Copy link
Contributor

I'm testing on Manjaro now

@rdrpenguin04
Copy link
Contributor

Works on Manjaro, confirmed.

@Veratil
Copy link
Contributor

Veratil commented Aug 16, 2021

Tested on Fedora 32, SDL2 is found and configured. Works when setting audio interface to it, too.

@DomClark
Copy link
Member Author

CI passes now, and this has been tested on Linux and Windows. Now it needs reviewing, and I'd still like it tested on Mac (although if necessary we can merge without, and fix any problems that come up when we bring SDL2 to CI Mac builds).

@DomClark DomClark added the needs code review A functional code review is currently required for this PR label Aug 16, 2021
@PhysSong
Copy link
Member

PhysSong commented Sep 5, 2021

Confirmed working on macOS.

@PhysSong
Copy link
Member

Looks good to me except the question above.

@PhysSong
Copy link
Member

PhysSong commented Oct 3, 2021

Merge?

@DomClark DomClark merged commit 3d7ef9f into LMMS:master Oct 5, 2021
@DomClark DomClark deleted the sdl2-cmake-targets branch October 5, 2021 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs code review A functional code review is currently required for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants