Skip to content
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.

Sign up for GitHub

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

fix(cmake): Broken export of header files an library targets. #128

Open
wants to merge 1 commit into
base: humble-devel
Choose a base branch
from
Open

fix(cmake): Broken export of header files an library targets. #128

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 79 additions & 67 deletions aruco_ros/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
cmake_minimum_required(VERSION 3.8)
cmake_minimum_required(VERSION 3.10.2)
project(aruco_ros)

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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

endif()
set(CMAKE_CXX_STANDARD 11) # C++11...
set(CMAKE_CXX_STANDARD_REQUIRED ON) #...is required...
set(CMAKE_CXX_EXTENSIONS ON) #...with compiler extensions like gnu++11

set(THIS_PACKAGE_INCLUDE_DEPENDS
# Public dependencies.
set(dependencies
OpenCV
cv_bridge
geometry_msgs
Expand All @@ -23,98 +21,112 @@ set(THIS_PACKAGE_INCLUDE_DEPENDS
sensor_msgs
visualization_msgs
)

find_package(ament_cmake REQUIRED)
foreach(Dependency IN ITEMS ${THIS_PACKAGE_INCLUDE_DEPENDS})
find_package(${Dependency} REQUIRED)
foreach(DEP ${dependencies})
find_package(${DEP} REQUIRED)
endforeach()

#generate_dynamic_reconfigure_options(
# cfg/ArucoThreshold.cfg
#)

add_library(aruco_ros_utils SHARED src/aruco_ros_utils.cpp)
target_include_directories(aruco_ros_utils
# Build library.
add_library(${PROJECT_NAME}_utils SHARED
src/aruco_ros_utils.cpp
)
target_include_directories(${PROJECT_NAME}_utils
PUBLIC
include)
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
$<INSTALL_INTERFACE:include/${PROJECT_NAME}>)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


target_include_directories(aruco_ros_utils
SYSTEM PUBLIC
target_link_libraries(${PROJECT_NAME}_utils PUBLIC
${OpenCV_LIBRARIES}
tf2_geometry_msgs::tf2_geometry_msgs
cv_bridge::cv_bridge
)

target_include_directories(${PROJECT_NAME}_utils PUBLIC
${OpenCV_INCLUDE_DIRS}
${aruco_INCLUDE_DIRS}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why including the aruco's include but not the libraries?

${sensor_msgs_INCLUDE_DIRS}
${visualization_msgs_INCLUDE_DIRS}
)
ament_target_dependencies(aruco_ros_utils ${THIS_PACKAGE_INCLUDE_DEPENDS})
target_link_libraries(aruco_ros_utils ${OpenCV_LIBRARIES})

# Build executables.
add_executable(single src/simple_single.cpp
src/aruco_ros_utils.cpp)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cpp file should not be there as it is compiled before in ${PROJECT_NAME}_utils and linked to this executable.

target_link_libraries(single
${PROJECT_NAME}_utils

target_include_directories(single
PUBLIC
include)
${aruco_LIBRARIES}
${OpenCV_LIBRARIES}
${visualization_msgs_TARGETS}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>)


target_include_directories(single
SYSTEM PUBLIC
image_transport::image_transport
)
target_include_directories(single PUBLIC
${OpenCV_INCLUDE_DIRS}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also useless as the dependency on ${PROJECT_NAME}_utils provide this as well.

)
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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cpp file should not be there as it is compiled before in ${PROJECT_NAME}_utils and linked to this executable.

target_include_directories(double
PUBLIC
include)
target_link_libraries(double
${PROJECT_NAME}_utils

${aruco_LIBRARIES}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comments.

${OpenCV_LIBRARIES}

target_include_directories(double
SYSTEM PUBLIC
image_transport::image_transport
)
target_include_directories(double PUBLIC
${OpenCV_INCLUDE_DIRS}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comments on the single target.

)
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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cpp file should not be there as it is compiled before in ${PROJECT_NAME}_utils and linked to this executable.

target_include_directories(marker_publisher
PUBLIC
include)

target_include_directories(marker_publisher
SYSTEM PUBLIC
target_link_libraries(marker_publisher
${PROJECT_NAME}_utils

${aruco_LIBRARIES}
${aruco_msgs_LIBRARIES}
${OpenCV_LIBRARIES}

image_transport::image_transport
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one could consider adding the image_transport::image_transport dependency to the ${PROJECT_NAME}_utils to simplify the CMakeLists.txt

)
target_include_directories(marker_publisher PUBLIC
${OpenCV_INCLUDE_DIRS}
${aruco_msgs_INCLUDE_DIRS}
)
ament_target_dependencies(marker_publisher ${THIS_PACKAGE_INCLUDE_DEPENDS})
target_link_libraries(marker_publisher ${OpenCV_LIBRARIES})

if(BUILD_TESTING)
find_package(ament_lint_auto REQUIRED)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we remove the unit-tests?


list(APPEND AMENT_LINT_AUTO_EXCLUDE
ament_cmake_copyright
)
ament_lint_auto_find_test_dependencies()
endif()

#############
## Install ##
#############
# Install library and resources.
install(DIRECTORY include/
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

considering the single header of this package I would recommend a simpler version of this:

install(DIRECTORY include/ DESTINATION include)

DESTINATION include/${PROJECT_NAME}
FILES_MATCHING PATTERN "*.hpp"
)

install(TARGETS aruco_ros_utils
install(TARGETS ${PROJECT_NAME}_utils
EXPORT export_${PROJECT_NAME}_utils
ARCHIVE DESTINATION lib
LIBRARY DESTINATION lib
RUNTIME DESTINATION bin)

install(TARGETS marker_publisher single double
DESTINATION lib/${PROJECT_NAME})
RUNTIME DESTINATION bin
)

install(DIRECTORY include/
DESTINATION include
FILES_MATCHING PATTERN "*.h"
install(TARGETS single
DESTINATION lib/${PROJECT_NAME}
)

foreach(dir etc launch)
install(DIRECTORY ${dir}/
DESTINATION share/${PROJECT_NAME}/${dir})
endforeach()
install(TARGETS double
DESTINATION lib/${PROJECT_NAME}
)

ament_package()
install(TARGETS marker_publisher
DESTINATION lib/${PROJECT_NAME}
)

install(
DIRECTORY launch
DESTINATION share/${PROJECT_NAME}
)

# Exports.
ament_export_include_directories(include)
ament_export_targets(export_${PROJECT_NAME}_utils HAS_LIBRARY_TARGET)
ament_export_dependencies(${dependencies})
ament_package()