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
Replace FindSFML.cmake with SFMLConfig.cmake #1335
Conversation
@@ -69,9 +69,6 @@ set(VERSION_MAJOR 2) | |||
set(VERSION_MINOR 4) | |||
set(VERSION_PATCH 2) | |||
|
|||
# add the SFML header path | |||
include_directories(${CMAKE_CURRENT_SOURCE_DIR}/include) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be "exportable", this needs to be defined on targets, not globally, thus https://github.com/SFML/SFML/pull/1335/files#diff-031c6047a0f4c227a313b7b2f3b77219R141
cmake/Macros.cmake
Outdated
@@ -2,13 +2,14 @@ include(CMakeParseArguments) | |||
|
|||
# add a new target which is a SFML library | |||
# ex: sfml_add_library(sfml-graphics | |||
# SOURCES sprite.cpp image.cpp ... | |||
# DEPENDS sfml-window sfml-system | |||
# EXTERNAL_LIBS opengl freetype ...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Distinction was only for internal vs external dependencies? (linking against a lib already creates a dependency)
I removed these because it doesn't allow specifying dependency by dependency if that said dependency is PRIVATE or PUBLIC
# add the install rule | ||
install(TARGETS ${target} | ||
install(TARGETS ${target} EXPORT SFMLConfigExport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The KEY to having targets exported to the final config file
Reused in https://github.com/SFML/SFML/pull/1335/files#diff-80cb5bd89215157ada070906e35cadbb
cmake/Macros.cmake
Outdated
# the first one is for the local package and the second one is for the installed package | ||
target_include_directories(${target} PUBLIC | ||
$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include> | ||
$<INSTALL_INTERFACE:include>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only PUBLIC property right now
cmake/export/SFMLConfig.cmake
Outdated
@@ -0,0 +1 @@ | |||
include("${CMAKE_CURRENT_LIST_DIR}/SFMLTargets.cmake") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmake/export/SFMLConfigExport.cmake
Outdated
@@ -0,0 +1,26 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/SFML/Graphics/CMakeLists.txt
Outdated
list(APPEND GRAPHICS_EXT_LIBS z) | ||
endif() | ||
list(APPEND GRAPHICS_EXT_LIBS ${FREETYPE_LIBRARY}) | ||
target_include_directories(sfml-graphics PRIVATE ${FREETYPE_INCLUDE_DIRS}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this one was previously duplicated line 110 and 122
src/SFML/Audio/CMakeLists.txt
Outdated
add_definitions(-DOV_EXCLUDE_STATIC_CALLBACKS) # avoids warnings in vorbisfile.h | ||
add_definitions(-DFLAC__NO_DLL) | ||
# define the sfml-audio target | ||
sfml_add_library(sfml-audio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
target needs to be defined before using target_link_libraries(), target_include_directories() and target_compile_definitions(), so moved from below
Amazing work, thanks 👍 I know very little about modern CMake usage, but what I see looks ok. We should now test it under various scenarios (static/dynamic link, on all OSes).
What exactly does that mean? Will CMake now automatically export the private dependencies in case of static linking, or does it require additional work? In any case, we no longer need
Should be done, since the dependency does exist. |
Didn't dig too deep into the changes, but with the branch as-is, You're basically missing this:
from Also are you sure about the location of the |
The most important part is correctly defining which target properties have PRIVATE, PUBLIC or INTERFACE visibility. Are you familiar with it? And in these properties you can define include directories, libraries to link or preprocessor definitions.
It will, at least that's what I want. But it's not done yet with current changes. And yes SFML_DEPENDENCIES won't be needed anymore if I get it to work. Will update the PR when it's ready.
Right, will update when it's done |
Indeed. Didn't do it yet as it's not needed on macOS, because I wanted feedbacks before going further. Will fix it.
There are several possible paths, see https://cmake.org/cmake/help/latest/command/find_package.html "Each entry is meant for installation trees following Windows (W), UNIX (U), or Apple (A) conventions:"
Still need to figure out where it'll be located in the end. It's probably not portable on this part right now. |
I initiated the CI builds, checkout the build logs (Config.hpp can't be found). |
Which is exactly what I've stated above. ;) |
Build is fixed for sfml-main, everything builds fine locally on Windows (VS2017 32bits). I ran the install step with the Debug/Release configuration and did a find_package(SFML) without any FindSFML.cmake and it did work on Windows, with the correct lib depending on selected configuration (Debug/Release). There doesn't look to be any absolute path in the generated config file on Windows, which is nice for making releases. On the contrary for now the config file generated on macOS has absolute paths (/Library/Frameworks). To be fixed. Still need to support static builds, most likely the hardest part because of the need to expose internal dependencies. And I don't know yet what to do about components handling... One important noteI changed a bit how the sfml-main library is created. Instead of custom add_library() and properties, it's created like any other SFML library : with suffix for debug / static, and with pdb next to the generated .lib file. The only customization I kept is that it always remains a static lib, whatever the value of BUILD_SHARED_LIBS. |
Nice. I'd say absolute paths aren't that bad for now. Wouldn't they be the same on any machine anyway? No experience with Frameworks, but from my understand they're supposed to sit in that specific directory? |
New revision is building fine for me on Windows. Do we even have to keep (and therefore maintain) |
It depends on what you want to do. Currently FindSFML.cmake allows finding SFML in standard paths or in custom paths if you set SFML_ROOT. I don't see any reason to restrict the benefits of CMake config files to standard installations only. Consider for example where you want to make sure that the package you're creating for your program is self-contained. In that case you don't want to have SFML installed in standard paths. You can also want to keep standard paths clean, or don't have admin rights, etc.
I'm always for keeping a simple and unique way to do things. But I don't think I'm the one to choose this for SFML :) Also I realize that currently existing |
Do you mean the |
Yes the |
@MarioLiebisch Do you know where sfml-graphics links against OpenGL ES libraries on Android? I can't find it on SFML/master and I broke link step of this module on Android: https://ci.sfml-dev.org/builders/android-armeabi-v7a-api13/builds/267/steps/compile/logs/stdio I saw that you worked on Android support so asking you :) |
It used to be here: https://github.com/SFML/SFML/blob/master/src/SFML/Graphics/CMakeLists.txt#L126 but obviously got lost in the rewrite. |
It's still there: https://github.com/Ceylo/SFML/blob/feature/CMakeTargetExport/src/SFML/Graphics/CMakeLists.txt#L124 And I don't think you pointed out the correct line, because in Config.cmake I see that Android uses OpenGL ES. |
It seems like it was simply not set. But it previously worked because another signature of target_link_libraries() was used: https://cmake.org/cmake/help/latest/command/target_link_libraries.html?highlight=target_link_libraries#libraries-for-both-a-target-and-its-dependents Which imported linkage for OpenGL ES from sfml-window target definition : https://github.com/SFML/SFML/blob/master/src/SFML/Window/CMakeLists.txt#L270 And it's no more the case with the explicit PRIVATE linkage (it was implicitly PUBLIC). Hopefully easy to fix :) |
But that's outside the |
Will be away till the end of the week so don’t be surprised if there’s no progress :) |
@eXpl0it3r As for where the PDB files go (ie. not next to libraries, and always in PREFIX/lib/Debug even for release builds), I checked and this is the same behavior as current master branch. Thus not going to change that for current PR. The only difference I noticed about PDB files is that with my PR you now also get a PDB for sfml-main library. All in all, if @LaurentGomila confirms expected behavior with find_package() and components, I guess current PR is still ok for merge. I'll just do a rebase against master to take into account latest commits. |
5d670c4
to
aa72ebf
Compare
Yes, we musn't (can't) make one entry depend on the other. But what about using some default path if
In case someone takes your minimal example as a reference: the
Not sure about the current behaviour. But that was just an idea anyway. Do you think it's worth investigating? |
I thought so. For me it's just a different approach I'll have to take and define the prefix manually before clicking the configuration button. Unfortunately this will be an issue many people will fail to understand and we'll have to keep answering the same question. IMHO on Windows it never makes sense to install the dependencies somewhere else, as such this is really inconvenient for Windows users.
By default it will use Would it be an option to not set the
Checked again on master branch, it exists as well with my Doxygen version. The reason I stuck with the version is because newer versions don't generate the nice HTML output we want, but that's not important for this PR. Works fine with the latest Doxygen, so let's forget about this.
Odd, I could swear the behavior was different in the past, but yet, I see the same thing on master branch, so let's align this with another PR.
I think it's okay the way it is right now. I just suspect again that people will get it wrong and we'll have to keep answering the same question. My suggestion would be, something that I kind of wanted for a while, to have a dedicated tutorial not on how to build SFML, but on how to use SFML with CMake. That way we can hopefully answer a lot of questions and have a reference for people. Also we need to make sure to update the existing CMake tutorial for these changes. |
This is a good point indeed. Dunno why I didn't do that just like for SFML_MISC_INSTALL_PREFIX. It's committed & pushed.
So with the relative path it's just fixed, not need for all these ifs :)
I don't think this is always true. For example if you take a project that depends on SFML and wants to embed all their dependencies in their repo (let's say with Git LFS). Setting this variable in the main project allows having a repo ready to use just with a single clone. So it depends on whether the developer wants to embed its dependencies or let its users provide them.
Just to make sure I tried using current FindSFML.cmake with no component. The result is actually worse than what I thought... with no component, it always consider SFML to be found even when it's not :/
So no regression on that matter. As for investigating about support "no component given" case, I think this should be discussed and eventually changed later. I don't really have an opinion at the moment about whether this is a good idea.
Currently I'd say that the best tutorial for this is the documentation in SFMLConfig.cmake file. But you have to find that file first, which is not quite obvious 😄 .
Do you mean more than #1335 (comment) ? Also, can one of you relaunch the CI builds? |
Could be more or less a 1:1 copy, I don't like spreading documentation across multiple places, especially since it's not an obvious place to look.
If that covers all, then that's good. 😊 |
A 1:1 copy would look weird to me. What about giving a link to SFMLConfig.cmake from SFML tutorials page? This way you can also make sure that the tutorial is consistent with what is shipped. Ideally there would be some sort of Doxygen that generates formatted doc from the comments in CMake files, but I’m not aware of such tool. By the way builds have finished now. |
Merged in fc655f5 Thanks for the exhaustive work and the pushing forward, really appreciated! 🎉 |
That was a huge task, and it was much awaited. Thank you very much 👍 |
Hooorrraaaaaay!!!!!!! I would like to discuss with you about the PR process. Mainly about the testing part: it took almost 2 months between the day the PR was ready for testing and the day it got merged. This is way too long, even in the opensource world, and honestly if it always take this long I'm not gonna do other PRs. So I'd like to talk and find solutions with you. Where can we do that? The PR comments don't feel like it's the right place, and on the forum (except maybe in section "SFML development" where I can't open topics) it's also too wide: this is mainly toward SFML development team & contributors. |
Yeah, this is a big step forward! I'm really happy for this top-quality contribution of yours! Thanks again. I understand the feeling, and share your point of view. But the fact is that the team alone cannot handle everything, at least not quickly enough to keep the flow. The only solution I see is asking the community to test things out. Now, in the last few months, several people have joined the effort, and I thank them. Hopefully, this will attract more testers as well. In the meantime, if you have ideas on how to improve the overall process (doc, how-to's, issue handling,...) I feel it would best fit on the forum. I'm pretty sure a moderator will agree to move your thread in the right category. :) |
I think that people will be more likely to test if developers either provide detailed instructions on how to perform testing or make a test program if it's possible. The easier it is for people to test, the more likely they're to do it. |
TODO list:
Out of scope:
=============
I've started doing the changes to support config file generation. At the moment this is working at least for me on macOS with frameworks. I expect it to work on other platforms too because it reuses the definitions of current SFML targets. This implies a LOT of changes though so I would like your feedback before going on.
Especially I have replaced all the listing of dependencies with calls to target_link_libraries() and target_include_directories() for 3 reasons:
In terms of usage right now (example from sfeMovie) of course without any FindSFML.cmake:
I didn't expose yet the fact that sfml-graphics depends on sfml-window and sfml-system. Could be done and would allow writing only
target_link_libraries(sfeMovie PRIVATE sfml-graphics sfml-audio)
Dunno yet if we want that though.
If you want to look at what the generated config looks like for now:
http://yalir.org/files/SFML/
It gets installed in /usr/local/lib/cmake/SFML by default.
References
https://cmake.org/cmake/help/latest/manual/cmake-packages.7.html#creating-packages
https://cmake.org/cmake/help/latest/command/find_package.html
https://cmake.org/cmake/help/latest/command/export.html
https://cmake.org/cmake/help/latest/module/CMakePackageConfigHelpers.html
master...SrTobi:support-config-file-packages
Related issues
#758
#937