-
Notifications
You must be signed in to change notification settings - Fork 688
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
Overlaying packages using CMake export targets can fail with merge install underlay #1150
Comments
Seems like problem is due to ament_target_dependencies.cmake. While debugging same problem with ros2/rosbag2#687: ament_target_dependencies(${PROJECT_NAME}
rcpputils
rosbag2_compression
zstd) this code executed(link): if(NOT ARG_INTERFACE)
target_compile_definitions(${target}
${required_keyword} ${definitions})
# the interface include dirs must be ordered
set(interface_include_dirs)
foreach(interface ${interfaces})
get_target_property(_include_dirs ${interface} INTERFACE_INCLUDE_DIRECTORIES)
if(_include_dirs)
list_append_unique(interface_include_dirs ${_include_dirs})
endif()
endforeach()
ament_include_directories_order(ordered_interface_include_dirs ${interface_include_dirs})
# the interface include dirs are used privately to ensure proper order
# and the interfaces cover the public case
target_include_directories(${target} ${system_keyword}
PRIVATE ${ordered_interface_include_dirs})
endif() with
accordingly.
Also, according to same documentation:
So fast idea for this to work we need to recursively get dependencies from |
@hexonxons Thanks for posting your investigation! The relevant ament_cmake code was added in ament/ament_cmake#260 to fix a similar issue related to overlays (ros2/geometry2#268). (I had forgotten about it even though I reviewed it 😅). I think your assessment is correct. The current code only orders include directories for the declared dependencies, it is not considering transitive dependencies. Though, with my current understanding of CMake, I agree with this comment, and CMake should already be able to handle the include directories of the transitive dependencies for us. I tried the applying the following change: --- a/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake
+++ b/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake
@@ -127,19 +127,7 @@ function(ament_target_dependencies target)
if(NOT ARG_INTERFACE)
target_compile_definitions(${target}
${required_keyword} ${definitions})
- # the interface include dirs must be ordered
- set(interface_include_dirs)
- foreach(interface ${interfaces})
- get_target_property(_include_dirs ${interface} INTERFACE_INCLUDE_DIRECTORIES)
- if(_include_dirs)
- list_append_unique(interface_include_dirs ${_include_dirs})
- endif()
- endforeach()
- ament_include_directories_order(ordered_interface_include_dirs ${interface_include_dirs})
- # the interface include dirs are used privately to ensure proper order
- # and the interfaces cover the public case
- target_include_directories(${target} ${system_keyword}
- PRIVATE ${ordered_interface_include_dirs})
+ target_link_libraries(${target} ${interfaces}) This gives us control over the order that include directories are passed to the compiler. For example, if I make the following change to my reproduction, then I can successfully compile the packages: --- a/overlay/src/package_d/CMakeLists.txt
+++ b/overlay/src/package_d/CMakeLists.txt
@@ -13,10 +13,10 @@ add_library(${PROJECT_NAME}
src/package_d.cpp
)
ament_target_dependencies(${PROJECT_NAME}
- package_b
# Targeting package_c causese underlay path to appear before overlay
# The underlay contains an older version of package_a that package_b is not expecting
package_c
+ package_b I'm not sure if this is a desirable solution since it makes It seems like we need a way to detect if a dependency is in the overlay or not. Then we could order the target dependencies such that dependencies in the same overlay as the package being built appear first. |
@jacobperron Yep, cmake is handling include directories of the transitive dependencies. Compile commands for
So in first case ament_target_dependencies(${PROJECT_NAME}
package_a
)
target_include_directories(${PROJECT_NAME}
PUBLIC
$<INSTALL_INTERFACE:include>
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
) How did I suggest to fix: if(NOT ARG_INTERFACE)
target_compile_definitions(${target}
${required_keyword} ${definitions})
# the interface include dirs must be ordered
set(interface_include_dirs)
set(interface_libraries)
foreach(interface ${interfaces})
set(current_interface_include_dirs)
set(current_interface_libraries)
ament_get_recursive_properties(current_interface_include_dirs current_interface_libraries ${interface})
message(WARNING "interface_include_dirs for target ${interface}: ${current_interface_include_dirs}")
message(WARNING "interface_libraries for target ${interface}: ${current_interface_libraries}")
list_append_unique(interface_include_dirs "${current_interface_include_dirs}")
list_append_unique(interface_libraries "${current_interface_libraries}")
endforeach()
message(WARNING "All interface_include_dirs: ${interface_include_dirs}")
message(WARNING "All interface_libraries: ${interface_libraries}")
ament_include_directories_order(ordered_interface_include_dirs ${interface_include_dirs})
# the interface include dirs are used privately to ensure proper order
# and the interfaces cover the public case
target_include_directories(${target} ${system_keyword}
PRIVATE ${ordered_interface_include_dirs})
endif() You could try to build your packages and see debugging warning messages :) |
But still that is very strange way of doing cmake job. In non-ros cmake projects if you have
So you will get So I prefer to build full ROS repo in case of building overlay workspaces. In this case |
Fixes ros2/ros2#1150 The current solution only orders the declared dependencies (and not their transitive dependencies). Instead or order the include directories, we can instead order the targets themselves based on a heuristic that checks if the dependencies are part of the current overlay or not. Dependencies that are part of the current overlay are prepended to the interface list so that we make sure to find their include directories before any in an underlay. Signed-off-by: Jacob Perron <[email protected]>
I pushed an experimental patch that relies on |
Nah, it wonk work so easy :( |
IIUC, this is what was happening before ament/ament_cmake#260 (ie. just try reverting ament/ament_cmake#260 and see what happens). I think we still need to worry about the order of targets passed to |
This ticket is about sorting include directories based on the order workspaces are overlayed, but that's not the only problem with overriding packages. Include directory search order matters anytime headers with the same name can be found in different directories. Here are few types of solutions that could address the include directory search order problem. Option 1: Reorder the include directories in CMake (ROS 1 does this)Catkin solved this with Something that makes this kind of solution harder in ROS 2 is the separation of the build tool (colcon) and the build system (CMake). Another reason this is hard is ROS 2 uses modern CMake targets instead of old-style CMake standard variables. Something that makes this very hard is Another thing that makes this kind of solution hard is it would almost certainly require all existing ROS packages to update their CMake code to use it. I don't think this option is worth the effort in ROS 2. Option 2: Make the include directories be installed to another prefix?Currently ROS 2 packages install headers # Instead of this
# install(DIRECTORY include/ DESTINATION include)
# Do this
install(DIRECTORY include/ DESTINATION include/${PROJECT_NAME}) This would require all packages in the underlay workspace to implement it. This alone would bring overriding in ROS 2 to a similar state as ROS 1, but without requiring packages to know about workspaces. Option 3: Can the build tool (colcon) or workspace hide duplicate includes with sandboxing?What if colcon detected when overriding packages, and made only the non-overridden packages from the underlay available? Maybe when overriding a package from an That sounds hard to do on one platform, let alone all of the supported ones. Option 4: Discourage overriding packages by warning or erroring about it?Another option is to make colcon warn or error when a package is being overridden. At build time (when
Maybe a user could silence warnings or allow building to proceed with a CLI flag like At run time (when sourcing an overlay workspace)
Maybe there's no way to silence this warning, but it wouldn't block the user. Personally I like this option the most because it gives an opportunity to warn about API/ABI issues. |
@sloretz Thanks for laying out those options! IIUC, ament/ament_cmake#343 proposes something close to option 1 (though I guess it's missing a recursive traversal of target dependencies). IMO, requiring calls to Option 3 is intriguing but sounds like the most challenging to implement IMO. +1 for option 4, regardless of whether another option is implemented since I don't see how we can do anything else reasonable to guard against ABI incompatibilities in the non-leaf package case. |
Yeah, totally agreed; regardless of what else we do, I think we should implement option 4 as a warning (maybe with a flag to disable). |
See colcon/colcon-core#449 and colcon/colcon-ros#129 for an implementation of option 4. It warns when |
This issue has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-10-28/22947/1 |
I agree with the concern about non-leaf packages leading to undefined behaviour, but once a warning is in place for those I think that the leaf package use case needs to be fixed urgently. This bug breaks a common and intended workflow, and I regularly lose time to the confusing compiler errors it can cause or having to work around it. Alternatively, if we're going to just not support overlaying packages then we should explicitly disable it by having colcon check for the same package name occurring multiple times across all sourced workspaces and throwing an error if it finds one. |
I'm not sure it needs to be fixed urgently as it's the easiest to work around. If the leaf package is uninstalled then the issue goes away. |
This issue has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-11-18/23209/1 |
If you're installing from binary debs, which is when this problem occurs most often due to the merged install space, then uninstalling the leaf package is often not possible. In my case the leaf packages are all rosbag2 packages, which can't be uninstalled without removing the |
Uninstalling is still possible, but I see how that makes it inconvenient. I tried uninstalling one of the rosbag2 debs and saw the other $ apt-cache depends ros-rolling-desktop | awk '/Depends/ {print $2}' | xargs apt-mark auto |
These are the packages that install headers and are depended on by After installing ros-rolling-desktop, packages that install files into /opt/ros/rolling/include/$ dpkg -S /opt/ros/rolling/include/ | sed 's/, /\n/g' | sed 's/:.*$//'
ros-rolling-turtlesim
ros-rolling-map-msgs
ros-rolling-rviz-default-plugins
ros-rolling-interactive-markers
ros-rolling-rosbag2-interfaces
ros-rolling-rviz-rendering
ros-rolling-laser-geometry
ros-rolling-rosbag2-cpp
ros-rolling-rosbag2-compression-zstd
ros-rolling-pcl-msgs
ros-rolling-robot-state-publisher
ros-rolling-tf2-msgs
ros-rolling-message-filters
ros-rolling-kdl-parser
ros-rolling-tf2-eigen
ros-rolling-tf2-ros
ros-rolling-tf2
ros-rolling-pluginlib
ros-rolling-rosbag2-transport
ros-rolling-rcl-lifecycle
ros-rolling-keyboard-handler
ros-rolling-lifecycle-msgs
ros-rolling-pybind11-vendor
ros-rolling-example-interfaces
ros-rolling-trajectory-msgs
ros-rolling-tf2-eigen-kdl
ros-rolling-stereo-msgs
ros-rolling-nav-msgs
ros-rolling-rclcpp-lifecycle
ros-rolling-joy
ros-rolling-diagnostic-msgs
ros-rolling-std-srvs
ros-rolling-geometry-msgs
ros-rolling-tf2-kdl
ros-rolling-pendulum-msgs
ros-rolling-angles
ros-rolling-composition-interfaces
ros-rolling-actionlib-msgs
ros-rolling-class-loader
ros-rolling-rcl-action
ros-rolling-rclcpp
ros-rolling-rosgraph-msgs
ros-rolling-statistics-msgs
ros-rolling-image-transport
ros-rolling-urdf-parser-plugin
ros-rolling-libstatistics-collector
ros-rolling-rosidl-typesupport-fastrtps-c
ros-rolling-fastrtps
ros-rolling-rcl
ros-rolling-rmw-dds-common
ros-rolling-rqt-gui-cpp
ros-rolling-cyclonedds
ros-rolling-rosidl-typesupport-fastrtps-cpp
ros-rolling-iceoryx-utils
ros-rolling-tf2-geometry-msgs
ros-rolling-urdfdom-headers
ros-rolling-libyaml-vendor
ros-rolling-rosidl-typesupport-c
ros-rolling-action-msgs
ros-rolling-iceoryx-binding-c
ros-rolling-rcl-yaml-param-parser
ros-rolling-action-tutorials-interfaces
ros-rolling-unique-identifier-msgs
ros-rolling-rosidl-typesupport-introspection-c
ros-rolling-rosidl-runtime-cpp
ros-rolling-rmw
ros-rolling-rosidl-typesupport-interface
ros-rolling-rosbag2-compression
ros-rolling-shared-queues-vendor
ros-rolling-rosbag2-storage-default-plugins
ros-rolling-rosbag2-storage
ros-rolling-visualization-msgs
ros-rolling-logging-demo
ros-rolling-image-tools
ros-rolling-std-msgs
ros-rolling-shape-msgs
ros-rolling-rcl-interfaces
ros-rolling-urdfdom
ros-rolling-rclcpp-components
ros-rolling-rcpputils
ros-rolling-rosidl-runtime-c
ros-rolling-rcutils
ros-rolling-iceoryx-posh
ros-rolling-builtin-interfaces
ros-rolling-ament-index-cpp
ros-rolling-console-bridge-vendor
ros-rolling-tlsf-cpp
ros-rolling-rcl-logging-interface
ros-rolling-tlsf
ros-rolling-rosidl-typesupport-introspection-cpp
ros-rolling-rosidl-typesupport-cpp
ros-rolling-qt-gui-cpp
ros-rolling-rqt-image-view
ros-rolling-urdf
ros-rolling-image-geometry
ros-rolling-depthimage-to-laserscan
ros-rolling-rmw-fastrtps-cpp
ros-rolling-foonathan-memory-vendor
ros-rolling-tracetools
ros-rolling-rviz-common
ros-rolling-rttest
ros-rolling-fastcdr
ros-rolling-intra-process-demo
ros-rolling-resource-retriever
ros-rolling-tf2-sensor-msgs
ros-rolling-teleop-twist-joy
ros-rolling-zstd-vendor
ros-rolling-tf2-bullet
ros-rolling-sensor-msgs
ros-rolling-orocos-kdl
ros-rolling-cv-bridge
ros-rolling-rclcpp-action
ros-rolling-pcl-conversions
ros-rolling-rmw-fastrtps-shared-cpp Another command to get the sources for repos that will likely need to be edited - except pcl_msgs. All of the msgs packages probably need edits to the C/C++ generator packages instead. Repos file for packages in ros-rolling-desktop that install headersdpkg -S /opt/ros/rolling/include/ | sed 's/, /\n/g' | sed 's/:.*$//' | sed 's/ros-rolling-//' | sed 's/-/_/g' | xargs echo | xargs rosinstall_generator --rosdistro rolling --format repos --upstream
The following repositories with an unknown upstream will be ignored: pcl_msgs
repositories:
ament_index:
type: git
url: https://github.com/ament/ament_index.git
version: 1.3.0
angles:
type: git
url: https://github.com/ros/angles.git
version: 1.12.4
class_loader:
type: git
url: https://github.com/ros/class_loader.git
version: 2.1.2
common_interfaces:
type: git
url: https://github.com/ros2/common_interfaces.git
version: 3.0.0
console_bridge_vendor:
type: git
url: https://github.com/ros2/console_bridge_vendor.git
version: 1.3.2
cyclonedds:
type: git
url: https://github.com/eclipse-cyclonedds/cyclonedds.git
version: 0.8.0
demos:
type: git
url: https://github.com/ros2/demos.git
version: 0.17.0
depthimage_to_laserscan:
type: git
url: https://github.com/ros-perception/depthimage_to_laserscan.git
version: 2.3.1
example_interfaces:
type: git
url: https://github.com/ros2/example_interfaces.git
version: 0.9.2
fastcdr:
type: git
url: https://github.com/eProsima/Fast-CDR.git
version: 1.0.20
fastrtps:
type: git
url: https://github.com/eProsima/Fast-DDS.git
version: 2.3.4
foonathan_memory_vendor:
type: git
url: https://github.com/eProsima/foonathan_memory_vendor.git
version: 1.2.0
geometry2:
type: git
url: https://github.com/ros2/geometry2.git
version: 0.19.0
iceoryx:
type: git
url: https://github.com/eclipse-iceoryx/iceoryx.git
version: 1.0.0
image_common:
type: git
url: https://github.com/ros-perception/image_common.git
version: 3.1.0
interactive_markers:
type: git
url: https://github.com/ros-visualization/interactive_markers.git
version: 2.3.0
joystick_drivers:
type: git
url: https://github.com/ros-drivers/joystick_drivers.git
version: 3.0.0
kdl_parser:
type: git
url: https://github.com/ros/kdl_parser.git
version: 2.5.0
keyboard_handler:
type: git
url: https://github.com/ros-tooling/keyboard_handler.git
version: 0.0.2
laser_geometry:
type: git
url: https://github.com/ros-perception/laser_geometry.git
version: 2.2.2
libstatistics_collector:
type: git
url: https://github.com/ros-tooling/libstatistics_collector.git
version: 1.1.0
libyaml_vendor:
type: git
url: https://github.com/ros2/libyaml_vendor.git
version: 1.2.0
message_filters:
type: git
url: https://github.com/ros2/message_filters.git
version: 4.2.0
navigation_msgs:
type: git
url: https://github.com/ros-planning/navigation_msgs.git
version: 2.1.0
orocos_kinematics_dynamics:
type: git
url: https://github.com/ros2/orocos_kinematics_dynamics.git
version: 3.3.3
perception_pcl:
type: git
url: https://github.com/ros-perception/perception_pcl.git
version: 2.4.0
pluginlib:
type: git
url: https://github.com/ros/pluginlib.git
version: 5.0.0
pybind11_vendor:
type: git
url: https://github.com/ros2/pybind11_vendor.git
version: 2.2.6
qt_gui_core:
type: git
url: https://github.com/ros-visualization/qt_gui_core.git
version: 2.1.0
rcl:
type: git
url: https://github.com/ros2/rcl.git
version: 4.0.0
rcl_interfaces:
type: git
url: https://github.com/ros2/rcl_interfaces.git
version: 1.1.0
rcl_logging:
type: git
url: https://github.com/ros2/rcl_logging.git
version: 2.2.0
rclcpp:
type: git
url: https://github.com/ros2/rclcpp.git
version: 13.1.0
rcpputils:
type: git
url: https://github.com/ros2/rcpputils.git
version: 2.3.0
rcutils:
type: git
url: https://github.com/ros2/rcutils.git
version: 5.0.0
realtime_support:
type: git
url: https://github.com/ros2/realtime_support.git
version: 0.11.0
resource_retriever:
type: git
url: https://github.com/ros/resource_retriever.git
version: 2.5.0
rmw:
type: git
url: https://github.com/ros2/rmw.git
version: 5.1.0
rmw_dds_common:
type: git
url: https://github.com/ros2/rmw_dds_common.git
version: 1.4.0
rmw_fastrtps:
type: git
url: https://github.com/ros2/rmw_fastrtps.git
version: 6.1.0
robot_state_publisher:
type: git
url: https://github.com/ros/robot_state_publisher.git
version: 2.7.0
ros2_tracing:
type: git
url: https://gitlab.com/ros-tracing/ros2_tracing.git
version: 3.1.0
rosbag2:
type: git
url: https://github.com/ros2/rosbag2.git
version: 0.11.0
rosidl:
type: git
url: https://github.com/ros2/rosidl.git
version: 3.0.0
rosidl_typesupport:
type: git
url: https://github.com/ros2/rosidl_typesupport.git
version: 1.4.1
rosidl_typesupport_fastrtps:
type: git
url: https://github.com/ros2/rosidl_typesupport_fastrtps.git
version: 2.0.3
rqt:
type: git
url: https://github.com/ros-visualization/rqt.git
version: 1.1.2
rqt_image_view:
type: git
url: https://github.com/ros-visualization/rqt_image_view.git
version: 1.1.1
rviz:
type: git
url: https://github.com/ros2/rviz.git
version: 9.0.0
teleop_twist_joy:
type: git
url: https://github.com/ros2/teleop_twist_joy.git
version: 2.4.3
tlsf:
type: git
url: https://github.com/ros2/tlsf.git
version: 0.5.2
turtlesim:
type: git
url: https://github.com/ros/ros_tutorials.git
version: 1.4.0
unique_identifier_msgs:
type: git
url: https://github.com/ros2/unique_identifier_msgs.git
version: 2.2.1
urdf:
type: git
url: https://github.com/ros2/urdf.git
version: 2.5.2
urdfdom:
type: git
url: https://github.com/ros/urdfdom.git
version: 2.3.5
urdfdom_headers:
type: git
url: https://github.com/ros/urdfdom_headers.git
version: 1.0.5
vision_opencv:
type: git
url: https://github.com/ros-perception/vision_opencv.git
version: 2.2.1 Repos checklist for implementation of Option 2
|
You have closed it but don't you give depreciation warnings for the feature fixed here? I wouldn't make significant changes for features to be depreciated. That looks pretty weak to me if so. (Actually I didn't encounter the problem mentioned here but read in the discussion. And I do not want to rely on ROS tooling as much as possible. Just wanted to switch from |
This issue has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/deprecate-ament-packages/26988/20 |
That is, the fallout from ros2/ros2#1150 should be mentioned for people who are using IDEs with Humble. While we are in here, do some minor fixes to the formatting of the release notes. Signed-off-by: Chris Lalancette <[email protected]>
That is, the fallout from ros2/ros2#1150 should be mentioned for people who are using IDEs with Humble. While we are in here, do some minor fixes to the formatting of the release notes. Signed-off-by: Chris Lalancette <[email protected]>
That is, the fallout from ros2/ros2#1150 should be mentioned for people who are using IDEs with Humble. While we are in here, do some minor fixes to the formatting of the release notes. Signed-off-by: Chris Lalancette <[email protected]> (cherry picked from commit d0120d0)
That is, the fallout from ros2/ros2#1150 should be mentioned for people who are using IDEs with Humble. While we are in here, do some minor fixes to the formatting of the release notes. Signed-off-by: Chris Lalancette <[email protected]> (cherry picked from commit d0120d0)
That is, the fallout from ros2/ros2#1150 should be mentioned for people who are using IDEs with Humble. While we are in here, do some minor fixes to the formatting of the release notes. Signed-off-by: Chris Lalancette <[email protected]> (cherry picked from commit d0120d0)
That is, the fallout from ros2/ros2#1150 should be mentioned for people who are using IDEs with Humble. While we are in here, do some minor fixes to the formatting of the release notes. Signed-off-by: Chris Lalancette <[email protected]> (cherry picked from commit d0120d0)
That is, the fallout from ros2/ros2#1150 should be mentioned for people who are using IDEs with Humble. While we are in here, do some minor fixes to the formatting of the release notes. Signed-off-by: Chris Lalancette <[email protected]> (cherry picked from commit d0120d0)
That is, the fallout from ros2/ros2#1150 should be mentioned for people who are using IDEs with Humble. While we are in here, do some minor fixes to the formatting of the release notes. Signed-off-by: Chris Lalancette <[email protected]> (cherry picked from commit d0120d0)
…#3707) That is, the fallout from ros2/ros2#1150 should be mentioned for people who are using IDEs with Humble. While we are in here, do some minor fixes to the formatting of the release notes. Signed-off-by: Chris Lalancette <[email protected]> (cherry picked from commit d0120d0) Co-authored-by: Chris Lalancette <[email protected]>
…#3708) That is, the fallout from ros2/ros2#1150 should be mentioned for people who are using IDEs with Humble. While we are in here, do some minor fixes to the formatting of the release notes. Signed-off-by: Chris Lalancette <[email protected]> (cherry picked from commit d0120d0) Co-authored-by: Chris Lalancette <[email protected]>
…#3709) That is, the fallout from ros2/ros2#1150 should be mentioned for people who are using IDEs with Humble. While we are in here, do some minor fixes to the formatting of the release notes. Signed-off-by: Chris Lalancette <[email protected]> (cherry picked from commit d0120d0) Co-authored-by: Chris Lalancette <[email protected]>
Some of these have been best practice for a while, and some of them will become best practice in the near future. 1. Move the include directory down one-level. This is to better support overlays and underlays; see ros2/ros2#1150 for the entire saga. The upshot is that all includes should be one more directory level down, and CMake will handle the differences. This has been best practice since Humble. 2. Switch from ament_target_dependencies() to target_link_libraries(). ament_target_dependencies was developed in the days before target_link_libraries() was fully capable, and nowadays target_link_libraries() is a superset of the functionality. There is one oddity here, in that in order to deal with ROS message packages, we have to use ${<msg_pkg_name>_TARGETS}, rather than the traditional CMake target like msg_pkg_name::msg_pkg_name. This is a bug in ROS that will eventually be fixed. 3. Export a CMake target from robot_localization. This means that downstream packages will be able to use target_link_libraries(<target> robot_localization::robot_localization) 4. Export all dependencies with ament_export_dependencies. This ensures that downstream project which rely on this one will be able to find all of the includes. Signed-off-by: Chris Lalancette <[email protected]>
Some of these have been best practice for a while, and some of them will become best practice in the near future. 1. Move the include directory down one-level. This is to better support overlays and underlays; see ros2/ros2#1150 for the entire saga. The upshot is that all includes should be one more directory level down, and CMake will handle the differences. This has been best practice since Humble. 2. Switch from ament_target_dependencies() to target_link_libraries(). ament_target_dependencies was developed in the days before target_link_libraries() was fully capable, and nowadays target_link_libraries() is a superset of the functionality. There is one oddity here, in that in order to deal with ROS message packages, we have to use ${<msg_pkg_name>_TARGETS}, rather than the traditional CMake target like msg_pkg_name::msg_pkg_name. This is a bug in ROS that will eventually be fixed. 3. Export a CMake target from robot_localization. This means that downstream packages will be able to use target_link_libraries(<target> robot_localization::robot_localization) 4. Export all dependencies with ament_export_dependencies. This ensures that downstream project which rely on this one will be able to find all of the includes. Signed-off-by: Chris Lalancette <[email protected]>
Some of these have been best practice for a while, and some of them will become best practice in the near future. 1. Move the include directory down one-level. This is to better support overlays and underlays; see ros2/ros2#1150 for the entire saga. The upshot is that all includes should be one more directory level down, and CMake will handle the differences. This has been best practice since Humble. 2. Switch from ament_target_dependencies() to target_link_libraries(). ament_target_dependencies was developed in the days before target_link_libraries() was fully capable, and nowadays target_link_libraries() is a superset of the functionality. There is one oddity here, in that in order to deal with ROS message packages, we have to use ${<msg_pkg_name>_TARGETS}, rather than the traditional CMake target like msg_pkg_name::msg_pkg_name. This is a bug in ROS that will eventually be fixed. 3. Export a CMake target from robot_localization. This means that downstream packages will be able to use target_link_libraries(<target> robot_localization::robot_localization) 4. Export all dependencies with ament_export_dependencies. This ensures that downstream project which rely on this one will be able to find all of the includes. Signed-off-by: Chris Lalancette <[email protected]>
Bug report
Required Info:
colcon build --merge-install
)Steps to reproduce issue
I've created some mock packages that reproduce the issue. These packages and instructions for reproducing can be found in this repository: https://github.com/jacobperron/overlay_bug_reproduction
Expected behavior
We are able to build a package that exists in an underlay ROS 2 installation and have other packages in the overlay workspace properly link against it.
Actual behavior
In some circumstances, subject to the dependency tree, the compiler is given the underlay include directory before the overlay include directories. If a package appears in both the overlay and underlay, then the underlay include directory will be used instead.
The text was updated successfully, but these errors were encountered: