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

modules: Make FindSDL2.cmake also find debug libraries #45

Closed
wants to merge 1 commit into from

Conversation

Squareys
Copy link
Contributor

@Squareys Squareys commented Jun 6, 2018

Hi @mosra !

As discussed in #42 , here's the PR that fixed the find module to also find the debug libs so that they are properly copied in vcpkg builds.

Cheers, Jonathan

if(SDL2_LIBRARY_DEBUG)
set_property(TARGET SDL2::SDL2 APPEND PROPERTY IMPORTED_CONFIGURATIONS DEBUG)
set_property(TARGET SDL2::SDL2 PROPERTY IMPORTED_LOCATION_DEBUG ${SDL2_LIBRARY_DEBUG})
endif()
endif()
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm. What about the case where there is only a debug SDL build? (As far as I know, the macOS framework has only a release version, so you don't need to duplicate the workaround for a debug version.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about the case where there is only a debug SDL build?

Then this script will not find anything (for backwards compatibility 😜) Will fix it.

And will remove the workaround

@Squareys
Copy link
Contributor Author

@mosra Did the requested change, please double check if the "only debug libraries" situation is properly handled this way (I tested it locally, and it works, but maybe it's not the way to go?).

@@ -118,12 +124,22 @@ if(NOT TARGET SDL2::SDL2)
endif()
add_library(SDL2::SDL2 ${_SDL2_IMPORTED_LIBRARY_KIND})


Copy link
Owner

Choose a reason for hiding this comment

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

Why the extra line? ;)

# Work around BUGGY framework support on macOS
# https://cmake.org/Bug/view.php?id=14105
if(CORRADE_TARGET_APPLE AND ${SDL2_LIBRARY} MATCHES "\\.framework$")
set_property(TARGET SDL2::SDL2 PROPERTY IMPORTED_LOCATION ${SDL2_LIBRARY}/SDL2)
if(CORRADE_TARGET_APPLE AND ${SDL2_LIBRARY_RELEASE} MATCHES "\\.framework$")
Copy link
Owner

Choose a reason for hiding this comment

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

I think this should be just if(CORRADE_TARGET_APPLE AND SDL2_LIBRARY_RELEASE MATCHES "\\.framework$"), so without the ${}. Not sure why I did it like that before, but with the change now I think this could fail if you are on Apple and have only the debug version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the documentation of MATCHES I would think that both are valid, will change it, though.

Signed-off-by: Squareys <squareys@googlemail.com>
@Squareys
Copy link
Contributor Author

@mosra Pushed again with new requested changes, I hope I correctly understood the comment for the apple stuff and there isn't more to do there than I did.

@mosra mosra added this to TODO in Project management via automation Jun 18, 2018
@mosra mosra added this to the 2018.0c milestone Jun 18, 2018
@mosra
Copy link
Owner

mosra commented Jun 18, 2018

Merged in mosra/magnum@52b7c26, now it also should be copied into all other repositories.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants