Skip to content

Commit

Permalink
Rely on target_link_libraries to add include directories
Browse files Browse the repository at this point in the history
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]>
  • Loading branch information
jacobperron committed Jun 13, 2021
1 parent 3f992ad commit a284c00
Showing 1 changed file with 21 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ function(ament_target_dependencies target)
set(definitions "")
set(include_dirs "")
set(interfaces "")
set(sorted_interfaces "")
set(libraries "")
set(link_flags "")
foreach(package_name ${ARG_UNPARSED_ARGUMENTS})
Expand Down Expand Up @@ -127,23 +128,34 @@ 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)

# Order interface to support workspace overlaying
# Interfaces built in the same workspace should appear before those in an underlay
# This fixes issues related to overlaying on a "merged" workspace, where includes from
# multiple packages appear in the same directory
set(_install_prefix ${CMAKE_INSTALL_PREFIX})
get_filename_component(_install_prefix_parent ${_install_prefix} DIRECTORY)
foreach(interface ${interfaces})
get_target_property(_include_dirs ${interface} INTERFACE_INCLUDE_DIRECTORIES)
# If include directory has the same parent as the target, then make sure it appears at the
# beginning of the list
if(_include_dirs)
list_append_unique(interface_include_dirs ${_include_dirs})
# TODO: handle case that _include_dirs is a list
if(_include_dirs MATCHES "^${_install_prefix}.*")
list(PREPEND sorted_interfaces ${interface})
elseif(_include_dirs MATCHES "^${_install_prefix_parent}.*")
list(PREPEND sorted_interfaces ${interface})
else()
list(APPEND sorted_interfaces ${interface})

This comment has been minimized.

Copy link
@hexonxons

hexonxons Jun 13, 2021

What if we have several overlays? So I built some set of packages in overlay A, sourced it and built other set n overlay B. All includes from overlay A will not be sorted.
ament_include_directories_order handles AMENT_PREFIX_PATH variable and it seems like paths in AMENT_PREFIX_PATH are already sorted in right way

endif()
else()
list(APPEND sorted_interfaces ${interface})
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()
ament_include_directories_order(ordered_include_dirs ${include_dirs})
target_link_libraries(${target}
${optional_keyword} ${interfaces})
${optional_keyword} ${sorted_interfaces})
target_include_directories(${target} ${system_keyword}
${required_keyword} ${ordered_include_dirs})
if(NOT ARG_INTERFACE)
Expand Down

0 comments on commit a284c00

Please sign in to comment.