-
Notifications
You must be signed in to change notification settings - Fork 55
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
Use CMake Usage Requirements for podio targets #54
Conversation
When building podio, use target_include_directories to set its build and install time header paths. Use of include_directories is retained in scripts due to ROOT's dictionary generation not supporting usage requirements until version 6.18. Update podioConfig to use full CMakePackageConfigHelpers system: - Automatic self-location - No hard coded install time paths - Re-find ROOT dependency - Proper imported target for libpodio These mean that a podio client can, in CMake, do ```cmake find_package(podio REQUIRED) add_executable(foo foo.cc) target_link_libraries(foo podio::podio) ``` and all include/link paths will be set correctly.
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.
Hi Ben,
I'll let @gaede do the final review, but this looks great and will help the dependent packages to clean up their cmake config. Thanks!
INCLUDE( FindPackageHandleStandardArgs ) | ||
FIND_PACKAGE_HANDLE_STANDARD_ARGS( podio DEFAULT_MSG podio_LIBRARY_DIR podio_INCLUDE_DIRS podio_LIBRARIES ) | ||
|
||
ADD_LIBRARY(examplelibrary SHARED IMPORTED) |
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.
Excellent, this line (left over from boilerplate) forced us to use a custom FindPodio script, great that this is gone.
Thanks @vvolkl ! I've just realised I left a debug |
Remove extraneous debugging message() call. Set additional interface include directories for podio imported target with set_property per CMake recommendations.
Looks good to me - will merge. |
The changes introduced by AIDASoft#54 caused installs to fail due to the change in location of the templates directory relative to python/. Following discussion in AIDASoft#58, implement simplest possible change to fix by moving templates inside the python directory. Retain this layout at install time, adjusting the `template_dir` variable in podio_class_generator.py accordingly. Fixes: AIDASoft#58
The changes introduced by #54 caused installs to fail due to the change in location of the templates directory relative to python/. Following discussion in #58, implement simplest possible change to fix by moving templates inside the python directory. Retain this layout at install time, adjusting the `template_dir` variable in podio_class_generator.py accordingly. Fixes: #58
This replaces the horribly out of date #29. key4hep/EDM4hep#1 identified a few issues in the way Podio is built and creates its CMake config files for downstream clients. This PR aims to address some of these by:
podioConfig.cmake
file and associatedpodioTargets.cmake
These mean that a podio client can, in CMake, do
and all include/link paths will be set correctly.
I wasn't sure how to address install of the Python code as this appears split between program and library code (e.g.
podio_class_generator.py
should probably go in theCMAKE_INSTALL_BINDIR
location, whereaspodio_config_reader.py
is more a Python module). Part of this would be to allowpodio_class_generator.py
to be supplied as a CMake imported target together with a CMake function to run code generation, but see the info in #29 on issues here.BEGINRELEASENOTES
podioConfig.cmake
file and associatedpodioTargets.cmake
A podio client can, in CMake, do
and all include/link paths will be set correctly.
ENDRELEASENOTES