ordered interface include dirs and use privately to ensure workspace order

[dirk-thomas]

Fixes ros2/geometry2#268.

[jacobperron]

I'll let @clalancette confirm that this fixes the connected issue.

[clalancette]

So, this moves the problem around, but doesn't solve it (as far as I can tell).

Going back to the example in ros2/geometry2#268, I built my ros2 workspace in merged install mode with this patch. I then tried to build the clalancette/overlay-include-problem branch in an overlay. The buffer.cpp include problem is fixed, but there is now a problem further along:

[ 32%] Building CXX object CMakeFiles/tf2_ros_test_listener.dir/test/listener_unittest.cpp.o
/usr/bin/c++  -DDEFAULT_RMW_IMPLEMENTATION=rmw_fastrtps_cpp -DSPDLOG_COMPILED_LIB -I/home/ubuntu/custom_tf/src/geometry2/tf2_ros/include -I/home/ubuntu/ros2_ws/install/src/gtest_vendor/include -isystem /home/ubuntu/ros2_ws/install/include -isystem /home/ubuntu/custom_tf/install/tf2/include -isystem /home/ubuntu/custom_tf/install/tf2_msgs/include  -Wall -Wextra -Wpedantic -std=gnu++14 -o CMakeFiles/tf2_ros_test_listener.dir/test/listener_unittest.cpp.o -c /home/ubuntu/custom_tf/src/geometry2/tf2_ros/test/listener_unittest.cpp

It's another manifestation of the same problem; -isystem /home/ubuntu/ros2_ws/install/include is before -isystem /home/ubuntu/custom_tf/install/tf2/include, so it picks up the underlay headers, not the overlay ones.

There may be a bug in the tf2_ros CMakeLists.txt; I'm still looking at that. I just thought I'd give early feedback.

[clalancette]

It turns out that there were a bunch of bugs in the tf2_ros CMakeLists.txt, mostly having to do with the dependencies being incorrect. ros2/geometry2@7116b34 fixes a lot of them.

Unfortunately, that's not enough. Even with those fixes, I still fail to build:

[ 37%] Building CXX object CMakeFiles/tf2_ros_test_time_reset.dir/test/time_reset_test.cpp.o
/usr/bin/c++  -DDEFAULT_RMW_IMPLEMENTATION=rmw_fastrtps_cpp -DSPDLOG_COMPILED_LIB -I/home/ubuntu/custom_tf/src/geometry2/tf2_ros/include -I/home/ubuntu/ros2_ws/install/src/gtest_vendor/include -isystem /home/ubuntu/ros2_ws/install/include -isystem /home/ubuntu/custom_tf/install/tf2/include -isystem /home/ubuntu/custom_tf/install/tf2_msgs/include  -Wall -Wextra -Wpedantic -std=gnu++14 -o CMakeFiles/tf2_ros_test_time_reset.dir/test/time_reset_test.cpp.o -c /home/ubuntu/custom_tf/src/geometry2/tf2_ros/test/time_reset_test.cpp

time_reset_test.cpp does not have a direct dependency on tf2, so that isn't listed in the ament_target_dependencies for it (correctly, I think). But it should inherit the include directory of tf2 in the overlay through tf2_ros, which it doesn't. I'm guessing this is because we are using target_link_libraries for tf2_ros, but as I understand it we have to do that since it is in the same CMakeLists.txt as that library (and thus it doesn't have access to the ament stuff for it yet). It seems like a chicken-and-egg problem to me.

@dirk-thomas any thoughts?

[dirk-thomas]

@clalancette I don't see how calling target_link_libraries(${PROJECT_NAME}_test_time_reset ${PROJECT_NAME}) would be sufficient to also get the include directories of the library target ${PROJECT_NAME} (see https://github.com/ros2/geometry2/blob/6143c536127af51132ae5a2db122f1d37fa55eaa/tf2_ros/CMakeLists.txt#L194-L200). Afaik CMake will only link against the shared library but not consider any include directories - that would even be true if this patch would set the target include dirs as PUBLIC instead of PRIVATE.

So to me it is not obvious how this would have ever worked in the case where this repo is in an overlay and relies on changes on sibling packages from this repo.

[dirk-thomas]

Set of CI builds:

• Linux
• Linux-aarch64
• macOS
• Windows

[dirk-thomas]

@clalancette Something like ros2/geometry2@626e2e1 would be necessary to specify the missing dependencies in geometry2.

[clalancette]

Afaik CMake will only link against the shared library but not consider any include directories - that would even be true if this patch would set the target include dirs as PUBLIC instead of PRIVATE.

My understanding is that with modern CMake, it would consider include directories when you use target_link_libraries with modern targets. But I'm (still) not a CMake expert, so I don't know.

So to me it is not obvious how this would have ever worked in the case where this repo is in an overlay and relies on changes on sibling packages from this repo.

This doesn't work in Eloquent either, so this has probably been broken all along. It was just reported to us recently.

@clalancette Something like ros2/geometry2@626e2e1 would be necessary to specify the missing dependencies in geometry2.

Yep, that's correct. In point of fact, with just the changes on https://github.com/ros2/geometry2/tree/clalancette/overlay-include-problem (and not using this PR), I can get the overlay to work properly. So maybe the fixes are just in there and we don't need this one? In any case, I'm going to open a PR to geometry2 to fix tf2_ros.

[clalancette]

See ros2/geometry2#269

[sloretz]

Afaik CMake will only link against the shared library but not consider any include directories - that would even be true if this patch would set the target include dirs as PUBLIC instead of PRIVATE.

My understanding is that with modern CMake, it would consider include directories when you use target_link_libraries with modern targets. But I'm (still) not a CMake expert, so I don't know.

I had the same understanding as @clalancette. IIUC when target_link_libraries() is called it should read INTERFACE_INCLUDE_DIRECTORIES from the target being depended on and add those to INCLUDE_DIRECTORIES on the target that has the dependency.

Is this patch ordering transitive include directories? Something seems off if ament_target_dependencies() needs to do anything with transitive dependencies when using modern CMake. All it should need to do is call target_link_libraries on direct dependencies.

[dirk-thomas]

I just gave the use case from ros2/geometry2#268 another try. I still fails with Foxy and still passes with this patch for me.

[dirk-thomas]

@clalancette Please feel free to update the previous geometry PR to demonstrate that the use case works with only that without requiring this change. Then we can close this PR without merging.

[clalancette]

I did a CI packaging build with just ros2/geometry2#269 in place, and tested it out. It does not work in the problematic case. So I think we still need to do something else, whether it be this PR or something else.

[dirk-thomas]

@clalancette I didn't mention / use ros2/geometry2#269. I literally followed the steps described in ros2/geometry2#268 and without this PR it fails and with this PR is works for me. Please try to reproduce the exact same use case.

[clalancette]

@clalancette I didn't mention / use ros2/geometry2#269. I literally followed the steps described in ros2/geometry2#268 and without this PR it fails and with this PR is works for me. Please try to reproduce the exact same use case.

I was responding to the previous comment:

@clalancette Please feel free to update the previous geometry PR to demonstrate that the use case works with only that without requiring this change. Then we can close this PR without merging.

So I verified that the "previous geometry PR" is not enough. I can run another packaging job with this PR in place.

[dirk-thomas]

I was responding to the previous comment:

Got it now. Thanks for clarifying.

[clalancette]

So I finally got back to looking at this.

I ran a packaging job with the latest ros2 code plus this PR on bionic: https://ci.ros2.org/job/ci_packaging_linux/384/

I then downloaded and sourced the resulting package. I then created a workspace with https://github.com/ros2/geometry2/tree/clalancette/overlay-include-problem2 . This differs from the previous attempts in that it just has the new methods, and no other changes in geometry2 (it is also rebased on the latest).

When trying to build in that system, I get past the previous error in building buffer_client.cpp, but now I fail further along. Here's the output when building with MAKEFLAGS="VERBOSE=1 -j1" colcon build --executor sequential

[ 32%] Building CXX object CMakeFiles/tf2_ros_test_listener.dir/test/listener_unittest.cpp.o
/usr/bin/c++  -DDEFAULT_RMW_IMPLEMENTATION=rmw_fastrtps_cpp -DSPDLOG_COMPILED_LIB -I/home/ubuntu/ros2-linux/src/gtest_vendor/include -isystem /home/ubuntu/ros2-linux/include -I/home/ubuntu/custom_tf/src/geometry2/tf2_ros/include -isystem /home/ubuntu/custom_tf/install/tf2/include -isystem /home/ubuntu/custom_tf/install/tf2_msgs/include  -Wall -Wextra -Wpedantic -std=gnu++14 -o CMakeFiles/tf2_ros_test_listener.dir/test/listener_unittest.cpp.o -c /home/ubuntu/custom_tf/src/geometry2/tf2_ros/test/listener_unittest.cpp
In file included from /home/ubuntu/custom_tf/src/geometry2/tf2_ros/include/tf2_ros/transform_listener.h:41:0,
                 from /home/ubuntu/custom_tf/src/geometry2/tf2_ros/test/listener_unittest.cpp:37:
/home/ubuntu/custom_tf/src/geometry2/tf2_ros/include/tf2_ros/buffer.h: In member function ‘void tf2_ros::Buffer::foo()’:
/home/ubuntu/custom_tf/src/geometry2/tf2_ros/include/tf2_ros/buffer.h:68:12: error: ‘foo’ is not a member of ‘tf2’
       tf2::foo();
            ^~~

The include order is still wrong, with the system one coming before the overlay ones.

[clalancette]

However, if I combine it with ros2/geometry2#269, it then does work properly. So now I think we need both.