-
Notifications
You must be signed in to change notification settings - Fork 80
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
modernize cmake syntax #13
base: main
Are you sure you want to change the base?
Conversation
@Ryanf55 FYSA |
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.
Nice improvement!
if(DEFINED ENV{ROS_DISTRO}) | ||
include_directories("/opt/ros/$ENV{ROS_DISTRO}/include") | ||
endif() | ||
"${OMPL_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.
Do you want to just call target_link_libraries
on ompl::ompl
?
DESTINATION ${CMAKE_INSTALL_LIBDIR} | ||
COMPONENT omplapp) | ||
endif() | ||
install(EXPORT omplExport |
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.
Will this conflict with the other export if someone installs both libraries into the same folder (a colcon user would share the install folder between multiple repos)? Usually, I've seen ${PROJECT_NAME}Export
to prevent conflicts.
@@ -1,6 +1,6 @@ | |||
add_executable(ompl_benchmark | |||
CFGBenchmark.cpp BenchmarkOptions.cpp BenchmarkTypes.cpp benchmark.cpp) | |||
target_link_libraries(ompl_benchmark ${OMPLAPP_LIBRARIES} ompl ompl_app_base ${Boost_PROGRAM_OPTIONS_LIBRARY}) | |||
target_link_libraries(ompl_benchmark ${OMPLAPP_LIBRARIES} ompl ompl_app_base Boost::program_options) |
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_link_libraries(ompl_benchmark ${OMPLAPP_LIBRARIES} ompl ompl_app_base Boost::program_options) | |
target_link_libraries(ompl_benchmark ompl::omplapp ompl ompl_app_base Boost::program_options) |
If you can, try linking to an ALIAS targets for all of these. They don't all exist, but are easily created for the internal ones..
I tried your PR in an isolated environment, and it didn't compile. After cloning and changing directory into the repo:
Results in
Note I use slightly different build instructions rather than calling make directly, but it works much the same, with less typing. |
The docker build error looks to me like libassimp-dev's Debian package is broken and doesn't declare a dependency on zlib1g-dev. Once you install the later, the build succeeds |
This PR mirrors the changes in ompl/ompl#1066.
The pkg-config file ompl.pc that is generated is actually broken. The same is true for the ompl repo. I'm thinking we should just remove the png-config related stuff.
I noticed that in the ompl repo, libompl gets installed in CMAKE_INSTALL_DATAROOTDIR (i.e., /usr/share) rather than CMAKE_INSTALL_LIBDIR (i.e. /usr/lib). That is fixed in this PR for omplapp, but if this is right, we need to make the same change in ompl.