CMakeLists.txt: fixing relocatable package issue #19
Conversation
CMakeLists.txt
Outdated
|
|
||
| install( | ||
| DIRECTORY include/ | ||
| DESTINATION include |
There was a problem hiding this comment.
Since we're modifying this, mind making it install the headers to a unique include directory? It's allows overriding in a merged colcon or ament workspace: https://colcon.readthedocs.io/en/released/user/overriding-packages.html#install-headers-to-a-unique-include-directory
There was a problem hiding this comment.
Actually, I really do not get the point of why in the world installing into include/foo/foo/foo.hpp would be a thing. I have read the paragraph, but it only advises to do so, not why. I have seen (core) packages doing it as well as others not doing it. Also, this is the behavior it had before, did it not?
There was a problem hiding this comment.
Actually, I really do not get the point of why in the world installing into include/foo/foo/foo.hpp would be a thing.
It's so that when one package overrides another, the overriding package's headers will be included regardless of the order that include directories are given in CMake. There's more info here: https://colcon.readthedocs.io/en/released/user/overriding-packages.html#include-directory-search-order-problem . ROS 1 solved it by ordering the include directories in find_package(catking REQUIRED COMPONENTS pkg1 pkg2), but that was no longer an option with modern CMake targets.
I have seen (core) packages doing it as well as others not doing it
Which ones? The core packages should all be using it by now.
There was a problem hiding this comment.
I have asked this question https://answers.ros.org/question/397356/ros2-conventions-on-installing-headers/
CMakeLists.txt
Outdated
| ) | ||
|
|
||
| ament_target_dependencies(${PROJECT_NAME} Eigen3) | ||
| ament_export_targets(${PROJECT_NAME}Targets HAS_LIBRARY_TARGET) |
There was a problem hiding this comment.
IIRC HAS_LIBRARY_TARGET means "has a shared library target", and causes an environment hook for LD_LIBRARY_PATH to be set. It shouldn't be set here because only an interface target is being used.
There was a problem hiding this comment.
LD_LIBRARY_PATH has nothing to do with it. The "library" is declared as INTERFACE and therefore is header only
There was a problem hiding this comment.
LD_LIBRARY_PATH has nothing to do with it.
HAS_LIBRARY_TARGET causes LD_LIBRARY_PATH to be set when the workspace containing this package is sourced. We don't need LD_LIBRARY_PATH because, as you say, this library is header only.
CMakeLists.txt
Outdated
| add_library(${PROJECT_NAME} INTERFACE | ||
| include/${PROJECT_NAME}/eigen_stl_containers.h | ||
| include/${PROJECT_NAME}/eigen_stl_map_container.h | ||
| include/${PROJECT_NAME}/eigen_stl_vector_container.h |
There was a problem hiding this comment.
I don't think these files will need to be specified as long as we set the install interface include directories correctly through target_include_directories()
There was a problem hiding this comment.
This is a difficult call for me, I know that it cmake does not yield a correct project for Microsoft Visual Studio if you do not mention these files here. But I use no Windows, so idk. It should not hurt at least.
CMakeLists.txt
Outdated
| include/${PROJECT_NAME}/eigen_stl_vector_container.h | ||
| ) | ||
|
|
||
| ament_target_dependencies(${PROJECT_NAME} Eigen3) |
There was a problem hiding this comment.
I'd recommend using target_link_libraries() in new code. ament/ament_cmake#292
There was a problem hiding this comment.
Oh, I missed this one. If you could change this to:
| ament_target_dependencies(${PROJECT_NAME} Eigen3) | |
| target_link_libraries(${PROJECT_NAME} INTERFACE Eigen3::Eigen3) |
I think that should do it.
|
Groomed through my old PR, updated this. Friendly bump |
As far as I can tell, we are still waiting for two changes to this PR:
Once those two are fixed, I think we can go ahead with another review of this. |
|
Hi @clalancette I did the requested changes. Thanks and regards, |
CMakeLists.txt
Outdated
| include/${PROJECT_NAME}/eigen_stl_vector_container.h | ||
| ) | ||
|
|
||
| ament_target_dependencies(${PROJECT_NAME} Eigen3) |
There was a problem hiding this comment.
Oh, I missed this one. If you could change this to:
| ament_target_dependencies(${PROJECT_NAME} Eigen3) | |
| target_link_libraries(${PROJECT_NAME} INTERFACE Eigen3::Eigen3) |
I think that should do it.
|
@clalancette yes, sure. done. |
With the current CMakeLists, the installed package will have an absolute path, which breaks if the package gets relocated later on. This patch tries to set up a proper header only ros2 package. Signed-off-by: Matthias Schoepfer <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
With the current CMakeLists, the installed package will have an absolute path, which breaks if the package gets relocated later on.
This patch tries to set up a proper header only ros2 package.