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

Expose SDL2_BIN_DIR on FindSDL2.cmake #347

Closed
wants to merge 2 commits into from
Closed

Expose SDL2_BIN_DIR on FindSDL2.cmake #347

wants to merge 2 commits into from

Conversation

@isc30
Copy link
Contributor

isc30 commented Jun 8, 2019

Hi, this change aims to help windows users that use Sdl2Application.

First, it makes use of SDL2_PATH (if set) as a HINT path for finding the library instead of always relying on CMAKE_PREFIX_PATH.

Then, it also exposes SDL2_BIN_DIR if SDL is found.
This way, the windows users can decide if they want to copy the SDL DLLs automatically with the following (or similar) snippet:

# Copy SDL2 DLL to output folder on Windows
if(NOT CORRADE_TARGET_EMSCRIPTEN AND WIN32)
    add_custom_command(TARGET Game
        POST_BUILD COMMAND ${CMAKE_COMMAND} -E copy_if_different ${SDL2_BIN_DIR}/SDL2.dll $<TARGET_FILE_DIR:Game>)
endif()

Without this change, detecting the bin folder while allowing crosscompiling (vs, mingw, clang, etc) isn't trivial.
Thanks!

isc30 added 2 commits Jun 8, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 9, 2019

Codecov Report

Merging #347 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #347   +/-   ##
======================================
  Coverage    71.4%   71.4%           
======================================
  Files         349     349           
  Lines       17602   17602           
======================================
  Hits        12568   12568           
  Misses       5034    5034

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e1e52d...20f545d. Read the comment docs.

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Jun 17, 2019

Hi,

Why isn't CMAKE_PREFIX_PATH sufficient? Since this option works generally for any library, I think it makes more sense to use that instead of having to remember SDL2_PATH, FREETYPE_PREFIX, ASSIMP_DIR and such, each with a slightly different name and semantics for each library

For finding the DLLs -- yes, I acknowledge it's a bit lacking right now, however doing this just for SDL isn't systematic enough I think. Two options come to my mind:

  • Use / update corrade_find_dlls_for_libs() that I use sometimes to bundle DLLs when deploying Windows apps. Not sure if it works for SDL -- maybe it does?
  • On Windows provide <FOO>_LIBRARY_DLL variables for each library -- unfortunately that means it won't "just work" for dependencies with 3rd party Find modules / Config files that don't provide these variables.

Or a combination of both -- for example the macro first looking for <FOO>_LIBRARY_DLL for every passed <FOO>_LIBRARY and as a fallback looking for the DLL in ../bin/ relative to the *lib as it does now. Thoughts? :)

@isc30

This comment has been minimized.

Copy link
Contributor Author

isc30 commented Jun 17, 2019

The macro is a really good idea, I like the proposed check (*_LIBRARY_DLL variable + fallback to search inside /bin folder).
I will have a look on how it is currently implemented and propose a change.
Thanks!

Edit: should we make it a list (*_LIBRARY_DLLS) just to cover the possible cases when a library comes with dependencies? (SDL2.dll + SDL2main.dll for example)

@isc30

This comment has been minimized.

Copy link
Contributor Author

isc30 commented Jun 17, 2019

About CMAKE_PREFIX_PATH, the idea of having a variable that (when set) takes priority over CMAKE_PREFIX_PATH is really helpful. It's a way of saying: if set, use this path and anything else.

Also, and this is just a personal preference, the command to run cmake looks way cleaner with multiple variables for paths and not just a single CMAKE_PREFIX_PATH with a huge list.

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Jun 20, 2019

Edit: should we make it a list (*_LIBRARY_DLLS)

Yes, that's a very good idea. Or maybe just <FOO>_DLLS?

About CMAKE_PREFIX_PATH, the idea of having a variable that (when set) takes priority over CMAKE_PREFIX_PATH is really helpful.

Are there any official guidelines / recommendations for how these variables should be named? Because I really saw many inconsistent variants and don't want to add more to that mess :D I know there's <foo>_DIR when using the <foo>Config.cmake files, but not aware of anything similar for Find<foo>.cmake.

@isc30

This comment has been minimized.

Copy link
Contributor Author

isc30 commented Jun 21, 2019

Lovely CMake... I haven't seen any standard on how to name these things... I guess it's a personal decision.

The most common variants I've seen are <foo>_PREFIX_PATH and <foo>_PATH. The first one actually makes a lot of sense and it's consistent with CMAKE_PREFIX_PATH. If I were to create a standard I would pick that one.

What do you think?

@mosra mosra added this to TODO in Platforms via automation Jul 28, 2019
@mosra mosra added this to the 2019.0b milestone Jul 28, 2019
@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Jul 28, 2019

Sorry for not getting back to this yet. Possibly related -- CMake master (which will become 3.16, I think?) has a new command, file(GET_RUNTIME_DEPENDENCIES). I imagine this could finally become the right way to get all dependency DLLs?

@isc30

This comment has been minimized.

Copy link
Contributor Author

isc30 commented Jul 30, 2019

woah, that seems really nice. I will have a look at it. It seems to only work when installing the project, but definitely worth doing it like that if it's going to become the standard way

@mosra mosra removed this from the 2019.0b milestone Oct 2, 2019
@mosra mosra added this to the 2019.0b milestone Oct 12, 2019
@williamjcm

This comment has been minimized.

Copy link
Contributor

williamjcm commented Oct 12, 2019

This way, the windows users can decide if they want to copy the SDL DLLs automatically with the following (or similar) snippet:

# Copy SDL2 DLL to output folder on Windows
if(NOT CORRADE_TARGET_EMSCRIPTEN AND WIN32)
    add_custom_command(TARGET Game
        POST_BUILD COMMAND ${CMAKE_COMMAND} -E copy_if_different ${SDL2_BIN_DIR}/SDL2.dll $<TARGET_FILE_DIR:Game>)
endif()

From personal experience, copy_if_different seems to only work correctly if the destination file already exists.

I previously used it on my end, and if I had a clean build directory, the command just didn't copy the DLLs, which meant I got plenty of "missing library" errors when trying to launch my executable.

YMMV, though.

@isc30

This comment has been minimized.

Copy link
Contributor Author

isc30 commented Oct 14, 2019

Sorry for not updating on this, I'm very busy lately.
If anyone is interested in having a look at GET_RUNTIME_DEPENDENCIES feel free to continue this work, I won't be able to work on this until March.

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Oct 15, 2019

The DLL-finding part is merged in dc48491, thank you (and with ef1fbd8 also copied next to binaries if using Magnum as a subproject), besides that I discovered there's IMPORTED_LOCATION vs. IMPORTED_IMPLIB for imported targets (where the former is a location of the DLL and the latter is a location of a corresponding *.lib). Right now the Find modules incorrectly put the *.lib into IMPORTED_LOCATION (and providing the DLL via a variable), it doesn't seem to be a problem so "don't fix what's not broken" but I may revisit this later as it seems to be a nice standardized way to expose DLL locations of imported libraries.

Besides that, for the record, I found that since 3.12 there actually seems to be a standard way to supply package prefixes -- <PackageName>_ROOT. Didn't have a chance to look further into this, but seems to be a way to supply a different root for each particular find_package call.

@mosra mosra closed this Oct 15, 2019
Platforms automation moved this from TODO to Done Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Platforms
  
Done
4 participants
You can’t perform that action at this time.