include order can cause the wrong headers to be used

[wjwwood]

This manifested in this pr: ros2/rviz#243

Basically, the urdf from somewhere like /usr/include (not sure exactly where from) when they should have been found in the workspace where rviz was being built.

Catkin had a way to deal with this by sorting include directories by workspace folder order, placing all include paths outside of any of the workspaces last in the order. ament_cmake should provide a function to emulate this behavior in order to better support overlaying.

@mikaelarguedas or @dirk-thomas feel free to add more details if you have them.

Also, this new function should either be used everywhere or at least where it has been a problem before, and in either case the pr linked above should be fixed with this function as well.

[mikaelarguedas]

this will be targeted post bouncy, removing the milestone.

[dirk-thomas]

A CMake function with that functionality already exists: ament_include_directories_order

And it is already being used within ament_target_dependencies:

ament_cmake/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake

Line 51 in 06f842e

ament_include_directories_order(ordered_include_dirs ${include_dirs})

Since the referenced PR collects the include directories manually the package needs to call ament_include_directories_order manually.

@wjwwood Can this ticket be closed?

[dirk-thomas]

Not relevant for the reported case of this ticket but after investigating another report I created #157 which fixes ament_include_directories_order on non-Windows platforms.

[wjwwood]

I think it could be closed if the workaround in ros2/rviz#243 were replaced with use of this function.

I think we ought to be using it everywhere, but that's a systemic issue.

[dirk-thomas]

I think it could be closed if the workaround in ros2/rviz#243 were replaced with use of this function.

Do you want to keep this ticket open until that has been addressed in the rviz repo? (If that isn't the case in the short term I would rather suggest creating a ticket in that repo instead.)

I think we ought to be using it everywhere, but that's a systemic issue.

Afaik we already do use ament_target_dependencies in numerous places. So I don't think it is a systemic issue. If there are any cases where we don't please point them out so we can change them.

[wjwwood]

Well, my original ask for this issue was to keep it open until the necessary feature was added and deployed in rviz and elsewhere:

Also, this new function should either be used everywhere or at least where it has been a problem before, and in either case the pr linked above should be fixed with this function as well.

-- #130 (comment)

---

I was unaware of the function and also unaware of any place where we were using it. I didn't realize we were using it in ament_target_dependencies(), but does this work if you call it multiple times per target?

Also, searching for target_include_directories() results in 204 matches across 83 files. So discounting a few instances inside of pure cmake packages and within ament_cmake, all of those cases could introduce a problem (as they are added outside of ament_target_dependencies). include_directories is less common, but is used in our googletest integration and in some random packages like laser_geometry and qt_gui_core, so those might an issue too (as I understand it).

Is there an example of how (or do you have an idea of how) to use this correctly when you need to add the include directory manually and with ament_target_dependencies? Do we just make sure they come after (or maybe before) ament_target_dependencies to avoid the issue?

At the very least we should address it in rviz. Since you suggested the fix in rviz originally (and because it's still a bit unclear to me how to do it along side ament_target_dependencies), can you have a look at the follow up to use this method to avoid it?

[dirk-thomas]

but does this work if you call it multiple times per target?

Each call is separate from each other. So only the dependencies from a single call are ordered relatively to each other. Separate calls are adding the information in the order of the calls.

Is there an example of how (or do you have an idea of how) to use this correctly when you need to add the include directory manually and with ament_target_dependencies?

I am not sure what kind of example you are looking for. If you need to call both functions they will add the information to the target in the order you call them. Just as if you would call target_include_directories twice in vanilla CMake.

can you have a look at the follow up to use this method to avoid it?

I am sorry, but I simply don't have the bandwidth to start looking into making rviz pull requests. The usage of the function should be really trivial: just call ament_target_dependencies for all dependencies which adhere to the CMake variable "standard" and call vanilla CMake function for everything else afterwards (assuming those will be system dependencies).

[dirk-thomas]

my original ask for this issue was to keep it open until the necessary feature was added and deployed in rviz and elsewhere:

I created ros2/ros2#658 to track the enhancement to use ament_target_dependencies wherever possible / beneficial / necessary (just for a better scope of the items rather than having it hidden in this repo).