fix(cmake): Broken export of header files an library targets. #128
Conversation
…broken export of header files and library targets.
There was a problem hiding this comment.
Good afternoon,
First of all thank you very much, I greatly appreciate the contribution!
I have some comments that needs to be taken care off.
Don't hesitate to ask if you think that my comments are unclear or if you disagree with me.
Best Regards,
PAL Alliance = PAL ROBOTICS + TOWARD Teams.
| if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang") | ||
| add_compile_options(-Wall -Wextra -Wpedantic) | ||
| if(NOT CMAKE_CXX_STANDARD) | ||
| set(CMAKE_CXX_STANDARD 20) |
There was a problem hiding this comment.
Is this absolutely required?
I believe that the minimal C++ version before and after this PR is C++11 which most recent compilers supports nowadays by default.
Hence I would remove this line entirely.
It's better practice to simply use the default once on your OS.
| PUBLIC | ||
| include) | ||
| $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include> | ||
| $<INSTALL_INTERFACE:include/${PROJECT_NAME}>) |
There was a problem hiding this comment.
This should only be:
$<INSTALL_INTERFACE:include>As you are supposed to install the content of the <package_name>/include folder in the include destination.
This way the users will have to include: #include "project_name/my_headers.hpp" which is crystal clear.
| ${OpenCV_INCLUDE_DIRS} | ||
| ${aruco_INCLUDE_DIRS} |
There was a problem hiding this comment.
why including the aruco's include but not the libraries?
| include) | ||
| ${aruco_LIBRARIES} | ||
| ${OpenCV_LIBRARIES} | ||
| ${visualization_msgs_TARGETS} |
There was a problem hiding this comment.
These three lines are useless as the target ${PROJECT_NAME}_utils already provide these informations.
So target_link_libraries(single ${PROJECT_NAME}_utils) is enough
There was a problem hiding this comment.
Though one usually add these lines by default anyway:
target_include_directories(single
PUBLIC
include)
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
$<INSTALL_INTERFACE:include>)| SYSTEM PUBLIC | ||
| image_transport::image_transport | ||
| ) | ||
| target_include_directories(single PUBLIC | ||
| ${OpenCV_INCLUDE_DIRS} |
There was a problem hiding this comment.
Also useless as the dependency on ${PROJECT_NAME}_utils provide this as well.
| ${OpenCV_INCLUDE_DIRS} | ||
| ) | ||
| ament_target_dependencies(single ${THIS_PACKAGE_INCLUDE_DEPENDS}) | ||
| target_link_libraries(single ${OpenCV_LIBRARIES}) | ||
|
|
||
| add_executable(double src/simple_double.cpp | ||
| src/aruco_ros_utils.cpp) |
There was a problem hiding this comment.
This cpp file should not be there as it is compiled before in ${PROJECT_NAME}_utils and linked to this executable.
| ${OpenCV_INCLUDE_DIRS} | ||
| ) | ||
| ament_target_dependencies(double ${THIS_PACKAGE_INCLUDE_DEPENDS}) | ||
| target_link_libraries(double ${OpenCV_LIBRARIES}) | ||
|
|
||
| add_executable(marker_publisher src/marker_publish.cpp | ||
| src/aruco_ros_utils.cpp) |
There was a problem hiding this comment.
This cpp file should not be there as it is compiled before in ${PROJECT_NAME}_utils and linked to this executable.
| ${aruco_msgs_LIBRARIES} | ||
| ${OpenCV_LIBRARIES} | ||
|
|
||
| image_transport::image_transport |
There was a problem hiding this comment.
one could consider adding the image_transport::image_transport dependency to the ${PROJECT_NAME}_utils to simplify the CMakeLists.txt
| target_link_libraries(marker_publisher ${OpenCV_LIBRARIES}) | ||
|
|
||
| if(BUILD_TESTING) | ||
| find_package(ament_lint_auto REQUIRED) |
There was a problem hiding this comment.
Why would we remove the unit-tests?
| ## Install ## | ||
| ############# | ||
| # Install library and resources. | ||
| install(DIRECTORY include/ |
There was a problem hiding this comment.
considering the single header of this package I would recommend a simpler version of this:
install(DIRECTORY include/ DESTINATION include)
The aruco_ros package in its current state doesn't export its header files, since it searches for ".h" files, but uses a ".hpp" instead. Also, it seems not to export any of the usual variables pointing towards the library. I reimplemented the CMakeLists.txt of aruco_ros to fix the broken export of header files and library targets. It now can be included referencing "${aruco_ros_TARGETS}" and "${aruco_ros_INCLUDE_DIRS}".