Order target interfaces instead of their include directories

[jacobperron]

Attempt to fix ros2/ros2#1150

Previously, we were sorting the include directories of dependencies and manually calling target_include_directories. This change instead relies on CMake's target_link_libraries for include directories. We still must order the dependencies passed to target_link_libraries to handle overlays.

The same logic is used for sorting based on include directories, though this heuristic could be replaced with something else if there's a better proposal.

[jacobperron]

@hexonxons I've updated the implementation to use AMENT_PREFIX_PATH. Let me know what you think.

[jacobperron]

Triggering CI to see if this breaks anything obvious:

• Linux
• Linux-aarch64
• macOS
• Windows

[hexonxons]

It seems transitive dependencies are still lost

E.g

• package_a depends on package_b
• package_b depends onrclcpp (located in /opt/ros/foxy) and some package_c

In that case for package_a:

• AMENT_PREFIX_PATH will be set to /some/path/to/package_b_install/:/some/path/to/package_c_install/:/opt/ros/foxy
• ament_target_dependencies_order(ordered_interfaces ${interfaces}) will be called with interfaces set to package_b
• _ament_target_dependencies_order will be called with output_targets set to package_b
• get_target_property(include_dirs ${interface} INTERFACE_INCLUDE_DIRECTORIES) will return /some/path/to/package_b_install/include

cuz rclcpp and package_c will be set to INTERFACE_LINK_LIBRARIES property of package_b target.

So finally include path will be -isystem /some/path/to/package_b_install/include -isystem /opt/ros/foxy -isystem /some/path/to/package_c_install/ and headers from /opt/ros/foxy will be used.

Correct me if I wrong, I haven't tested it on real packages.

That's why in my example I was traversing all targets via ament_get_recursive_properties :(

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

[gavanderhoorn]

Probably because this didn't work for all cases as described by @hexonxons, but could you add a final comment here @jacobperron why this PR was closed? For 'future readers'?

[jacobperron]

Correct, the changes in the PR did not handle all cases. I decided to close this since, as pointed out in ros2/ros2#1150 (comment) (option 1), a general solution requires a lot more effort to make work, and I simply do not have the bandwidth to continue.