Closed SpaceIm closed 1 year ago
Hi Spacelm,
Sounds like useful suggestions. Thank you for these!
Do you by chance already have these change suggestions in a branch that you'd like to make a pull request (PR) from? Just asking though. I can make the changes of course but I'm happy to give you credit for them in the form of taking a PR from you.
I've merged commit ce2b95fae5f394792fa1703e2383fd89a8690725 into the master branch and cherry picked that as commit 4933b021dee8ea12eb72cebb5287b9ea1dd462ba into the release-1.1.1 branch I'm targeting for eventually tagging and releasing as release 1.1.1.
The file created during export of targets should be included in config file, otherwise it's completely useless.
Currently, a downstream project can't link imported target
PlayRho
(orPlayRho_shared
) after afind_package(PlayRho)
. It ends up linking the raw library through CMake, not the target abstraction, so it's fragile.Looking at git history, this CMakeLists was cherry picked from an old version of Box2D (when it was more or less broken).
I would advice to:
[x] Drop this
usePlayRho.cmake
, it's really an old and terrible way to design a config file.[x] Properly install
PlayRho-targets.cmake
alongPlayRhoConfig.cmake
(not in a different folder).[x] Just write this in
PlayRhoConfig.cmake.in
(and drop all these non-relocatable variables):[x] Configure this
PlayRhoConfig.cmake.in
withconfigure_package_config_file()
(fromCMakePackageConfigHelpers
module).[x] Define
INCLUDES DESTINATION
ininstall(TARGETS ...)
command (so that targets properties are correct inPlayRho-targets.cmake
). I guess it should beINCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
(and by the way it would be better to rely onGNUInstallDirs
variables instead of hardcoded bin, lib, include names).[x] Add a namespace to imported targets (
NAMESPACE
argument ininstall(EXPORT ...)
), so that imported target could bePlayRho::PlayRho
(orPlayRho::PlayRho_shared
).[x] Add an
ALIAS
targetPlayRho::PlayRho
(andPlayRho::PlayRho_shared
) for consistency betweenadd_subdirectory(PlayRho)
of a vendored PlayRho andfind_package(PlayRho)
of externally installed PlayRho.With these suggestions, users will be able to easily consume PlayRho in their project with: