Fix include order: correctly consider _all_ dependencies recursively

[rhaschke]

I ran into a similar issue as described by @jacobperron in https://github.com/jacobperron/overlay_bug_reproduction.
However, in my case, transitive dependencies were involved - which, surprisingly, are not resolved at all.

I modified the example slightly. I can be found in this repo.
There are four packages with the following dependencies:

• package_c -> package_a, package_b
• package_d -> package_c

package_a exists in both the underlay and overlay workspace, with a modified version in the overlay.
package_c depends on the modified version of package_a (in the overlay) and package_b in the underlay.
package_d depends on package_c and transitively on package_a and package_b.

When building package_c, its interface dependencies are explicitly sorted.

However, when building package_d, it doesn't report INTERFACE_INCLUDE_DIRECTORIES of transitive dependencies. Thus, the include directories of a and b are not considered for sorting in above-mentioned code, but are used according to the order given in ament_target_dependencies(package_c ...). Indeed, changing the order there would fix the issue.

Solution Proposal

Obviously, sorting of include directories should consider all dependencies.
Actually, #259 already implemented a function to recursively retrieve all include dirs. However, #260, which introduced the code above, didn't consider that?!

I was really surprised to hit such a fundamental issue. Are transitive dependencies not used/allowed in ROS2?

[rhaschke]

I suggest skipping ordering of include dirs in ament_include_directories_order() as we need to reorder here anyway - including the non-interface include directories. However, this will change behavior.

[rhaschke]

Note that INTERFACE_INCLUDE_DIRECTORIES might comprise generator expressions. In that case, the manual sorting performed by ament_cmake will break too (these expressions are not returned in resolved form). Not sure, cmake provides means to resolve those expressions.

[sloretz]

I was really surprised to hit such a fundamental issue. Are transitive dependencies not used/allowed in ROS2?

This was worked on in ros2/ros2#1150, and this comment has some options considered. We decided to have all packages install their headers to a unique include directory instead of catkin-like include directory sorting.

Note that INTERFACE_INCLUDE_DIRECTORIES might comprise generator expressions. In that case, the manual sorting performed by ament_cmake will break too (these expressions are not returned in resolved form). Not sure, cmake provides means to resolve those expressions.

This was one of the reasons we decided not to rely on include directory sorting.

Separately, now that packages are using modern CMake targets I'd recommend using target_link_libraries() directly instead of ament_target_dependencies() #292.

[rhaschke]

I see. Unfortunately, I ran into this issue on Ubuntu Focal, running ROS Galactic 😢
Note, that the override warning mentioned in ros2/ros2#1150 (comment) didn't show up in my example.

[rhaschke]

Feel free to close this issue, if you think it's superseded.
Given that ament_target_dependencies() implements include dir sorting, I think this patch should be applied to fix an obvious bug. Maybe, this PR should be targeted against the galactic branch instead.
And yes, agreed, one could directly use target_link_libraries() now.

[rhaschke]

I rebased and re-targeted this PR against galactic now.

[rhaschke]

Closing and reopening to retrigger CI.