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.

Sign up for GitHub

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

Jump to bottom

Update the SDL2 CMake modules #190

Merged
merged 3 commits into from
Aug 14, 2023
Merged

Update the SDL2 CMake modules #190

merged 3 commits into from
Aug 14, 2023

Conversation

pinotree
Copy link
Contributor

@pinotree pinotree commented Aug 3, 2023 •
edited
Loading

  • drop FindSDL2.cmake, since SDL2 ships config files for CMake already
  • drop FindSDL2_image.cmake, as SDL2_image is not used
  • update the license text of FindSDL2_ttf.cmake

@pinotree
Copy link
Contributor Author

pinotree commented Aug 3, 2023

Needs more testing -> draft for now.

@pinotree pinotree marked this pull request as draft August 3, 2023 05:08
@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2023 •
edited
Loading

Codecov Report

Merging #190 (6895601) into master (b491952) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master     #190   +/-   ##
=======================================
  Coverage   94.78%   94.78%           
=======================================
  Files          81       81           
  Lines        2875     2875           
=======================================
  Hits         2725     2725           
  Misses        150      150

@arximboldi
Copy link
Owner

It seems the build passed, what kind of further testing would you like to have?

@pinotree
cmake: drop FindSDL2_image.cmake
436a784 
SDL2_image is not used anywhere, so drop this unused module.
@pinotree
cmake: drop FindSDL2.cmake
d852ae9 
SDL2 has been providing CMake config files for many years; hence, there
is no need to use an own FindSDL2.cmake module for find_package().

This needs to update the variables used for SDL2 to the ones provided
by the SDL2 CMake config files:
- SDL2_INCLUDE_DIR -> SDL2_INCLUDE_DIRS
- SDL2_LIBRARY -> SDL2_LIBRARIES
@pinotree
cmake: update license text of FindSDL2_ttf.cmake
6895601 
It looks like this module originated from CMake, which uses a 3-clause
BSD license; since the Copyright.txt file is not shipped, and the text
even mention to replace the short text with the full license when using
it outside of CMake, then place the complete license text.
@pinotree
Copy link
Contributor Author

what kind of further testing would you like to have?

I was not sure yet about the approach to do for FindSDL2_ttf.cmake, since the main problem I wanted to solve was the lack of the license text (mentioned there); while the original idea was also to provide a newer/better version of it, in the end I chose to simply fix the license text, and keep the existing module (which works fine).

Hence, it should be fine to review now, thanks!

@pinotree pinotree marked this pull request as ready for review August 12, 2023 17:58
@arximboldi
Copy link
Owner

Thank you!!

@arximboldi arximboldi merged commit 4406b24 into arximboldi:master Aug 14, 2023
7 checks passed
@pinotree pinotree deleted the cmake-sdl-modules branch August 14, 2023 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pinotree @codecov-commenter @arximboldi