pybind11_add_module adds OS-installed ROS include path before ament include paths

[pjreed]

This isn't exactly a problem with this repository, but I think it might be the best place to fix it, so I'm creating an issue here...

First, the problem: rosbag2_py fails to build if there are binary-installed versions of other rosbag2 packages it depends on. See ros2/rosbag2#583 for more details.

This is because it uses pybind11_add_module, provided by the pybind11 package in pybind11Tools.cmake, to declare its targets. That macro declares the target and then calls target_include_directories; it adds system-installed include paths such as /opt/ros/rolling/include to the include directories before it is possible to call ament_target_dependencies, which adds include paths from the source workspace. Because the system-installed directories are added first, gcc prefers to use them to locate header files, which causes the compilation to fail if there are any API incompatibilities. If you declare targets with add_executable or add_library, the right way to fix this is to call ament_target_dependencies before you call target_include_directories or target_link_libraries, but that is not possible with pybind11 because pybind11_add_module does this (at line 154):

  add_library(${target_name} ${lib_type} ${exclude_from_all} ${ARG_UNPARSED_ARGUMENTS})

  if(ARG_SYSTEM)
    set(inc_isystem SYSTEM)
  endif()

  target_include_directories(${target_name} ${inc_isystem}
    PRIVATE ${PYBIND11_INCLUDE_DIR}  # from project CMakeLists.txt
    PRIVATE ${pybind11_INCLUDE_DIR}  # from pybind11Config
    PRIVATE ${PYTHON_INCLUDE_DIRS})

The best solution I've thought of to fix this is to add something like a ros_pybind11_add_module macro that has a AMENT_DEPENDENCIES option; it would duplicate the behavior of pybind11_add_module but the above block of code would call ament_target_dependencies with the contents of that option between add_library and target_include_directories. The down side to this approach is it would also need to be updated any time pybind11 is updated to ensure that ros_pybind11_add_module's functionality is parallel with pybind11_add_module.

Since pybind11 isn't tied to ROS, it shouldn't need to know anything about ament, but I don't think rosbag2_py is the right place to put any custom cmake code that would be useful for any ROS package that uses pybind11; this package seems like the right place to me. Does that sound reasonable to you, or do you have any other suggestions?

[sloretz]

Thanks for the report! It looks like something else is going on here. I see a different compile error that doesn't involve pybind11_add_module(). I didn't put rcutils from source in the workspace. Should I have?

[ 84%] Building CXX object CMakeFiles/test_sqlite_storage.dir/test/rosbag2_storage_default_plugins/sqlite/test_sqlite_storage.cpp.o
[ 84%] Building CXX object CMakeFiles/test_sqlite_wrapper.dir/test/rosbag2_storage_default_plugins/sqlite/test_sqlite_wrapper.cpp.o
/usr/bin/c++  -DDEFAULT_RMW_IMPLEMENTATION=rmw_cyclonedds_cpp -DPLUGINLIB__DISABLE_BOOST_FUNCTIONS -DRCUTILS_ENABLE_FAULT_INJECTION -I/opt/ros/rolling/src/gmock_vendor/include -I/opt/ros/rolling/src/gtest_vendor/include -I/home/sloretz/bigssd/rosbag/src/rosbag2/rosbag2_storage_default_plugins/include -isystem /home/sloretz/bigssd/rosbag/install/rosbag2_test_common/include -isystem /opt/ros/rolling/include -isystem /home/sloretz/bigssd/rosbag/install/rosbag2_storage/include -isystem /opt/ros/rolling/opt/yaml_cpp_vendor/include  -Wall -Wextra -Wpedantic -Werror -std=gnu++14 -o CMakeFiles/test_sqlite_storage.dir/test/rosbag2_storage_default_plugins/sqlite/test_sqlite_storage.cpp.o -c /home/sloretz/bigssd/rosbag/src/rosbag2/rosbag2_storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/test_sqlite_storage.cpp
/usr/bin/c++  -DDEFAULT_RMW_IMPLEMENTATION=rmw_cyclonedds_cpp -DPLUGINLIB__DISABLE_BOOST_FUNCTIONS -DRCUTILS_ENABLE_FAULT_INJECTION -I/opt/ros/rolling/src/gmock_vendor/include -I/opt/ros/rolling/src/gtest_vendor/include -I/home/sloretz/bigssd/rosbag/src/rosbag2/rosbag2_storage_default_plugins/include -isystem /home/sloretz/bigssd/rosbag/install/rosbag2_test_common/include -isystem /opt/ros/rolling/include -isystem /home/sloretz/bigssd/rosbag/install/rosbag2_storage/include -isystem /opt/ros/rolling/opt/yaml_cpp_vendor/include  -Wall -Wextra -Wpedantic -Werror -std=gnu++14 -o CMakeFiles/test_sqlite_wrapper.dir/test/rosbag2_storage_default_plugins/sqlite/test_sqlite_wrapper.cpp.o -c /home/sloretz/bigssd/rosbag/src/rosbag2/rosbag2_storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/test_sqlite_wrapper.cpp
In file included from /home/sloretz/bigssd/rosbag/src/rosbag2/rosbag2_storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/test_sqlite_storage.cpp:29:
/home/sloretz/bigssd/rosbag/src/rosbag2/rosbag2_storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/storage_test_fixture.hpp: In member function ‘rosbag2_storage::StorageOptions StorageTestFixture::make_storage_options_with_config(const string&, const string&)’:
/home/sloretz/bigssd/rosbag/src/rosbag2/rosbag2_storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/storage_test_fixture.hpp:154:22: error: too many initializers for ‘rosbag2_storage::StorageOptions’
  154 |       "", yaml_config};
      |                      ^
In file included from /home/sloretz/bigssd/rosbag/src/rosbag2/rosbag2_storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/test_sqlite_wrapper.cpp:27:
/home/sloretz/bigssd/rosbag/src/rosbag2/rosbag2_storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/storage_test_fixture.hpp: In member function ‘rosbag2_storage::StorageOptions StorageTestFixture::make_storage_options_with_config(const string&, const string&)’:
/home/sloretz/bigssd/rosbag/src/rosbag2/rosbag2_storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/storage_test_fixture.hpp:154:22: error: too many initializers for ‘rosbag2_storage::StorageOptions’
  154 |       "", yaml_config};
      |                      ^
/home/sloretz/bigssd/rosbag/src/rosbag2/rosbag2_storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/test_sqlite_storage.cpp: In member function ‘virtual void StorageTestFixture_storage_configuration_file_applies_over_storage_preset_profile_Test::TestBody()’:
/home/sloretz/bigssd/rosbag/src/rosbag2/rosbag2_storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/test_sqlite_storage.cpp:334:11: error: ‘struct rosbag2_storage::StorageOptions’ has no member named ‘storage_preset_profile’
  334 |   options.storage_preset_profile = "resilient";
      |           ^~~~~~~~~~~~~~~~~~~~~~
/home/sloretz/bigssd/rosbag/src/rosbag2/rosbag2_storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/test_sqlite_storage.cpp: In member function ‘virtual void StorageTestFixture_storage_preset_profile_applies_over_defaults_Test::TestBody()’:
/home/sloretz/bigssd/rosbag/src/rosbag2/rosbag2_storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/test_sqlite_storage.cpp:353:82: error: too many initializers for ‘rosbag2_storage::StorageOptions’
  353 |   rosbag2_storage::StorageOptions options{storage_uri, kPluginID, 0, 0, 0, "", ""};
      |                                                                                  ^
/home/sloretz/bigssd/rosbag/src/rosbag2/rosbag2_storage_default_plugins/test/rosbag2_storage_default_plugins/sqlite/test_sqlite_storage.cpp:355:11: error: ‘struct rosbag2_storage::StorageOptions’ has no member named ‘storage_preset_profile’
  355 |   options.storage_preset_profile = "resilient";

This similarly looks like it's finding the installed header for rosbag2_storage::StorageOptions before the one in the workspace. What looks suspicous to me is -isystem instead of -I includes: -isystem /opt/ros/rolling/include -isystem /home/sloretz/bigssd/rosbag/install/rosbag2_storage/include -isystem /opt/ros/rolling/opt/yaml_cpp_vendor/include. I'm pretty sure that changes the include order, but I'm struggling to find clear documentation about it. Do you know if anything in rosbag2 is using the SYSTEM keyword argument to target_include_directories()?

[sloretz]

Looks like -isystem is because includes from imported targets are treated as system includes by default: https://cmake.org/cmake/help/v3.0/prop_tgt/NO_SYSTEM_FROM_IMPORTED.html

[sloretz]

One way to build rosbag2_storage_default_plugins is to set -DCMAKE_NO_SYSTEM_FROM_IMPORTED=1 and reorder the includes. Put the build include directories first, then add rosbag2_storage before pluginlib.

diff --git a/rosbag2_storage_default_plugins/CMakeLists.txt b/rosbag2_storage_default_plugins/CMakeLists.txt
index f519aba..264b251 100644
--- a/rosbag2_storage_default_plugins/CMakeLists.txt
+++ b/rosbag2_storage_default_plugins/CMakeLists.txt
@@ -34,20 +34,19 @@ add_library(${PROJECT_NAME} SHARED
   src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp
   src/rosbag2_storage_default_plugins/sqlite/sqlite_statement_wrapper.cpp)
 
+target_include_directories(${PROJECT_NAME}
+  PUBLIC
+  $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
+  $<INSTALL_INTERFACE:include>
+)
 ament_target_dependencies(${PROJECT_NAME}
-  pluginlib
   rosbag2_storage
+  pluginlib
   rcpputils
   rcutils
   SQLite3
   yaml_cpp_vendor)
 
-target_include_directories(${PROJECT_NAME}
-  PUBLIC
-  $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
-  $<INSTALL_INTERFACE:include>
-)
-
 # Causes the visibility macros to use dllexport rather than dllimport,
 # which is appropriate when building the dll but not consuming it.
 target_compile_definitions(${PROJECT_NAME} PRIVATE

[pjreed]

Thanks for the report! It looks like something else is going on here. I see a different compile error that doesn't involve pybind11_add_module(). I didn't put rcutils from source in the workspace. Should I have?

These errors are expected if you're building the master branch of rosbag2 and you also have binary rosbag2 packages installed; right now there are several packages that have dependency ordering issues similar to the one I reported here with rosbag2_py. I've got a PR open at ros2/rosbag2#585 that should fix the issue for everything except rosbag2_py, which is what lead me to opening up this issue.

I'll experiment and see if -DCMAKE_NO_SYSTEM_FROM_IMPORTED=1 helps with rosbag2_py...

[sloretz]

I'll experiment and see if -DCMAKE_NO_SYSTEM_FROM_IMPORTED=1 helps with rosbag2_py...

The -isystem bit might not be the real issue. Looks like with the diff I posted everything builds fine after a clean build (edit: without changing CMAKE_NO_SYSTEM_FROM_IMPORTED), including rosbag2_py. I'm not sure what -isystem is doing to the search order, but it didn't need to be changed for the issue to go away.

[pjreed]

It looks like since I opened the original issue, there's been a new Rolling release that has, for better or worse, made the issue obsolete; I just tried again in a brand new ros:rolling container and rosbag2_py builds fine in my branch. So the issue probably still exists but is going to be hard to reproduce until there's another API-breaking change...

[sloretz]

About the rosbag issue: opened a PR in ros2/rosbag2#623

---

About this issue:

If I understand correctly, pybind11_add_module() adds -I/opt/ros/rolling/include because that's where pybind11_INCLUDE_DIR or PYBIND11_INCLUDE_DIR point to.

[...] something like a ros_pybind11_add_module macro that has a AMENT_DEPENDENCIES option; it would duplicate the behavior of pybind11_add_module but the above block of code would call ament_target_dependencies with the contents of that option between add_library and target_include_directories

Creating a new ros_pybind11_add_module that called ament_target_dependencies() first before target_include_directories() would not have changed the order the includes were searched. ament_target_dependencies() is depending on CMake targets, and CMake is using -isystem for includes from imported targets, and -I always is searched before -isystem.

The lookup order is as follows:
[...]
3. Directories specified with -I options are scanned in left-to-right order.
4. Directories specified with -isystem options are scanned in left-to-right order.

In this case there is a call to ament_target_dependencies() with a package not in the overlay. This causes /opt/ros/rolling/include to get added as an -isystem. This causes the -I/opt/ros/rolling/include include to be removed, probably done automatically by CMake because it would have been ignored anyways.

If a standard system include directory, or a directory specified with -isystem, is also specified with -I, the -I option is ignored.

Which leaves me wondering, what happened with rosbag2_py? Maybe if everything in the call to ament_target_dependencies() was in the overlay, then nothing would have added -isystem /opt/ros/rolling/include and the -I/opt/ros/rolling/include would have still been present? Is It possible that's the case you were looking at @pjreed? I have no ideas about how to fix that.