Foxy: Cannot overlay tf2_ros over existing tf2_ros

[clalancette]

Bug report

If you want to put a custom function into one of the geometry2 packages (let's say tf2 for the sake of this example), the obvious thing to do is to install Foxy (either from debs or the fat binary), then make a workspace with geometry2 in it that has your custom code. The expectation here is that the custom version of geometry2 will overlay the binary installed version. However, this doesn't currently work; if you add a new method to tf2, it will fail to find that method if you try to call it from elsewhere (say in tf2_ros). This is because the include directory order is wrong; see below for more details.

Required Info:

• Operating System:
  • Ubuntu 20.04
• Installation type:
  • Binary (debian packages)
• Version or commit hash:
  • a0cd9d7
• DDS implementation:
  • Fast-RTPS

Steps to reproduce issue

1. Install ros-foxy debians on 20.04: sudo apt-get install ros-foxy-desktop
2. Make a workspace: mkdir -p custom_tf/src
3. Clone a custom branch into the workspace: pushd custom_tf/src ; git clone https://github.com/ros2/geometry2 -b clalancette/overlay-include-problem ; popd
4. Source foxy: source /opt/ros/foxy/setup.bash
5. Try to build the overlay: colcon build

Expected behavior

Overlay compiles successfully.

Actual behavior

Overlay fails to build with:

In file included from /home/ubuntu/custom_tf/src/geometry2/tf2_ros/src/buffer_server.cpp:41:
/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’
   68 |       tf2::foo();
      |            ^~~
In file included from /home/ubuntu/custom_tf/src/geometry2/tf2_ros/src/buffer.cpp:33:
/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’
   68 |       tf2::foo();
      |            ^~~
/home/ubuntu/custom_tf/src/geometry2/tf2_ros/src/buffer.cpp: In constructor ‘tf2_ros::Buffer::Buffer(rclcpp::Clock::SharedPtr, tf2::Duration)’:
/home/ubuntu/custom_tf/src/geometry2/tf2_ros/src/buffer.cpp:55:8: error: ‘foo’ is not a member of ‘tf2’
   55 |   tf2::foo();
      |        ^~~

Additional information

Turning on additional debugging, I see the following compile line for the file in question:

/usr/bin/c++  -DDEFAULT_RMW_IMPLEMENTATION=rmw_fastrtps_cpp -DSPDLOG_COMPILED_LIB -DTF2_ROS_BUILDING_DLL -Dtf2_ros_EXPORTS -I/home/ubuntu/custom_tf/src/geometry2/tf2_ros/include -isystem /opt/ros/foxy/include -isystem /home/ubuntu/custom_tf/install/tf2/include -isystem /home/ubuntu/custom_tf/install/tf2_msgs/include  -fPIC   -Wall -Wextra -Wpedantic -std=gnu++14 -o CMakeFiles/tf2_ros.dir/src/buffer.cpp.o -c /home/ubuntu/custom_tf/src/geometry2/tf2_ros/src/buffer.cpp

It is clearly picking up the tf2 headers from /opt/ros/foxy/include before the custom ones in /home/ubuntu/custom_tf/install/tf2/include

[clalancette]

Interestingly, this does not seem to be a problem in from-source builds.

[clalancette]

Doing some additional testing, this also doesn't work totally right on Eloquent either. The place where it fails is different, but it fails in similar ways (with similar g++ command-lines). So this isn't a regression, though it is a problem people are running into.

[dirk-thomas]

Interestingly, this does not seem to be a problem in from-source builds.

Are you using a merged install or the default isolated? For the latter I would expect the problem to not show up since include directory ordering is not a problem if each include dir is a separate path anyway.

[clalancette]

Are you using a merged install or the default isolated? For the latter I would expect the problem to not show up since include directory ordering is not a problem if each include dir is a separate path anyway.

Default isolated. But what I don't understand is why the situation would be different there. How does ament "know" that it should put the include directory path to the overlay before the underlay? In other words, how does it know it should do:

g++ -Ipath/to/overlay/install/tf2/include -Ipath/to/underlay/install/tf2/include

rather than:

g++ -Ipath/to/underlay/install/tf2/include -Ipath/to/overlay/install/tf2/include

[dirk-thomas]

Default isolated. But what I don't understand is why the situation would be different there.

In a merged install all headers are in the same include dir `ws/install/include. For an isolated install each package has a separate include dir.

How does ament "know" that it should put the include directory path to the overlay before the underlay?

Based on the order of the paths in AMENT_PREFIX_PATH. For merged installs it only contains a single entry - for isolated installs each package contributes an entry.

[clalancette]

Based on the order of the paths in AMENT_PREFIX_PATH. For merged installs it only contains a single entry - for isolated installs each package contributes an entry.

OK. So fundamentally I think the problem is when you mix a merged install and an isolated installed. That is, when installing from debians, you essentially get a merged install as everything is under /opt/ros/foxy/include. If you then try to build on top of that with an isolated environment, as soon as you do ament_target_dependencies for anything in that merged install, then you get your first include directory as -I/opt/ros/foxy/include. After that, you can't use the headers for overlaid packages in the overlay. Do you agree with that assessment of the problem?

[dirk-thomas]

I think the problem is when you mix a merged install and an isolated installed. That is, when installing from debians, you essentially get a merged install

I highly doubt that. Each workspace / prefix path exists independent of each other.

Maybe post the compiler command line when building buffer.cpp.

[clalancette]

Maybe post the compiler command line when building buffer.cpp.

It's in the original comment.

I will point out that if I remove all dependencies except for tf2 from

geometry2/tf2_ros/CMakeLists.txt

Line 39 in a0cd9d7

ament_target_dependencies(${PROJECT_NAME}

, then the compile of buffer.cpp works. This is obviously wrong for other reasons, but I think it illustrates my point; once we find a dependency in the underlay, it puts /opt/ros/foxy/include on the include path, and everything after that point will only use the underlay header if it exists.

[dirk-thomas]

It's in the original comment.

Sorry, I missed that:

-I/home/ubuntu/custom_tf/src/geometry2/tf2_ros/include
-isystem /opt/ros/foxy/include
-isystem /home/ubuntu/custom_tf/install/tf2/include
-isystem /home/ubuntu/custom_tf/install/tf2_msgs/include

Debugging what ament_include_directories_order() is doing when called by ament_target_dependencies()...

The only include directory added with target_include_directories() is /opt/ros/foxy/include (I guess coming from the message_filters package which doesn't use modern CMake yet - so updating that package should fix the problem but that is not what we are looking for...).

All other dependencies provide modern interface targets which are added with target_link_libraries. Reading https://stackoverflow.com/questions/47175683/cmake-target-link-libraries-propagation-of-include-directories it sounds like CMake doesn't preserve the order they are added in: https://github.com/ament/ament_cmake/blob/b817874d59b4adf5ee85ea00d7dd073a918c5190/ament_cmake_target_dependencies/cmake/ament_target_dependencies.cmake#L121-L124

Please see ament/ament_cmake#260 which should address the problem.

[dirk-thomas]

I just gave this use case another try. I still fails with Foxy and still passes with ament/ament_cmake#260 for me.