Wrong include directories order

[hexonxons]

Description

I've tried to build #687 to check if it works in our environment, but it seems it wont build clean if ros-foxy-rosbag2... packages are installed on the target machine.

I've checked it while building on host machine and reproduced in ros:foxy docker container. In both cases colcon build finishes with an error:

ERROR

--- stderr: rosbag2_transport_future                                      
In file included from /ws/src/rosbag2_transport/test/rosbag2_transport/rosbag2_transport_test_fixture.hpp:42,
                 from /ws/src/rosbag2_transport/test/rosbag2_transport/record_integration_fixture.hpp:31,
                 from /ws/src/rosbag2_transport/test/rosbag2_transport/test_record_regex.cpp:24:
/ws/src/rosbag2_transport/test/rosbag2_transport/mock_sequential_reader.hpp:28:8: error: ‘void MockSequentialReader::open(const rosbag2_storage::StorageOptions&, const rosbag2_cpp::ConverterOptions&)’ marked ‘override’, but does not override
   28 |   void open(
      |        ^~~~
In file included from /ws/src/rosbag2_transport/test/rosbag2_transport/rosbag2_transport_test_fixture.hpp:43,
                 from /ws/src/rosbag2_transport/test/rosbag2_transport/record_integration_fixture.hpp:31,
                 from /ws/src/rosbag2_transport/test/rosbag2_transport/test_record_regex.cpp:24:
/ws/src/rosbag2_transport/test/rosbag2_transport/mock_sequential_writer.hpp:28:8: error: ‘void MockSequentialWriter::open(const rosbag2_storage::StorageOptions&, const rosbag2_cpp::ConverterOptions&)’ marked ‘override’, but does not override
   28 |   void open(
      |        ^~~~
In file included from /usr/include/c++/9/memory:80,
                 from /opt/ros/foxy/src/gmock_vendor/include/gmock/gmock-actions.h:45,
                 from /opt/ros/foxy/src/gmock_vendor/include/gmock/gmock.h:59,
                 from /ws/src/rosbag2_transport/test/rosbag2_transport/test_record_regex.cpp:15:
/usr/include/c++/9/bits/unique_ptr.h: In instantiation of ‘typename std::_MakeUniq<_Tp>::__single_object std::make_unique(_Args&& ...) [with _Tp = MockSequentialReader; _Args = {}; typename std::_MakeUniq<_Tp>::__single_object = std::unique_ptr<MockSequentialReader, std::default_delete<MockSequentialReader> >]’:
/ws/src/rosbag2_transport/test/rosbag2_transport/rosbag2_transport_test_fixture.hpp:62:90:   required from here
/usr/include/c++/9/bits/unique_ptr.h:857:30: error: invalid new-expression of abstract class type ‘MockSequentialReader’
  857 |     { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); }
      |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /ws/src/rosbag2_transport/test/rosbag2_transport/rosbag2_transport_test_fixture.hpp:42,
                 from /ws/src/rosbag2_transport/test/rosbag2_transport/record_integration_fixture.hpp:31,
                 from /ws/src/rosbag2_transport/test/rosbag2_transport/test_record_regex.cpp:24:
/ws/src/rosbag2_transport/test/rosbag2_transport/mock_sequential_reader.hpp:25:7: note:   because the following virtual functions are pure within ‘MockSequentialReader’:
   25 | class MockSequentialReader : public rosbag2_cpp::reader_interfaces::BaseReaderInterface
      |       ^~~~~~~~~~~~~~~~~~~~
In file included from /ws/src/rosbag2_transport/test/rosbag2_transport/mock_sequential_reader.hpp:23,
                 from /ws/src/rosbag2_transport/test/rosbag2_transport/rosbag2_transport_test_fixture.hpp:42,
                 from /ws/src/rosbag2_transport/test/rosbag2_transport/record_integration_fixture.hpp:31,
                 from /ws/src/rosbag2_transport/test/rosbag2_transport/test_record_regex.cpp:24:
/opt/ros/foxy/include/rosbag2_cpp/reader_interfaces/base_reader_interface.hpp:40:16: note:  ‘virtual void rosbag2_cpp::reader_interfaces::BaseReaderInterface::open(const rosbag2_cpp::StorageOptions&, const rosbag2_cpp::ConverterOptions&)’
   40 |   virtual void open(
      |                ^~~~
In file included from /usr/include/c++/9/memory:80,
                 from /opt/ros/foxy/src/gmock_vendor/include/gmock/gmock-actions.h:45,
                 from /opt/ros/foxy/src/gmock_vendor/include/gmock/gmock.h:59,
                 from /ws/src/rosbag2_transport/test/rosbag2_transport/test_record_regex.cpp:15:
/usr/include/c++/9/bits/unique_ptr.h: In instantiation of ‘typename std::_MakeUniq<_Tp>::__single_object std::make_unique(_Args&& ...) [with _Tp = MockSequentialWriter; _Args = {}; typename std::_MakeUniq<_Tp>::__single_object = std::unique_ptr<MockSequentialWriter, std::default_delete<MockSequentialWriter> >]’:
/ws/src/rosbag2_transport/test/rosbag2_transport/rosbag2_transport_test_fixture.hpp:63:90:   required from here
/usr/include/c++/9/bits/unique_ptr.h:857:30: error: invalid new-expression of abstract class type ‘MockSequentialWriter’
  857 |     { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); }
      |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /ws/src/rosbag2_transport/test/rosbag2_transport/rosbag2_transport_test_fixture.hpp:43,
                 from /ws/src/rosbag2_transport/test/rosbag2_transport/record_integration_fixture.hpp:31,
                 from /ws/src/rosbag2_transport/test/rosbag2_transport/test_record_regex.cpp:24:
/ws/src/rosbag2_transport/test/rosbag2_transport/mock_sequential_writer.hpp:25:7: note:   because the following virtual functions are pure within ‘MockSequentialWriter’:
   25 | class MockSequentialWriter : public rosbag2_cpp::writer_interfaces::BaseWriterInterface
      |       ^~~~~~~~~~~~~~~~~~~~
In file included from /ws/src/rosbag2_transport/test/rosbag2_transport/mock_sequential_writer.hpp:23,
                 from /ws/src/rosbag2_transport/test/rosbag2_transport/rosbag2_transport_test_fixture.hpp:43,
                 from /ws/src/rosbag2_transport/test/rosbag2_transport/record_integration_fixture.hpp:31,
                 from /ws/src/rosbag2_transport/test/rosbag2_transport/test_record_regex.cpp:24:
/opt/ros/foxy/include/rosbag2_cpp/writer_interfaces/base_writer_interface.hpp:37:16: note:  ‘virtual void rosbag2_cpp::writer_interfaces::BaseWriterInterface::open(const rosbag2_cpp::StorageOptions&, const rosbag2_cpp::ConverterOptions&)’
   37 |   virtual void open(
      |                ^~~~
make[2]: *** [CMakeFiles/test_record_regex__rmw_fastrtps_cpp.dir/build.make:63: CMakeFiles/test_record_regex__rmw_fastrtps_cpp.dir/test/rosbag2_transport/test_record_regex.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:512: CMakeFiles/test_record_regex__rmw_fastrtps_cpp.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:141: all] Error 2
---
Failed   <<< rosbag2_transport_future [37.1s, exited with code 2]

For some reason /opt/ros/foxy/include/rosbag2_cpp/writer_interfaces/base_writer_interface.hpp header was selected instead of src/rosbag2_cpp/include/, so I've build it with exported compile commands and found that test_record_regex is compiled like:

{
  "directory": "/ws/build/rosbag2_transport_future",
  "command": "/usr/bin/c++  -DDEFAULT_RMW_IMPLEMENTATION=rmw_fastrtps_cpp -DPLUGINLIB__DISABLE_BOOST_FUNCTIONS -DRCUTILS_ENABLE_FAULT_INJECTION -DROS_PACKAGE_NAME=\\\"rosbag2_transport_future\\\" -DSPDLOG_COMPILED_LIB -I/opt/ros/foxy/src/gmock_vendor/include -I/opt/ros/foxy/src/gtest_vendor/include -I/ws/src/rosbag2_transport/include -isystem /ws/install/rosbag2_test_common_future/include -isystem /opt/ros/foxy/include -isystem /ws/install/rosbag2_compression_future/include -isystem /ws/install/rosbag2_cpp_future/include -isystem /ws/install/rosbag2_storage_future/include -isystem /opt/ros/foxy/opt/yaml_cpp_vendor/include -isystem /ws/install/shared_queues_vendor_future/include/moodycamel -isystem /ws/install/shared_queues_vendor_future/include    -Wall -Wextra -Wpedantic -Werror -std=gnu++14 -o CMakeFiles/test_record_regex__rmw_fastrtps_cpp.dir/test/rosbag2_transport/test_record_regex.cpp.o -c /ws/src/rosbag2_transport/test/rosbag2_transport/test_record_regex.cpp",
  "file": "/ws/src/rosbag2_transport/test/rosbag2_transport/test_record_regex.cpp"
}

Note than -isystem /ws/install/rosbag2_cpp_future/include goes AFTER -isystem /opt/ros/foxy/include
While for test_record everything seems ok:

{
  "directory": "/ws/build/rosbag2_transport_future",
  "command": "/usr/bin/c++  -DDEFAULT_RMW_IMPLEMENTATION=rmw_fastrtps_cpp -DPLUGINLIB__DISABLE_BOOST_FUNCTIONS -DRCUTILS_ENABLE_FAULT_INJECTION -DROS_PACKAGE_NAME=\\\"rosbag2_transport_future\\\" -DSPDLOG_COMPILED_LIB -I/opt/ros/foxy/src/gmock_vendor/include -I/opt/ros/foxy/src/gtest_vendor/include -I/ws/src/rosbag2_transport/src/rosbag2_transport -I/ws/src/rosbag2_transport/include -isystem /ws/install/rosbag2_cpp_future/include -isystem /ws/install/rosbag2_storage_future/include -isystem /ws/install/rosbag2_test_common_future/include -isystem /opt/ros/foxy/include -isystem /opt/ros/foxy/opt/yaml_cpp_vendor/include -isystem /ws/install/rosbag2_compression_future/include -isystem /ws/install/shared_queues_vendor_future/include/moodycamel -isystem /ws/install/shared_queues_vendor_future/include    -Wall -Wextra -Wpedantic -Werror -std=gnu++14 -o CMakeFiles/test_record__rmw_fastrtps_cpp.dir/test/rosbag2_transport/test_record.cpp.o -c /ws/src/rosbag2_transport/test/rosbag2_transport/test_record.cpp",
  "file": "/ws/src/rosbag2_transport/test/rosbag2_transport/test_record.cpp"
}

Well, not truly ok, cuz -isystem /opt/ros/foxy/include still goes before other includes from workspace. But it compiles at least.

It seems that adding rosbag2_cpp_future to AMENT_DEPS for test_record_regex moves -isystem /ws/install/rosbag2_cpp_future/include to position before -isystem /opt/ros/foxy/include but I am not sure it is right fix, cuz I dont know how does ament_target_dependencies work and how does it handle transitive dependencies. It might be a problem in ament cmake scripts

Observed behavior is not only for rosbag2_transport_future package. It seems that all compile commands for all packages should be reviewed, cuz this behavior might lead to very confusing bugs

Expected Behavior

Everything compiles successfully and include directories order is right

Actual Behavior

Error while compiling rosbag2 packages

To Reproduce

1. Run ros container with rosbag2 inside:

docker run --rm -it ros:foxy /bin/bash

2. Install deps:

# Install deps
apt update && apt install ros-foxy-test-msgs pybind11-dev

3. Create workspace and clone rosbag2:

mkdir -p ws/src
git clone https://github.com/ros2/rosbag2.git -b emersonknapp/futurize-foxy ws/src
cd ws

4. Build:

colcon build --ament-cmake-args -DCMAKE_EXPORT_COMPILE_COMMANDS=ON

5. Check for compile commands: cat build/rosbag2_transport_future/compile_commands.json

System (please complete the following information)

• OS: Ubuntu Bionic
• ROS 2 Distro: foxy
• Version: Reproduced in emersonknapp/futurize-foxy, but master branch still affected

[emersonknapp]

It seems that adding rosbag2_cpp_future to AMENT_DEPS for test_record_regex moves -isystem /ws/install/rosbag2_cpp_future/include to position before -isystem /opt/ros/foxy/include but I am not sure it is right fix, cuz I dont know how does ament_target_dependencies work and how does it handle transitive dependencies. It might be a problem in ament cmake scripts

I think that this is the right fix - most of the time this type of problem happens because target aren't declaring their dependencies explicitly, so if there is a system-installed version it gets picked up by accident. PRs welcome

[emersonknapp]

Note: we probably want a regular build that tries to build from source, when the binary package is already installed. We have no way to catch these problems in the current setup

[emersonknapp]

This is a symptom of ros2/ros2#1150, right?

[MichaelOrlov]

• Closing as stale issue.