-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add pkg-config files for control libraries #326
Conversation
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.
Seems like a great addition. Thanks for advancing our project configuration with more cmake magic!
source/CMakeLists.txt
Outdated
configure_package_config_file(${CMAKE_CURRENT_SOURCE_DIR}/control_libraries-config.cmake.in | ||
"${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}Config.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.
The base config input file was renamed, but the generated output is still ${PROJECT_NAME}Config.cmake
. Should it also be changed to the lowercase naming convention <lowercasePackageName>-config.cmake
? It shouldn't be a breaking change since find_package
looks for both name types but I would also make sure that's tested before confirming it.
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.
Hm yeah good point, I think I like ${PROJECT_NAME}Config.cmake
better. I renamed the file
source/CMakeLists.txt
Outdated
if(PKGCONFIG_LIBRARIES) | ||
set(PKGCONFIG_LIBRARIES "${PKGCONFIG_LIBRARIES}, ${ARGV0} >= ${ARGV1}") | ||
else() | ||
set(PKGCONFIG_LIBRARIES "${ARGV0} >= ${ARGV1}") | ||
endif() |
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.
Indentation looks a bit off here
if(PKGCONFIG_LIBRARIES) | |
set(PKGCONFIG_LIBRARIES "${PKGCONFIG_LIBRARIES}, ${ARGV0} >= ${ARGV1}") | |
else() | |
set(PKGCONFIG_LIBRARIES "${ARGV0} >= ${ARGV1}") | |
endif() | |
if(PKGCONFIG_LIBRARIES) | |
set(PKGCONFIG_LIBRARIES "${PKGCONFIG_LIBRARIES}, ${ARGV0} >= ${ARGV1}") | |
else() | |
set(PKGCONFIG_LIBRARIES "${ARGV0} >= ${ARGV1}") | |
endif() |
|
||
if(${PKG_CONFIG_FOUND}) | ||
set(PKG_NAME ${LIBRARY_NAME}) | ||
set(PKG_DESC "This library provides a set of classes to represent dynamical systems; functions which map a state to a state derivative.") |
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 semicolon here is not quite right - I would suggest a hyphen or colon instead.
set(PKG_DESC "This library provides a set of classes to represent dynamical systems; functions which map a state to a state derivative.") | |
set(PKG_DESC "This library provides a set of classes to represent dynamical systems: functions which map a state to a state derivative.") |
Similar to a recent PR on network-interfaces, I'm adding here the pkg-config files to the control libraries. For each module, I'm creating a separate file and then I'm also creating one for the top level cmake project that summarizes the 4 modules. The result are 5 files:
/usr/local/lib/pkgconfig/state_representation.pc
:/usr/local/lib/pkgconfig/dynamical_systems.pc
:/usr/local/lib/pkgconfig/robot_model.pc
:/usr/local/lib/pkgconfig/controllers.pc
:/usr/local/lib/pkgconfig/control_libraries.pc
:Of course, the
Requires
tags in that last file will only be filled with the libraries that are actually installed.It's a small feature that will most likely not used a lot but it can't hurt to do it. My main motivation is to add it here such that other packages can create pkg-config files where we add
Requires: control_libraries
. This wouldn't be possible if control libraries doesn't have pkg-config files itself.