From a284c007f375d59c71cb261c6ce7e6b253d0d4be Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Sun, 13 Jun 2021 14:30:19 -0700 Subject: [PATCH 1/3] Rely on target_link_libraries to add include directories Fixes https://github.com/ros2/ros2/issues/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 --- .../cmake/ament_target_dependencies.cmake | 30 +++++++++++++------ 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake b/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake index 50bb69c7..f135dbcc 100644 --- a/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake +++ b/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake @@ -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}) @@ -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}) + 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) From 28ba07042033be0f7faa0c295a2c45321dc8415b Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Sun, 13 Jun 2021 16:25:39 -0700 Subject: [PATCH 2/3] Use AMENT_PREFIX_PATH to order interfaces Similar to how we order include directories with ament_include_directories_order. Moved the implementation to a separate function. Signed-off-by: Jacob Perron --- ...ent_cmake_target_dependencies-extras.cmake | 2 + .../cmake/ament_target_dependencies.cmake | 31 ++---- .../ament_target_dependencies_order.cmake | 94 +++++++++++++++++++ 3 files changed, 102 insertions(+), 25 deletions(-) create mode 100644 ament_cmake_target_dependencies/cmake/ament_target_dependencies_order.cmake diff --git a/ament_cmake_target_dependencies/ament_cmake_target_dependencies-extras.cmake b/ament_cmake_target_dependencies/ament_cmake_target_dependencies-extras.cmake index 386608b8..c679525b 100644 --- a/ament_cmake_target_dependencies/ament_cmake_target_dependencies-extras.cmake +++ b/ament_cmake_target_dependencies/ament_cmake_target_dependencies-extras.cmake @@ -21,5 +21,7 @@ find_package(ament_cmake_libraries QUIET REQUIRED) include( "${ament_cmake_target_dependencies_DIR}/ament_get_recursive_properties.cmake") +include( + "${ament_cmake_target_dependencies_DIR}/ament_target_dependencies_order.cmake") include( "${ament_cmake_target_dependencies_DIR}/ament_target_dependencies.cmake") diff --git a/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake b/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake index f135dbcc..83bd3964 100644 --- a/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake +++ b/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake @@ -128,34 +128,15 @@ function(ament_target_dependencies target) if(NOT ARG_INTERFACE) target_compile_definitions(${target} ${required_keyword} ${definitions}) - - # 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) - # 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}) - endif() - else() - list(APPEND sorted_interfaces ${interface}) - endif() - endforeach() endif() + # Order interfaces 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 + ament_target_dependencies_order(ordered_interfaces ${interfaces}) ament_include_directories_order(ordered_include_dirs ${include_dirs}) target_link_libraries(${target} - ${optional_keyword} ${sorted_interfaces}) + ${optional_keyword} ${ordered_interfaces}) target_include_directories(${target} ${system_keyword} ${required_keyword} ${ordered_include_dirs}) if(NOT ARG_INTERFACE) diff --git a/ament_cmake_target_dependencies/cmake/ament_target_dependencies_order.cmake b/ament_cmake_target_dependencies/cmake/ament_target_dependencies_order.cmake new file mode 100644 index 00000000..e7f58438 --- /dev/null +++ b/ament_cmake_target_dependencies/cmake/ament_target_dependencies_order.cmake @@ -0,0 +1,94 @@ +# Copyright 2021 Open Source Robotics Foundation, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# +# Order targets according to the chain of ament prefixes. +# +# :param output_targets: the output list of ordered targets. +# :type output_targets: list of targets +# :param ARGN: a list of targets. +# :type ARGN: list of targets +# +# @public +# +macro(ament_target_dependencies_order output_targets) + set(_ament_prefix_path_list "$ENV{AMENT_PREFIX_PATH}") + if(NOT WIN32) + string(REPLACE ":" ";" _ament_prefix_path_list "${_ament_prefix_path_list}") + endif() + _ament_target_dependencies_order(${output_targets} "${_ament_prefix_path_list}" ${ARGN}) +endmacro() + + +# This implementation was copied and adapted from "ament_include_directories_order" +function(_ament_target_dependencies_order output_targets prefixes) + # create list of empty slots, one per prefix and one for unknown prefixes + list(LENGTH prefixes prefix_count) + foreach(index RANGE ${prefix_count}) + set(slot_${index} "") + endforeach() + + # append the target to the first prefix matching any of it's include directories + foreach(interface ${ARGN}) + get_target_property(include_dirs ${interface} INTERFACE_INCLUDE_DIRECTORIES) + + set(match FALSE) + foreach(include_dir ${include_dirs}) + string(LENGTH "${include_dir}" include_dir_length) + + set(index 0) + while(TRUE) + if(NOT ${index} LESS ${prefix_count}) + # no match + break() + endif() + + list(GET prefixes ${index} prefix) + # exact match + if(prefix STREQUAL include_dir) + set(match TRUE) + break() + endif() + + string(LENGTH "${prefix}" prefix_length) + if(${prefix_length} LESS ${include_dir_length}) + math(EXPR prefix_length_plus_one "${prefix_length} + 1") + string(SUBSTRING "${include_dir}" + 0 ${prefix_length_plus_one} include_dir_prefix) + # prefix match + if("${prefix}/" STREQUAL "${include_dir_prefix}") + set(match TRUE) + break() + endif() + endif() + + math(EXPR index "${index} + 1") + endwhile() + if(match) + list(APPEND slot_${index} ${interface}) + break() + endif() + endforeach() + if(NOT match) + list(APPEND slot_${index} ${interface}) + endif() + endforeach() + + # join the slot lists + set(ordered "") + foreach(index RANGE ${prefix_count}) + list(APPEND ordered ${slot_${index}}) + endforeach() + set(${output_targets} "${ordered}" PARENT_SCOPE) +endfunction() From aabecfd0774754ea2ff664098409c14c7622a780 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Sun, 13 Jun 2021 16:37:28 -0700 Subject: [PATCH 3/3] Remove vestigial variable Signed-off-by: Jacob Perron --- .../cmake/ament_target_dependencies.cmake | 1 - 1 file changed, 1 deletion(-) diff --git a/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake b/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake index 83bd3964..ef952616 100644 --- a/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake +++ b/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake @@ -70,7 +70,6 @@ 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})