Gtest headers as system headers

[pkohout]

Hi,
during building our ros2 code we noticed that the GTEST headers are not included as system headers but as normal include directory. This caues g++ to generate compiler warning caused by code in the include files from GTEST. Adding the SYSTEM argument to targent_include_directory see docs cmake does this.

Here is our concrete example.
With the non system include we get the following compile command, which causes the compile error below (interesting part is the -I/opt/ros/foxy/src/gtest_vendor/include):

/usr/lib/ccache/c++ -DDEFAULT_RMW_IMPLEMENTATION=rmw_fastrtps_cpp -DRCUTILS_ENABLE_FAULT_INJECTION -DSPDLOG_COMPILED_LIB -I/opt/ros/foxy/src/gtest_vendor/include -I/home/peter/workspace/alpen/install/include -I/home/peter/workspace/alpen/ros2_ws/src/kin_lifecycle_manager/include -I/home/peter/workspace/alpen/ros2_ws/src/kin_lifecycle_manager/tests/. -I/home/peter/workspace/alpen/ros2_ws/src/kin_lifecycle_manager/tests/../include -I/home/peter/workspace/alpen/ros2_ws/src/kin_lifecycle_manager/src -isystem /home/peter/workspace/alpen/ros2_ws/install/alpen_msgs/include -isystem /opt/ros/foxy/include -isystem /usr/include/eigen3 -O2 -g -DNDEBUG -g -fPIC -std=c++14 -Wall -Wno-switch -Wsuggest-override -Werror=return-type -Wnon-virtual-dtor -Wextra -Wpedantic -Werror -Wdeprecated -pthread -std=gnu++14 -o CMakeFiles/lifecycle_tests.dir/main.cpp.o -c /home/peter/workspace/alpen/ros2_ws/src/kin_lifecycle_manager/tests/main.cpp
In file included from /opt/ros/foxy/src/gtest_vendor/include/gtest/internal/gtest-death-test-internal.h:39,
                 from /opt/ros/foxy/src/gtest_vendor/include/gtest/gtest-death-test.h:41,
                 from /opt/ros/foxy/src/gtest_vendor/include/gtest/gtest.h:62,
                 from /home/peter/workspace/alpen/ros2_ws/src/kin_lifecycle_manager/tests/main.cpp:5:
/opt/ros/foxy/src/gtest_vendor/include/gtest/gtest-matchers.h: In instantiation of ‘class testing::PolymorphicMatcher<testing::internal::MatchesRegexMatcher>::MonomorphicImpl<const std::__cxx11::basic_string<char>&>’:
/opt/ros/foxy/src/gtest_vendor/include/gtest/gtest-matchers.h:538:23:   required from ‘testing::PolymorphicMatcher<Impl>::operator testing::Matcher<T>() const [with T = const std::__cxx11::basic_string<char>&; Impl = testing::internal::MatchesRegexMatcher]’
/opt/ros/foxy/src/gtest_vendor/include/gtest/internal/gtest-death-test-internal.h:170:39:   required from here
/opt/ros/foxy/src/gtest_vendor/include/gtest/gtest-matchers.h:547:18: error: ‘void testing::PolymorphicMatcher<Impl>::MonomorphicImpl<T>::DescribeTo(std::ostream*) const [with T = const std::__cxx11::basic_string<char>&; Impl = testing::internal::MatchesRegexMatcher; std::ostream = std::basic_ostream<char>]’ can be marked override [-Werror=suggest-override]
  547 |     virtual void DescribeTo(::std::ostream* os) const { impl_.DescribeTo(os); }
      |                  ^~~~~~~~~~
/opt/ros/foxy/src/gtest_vendor/include/gtest/gtest-matchers.h:549:18: error: ‘void testing::PolymorphicMatcher<Impl>::MonomorphicImpl<T>::DescribeNegationTo(std::ostream*) const [with T = const std::__cxx11::basic_string<char>&; Impl = testing::internal::MatchesRegexMatcher; std::ostream = std::basic_ostream<char>]’ can be marked override [-Werror=suggest-override]
  549 |     virtual void DescribeNegationTo(::std::ostream* os) const {
      |                  ^~~~~~~~~~~~~~~~~~
/opt/ros/foxy/src/gtest_vendor/include/gtest/gtest-matchers.h:553:18: error: ‘bool testing::PolymorphicMatcher<Impl>::MonomorphicImpl<T>::MatchAndExplain(T, testing::MatchResultListener*) const [with T = const std::__cxx11::basic_string<char>&; Impl = testing::internal::MatchesRegexMatcher]’ can be marked override [-Werror=suggest-override]
  553 |     virtual bool MatchAndExplain(T x, MatchResultListener* listener) const {
      |                  ^~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

With the system include we get the follwoing compile command (which works). Now the headers are set via -isystem /opt/ros/foxy/src/gtest_vendor/include, preventing the compiler from generating the -Wsuggest-override warnings

/usr/lib/ccache/c++ -DDEFAULT_RMW_IMPLEMENTATION=rmw_fastrtps_cpp -DRCUTILS_ENABLE_FAULT_INJECTION -DSPDLOG_COMPILED_LIB -I/home/peter/workspace/alpen/install/include -I/home/peter/workspace/alpen/ros2_ws/src/kin_lifecycle_manager/include -I/home/peter/workspace/alpen/ros2_ws/src/kin_lifecycle_manager/tests/. -I/home/peter/workspace/alpen/ros2_ws/src/kin_lifecycle_manager/tests/../include -I/home/peter/workspace/alpen/ros2_ws/src/kin_lifecycle_manager/src -isystem /opt/ros/foxy/src/gtest_vendor/include -isystem /home/peter/workspace/alpen/ros2_ws/install/alpen_msgs/include -isystem /opt/ros/foxy/include -isystem /usr/include/eigen3 -O2 -g -DNDEBUG -g -fPIC -std=c++14 -Wall -Wno-switch -Wsuggest-override -Werror=return-type -Wnon-virtual-dtor -Wextra -Wpedantic -Werror -Wdeprecated -pthread -std=gnu++14 -o CMakeFiles/lifecycle_tests.dir/main.cpp.o -c /home/peter/workspace/alpen/ros2_ws/src/kin_lifecycle_manager/tests/main.cpp

If there is a reason why the SYSTEM arg is not used, please let me know, right now I don't see any. Then I will have to find another solution to get it working.

All the best,
Peter

[hidmic]

Seems reasonable to me. System include directories are also ignored from dependency calculations, but I wouldn't expect gtest sources to change. @clalancette thoughts against?

[clalancette]

If I remember properly, the main reason we don't use system includes is because overlays then don't work right. But this is based on a vague memory from a while ago, so it bears testing again.

[koniarik]

Can I ask what this waits for? If there is something I can help it I am willing to do it.

[clalancette]

Can I ask what this waits for? If there is something I can help it I am willing to do it.

Looking at this further, I actually think this is the wrong thing to do (though others might disagree with me). This PR is essentially abusing the "system" connotations of -isystem to disable warnings in gtest headers (see the gcc manpage for more information about system headers).

There are 2 problems with that:

1. If you don't want the see these warnings, then just don't enable the warnings, and/or fix the warnings in the upstream gtest code. We'd also accept PRs to https://github.com/ament/googletest , assuming the relevant fixes are also headed to the upstream.
2. Marking these header files as system has consequences for overlays (if I'm understanding the gcc manpage properly). Essentially, the manpage says you should use -I to override system header files, which is what we are doing here. This also means that someone should be able to introduce their own version of the gtest libraries in an overlay, and have that used. If we switch this to -isystem, I don't think that will work anymore.

So I think this is the wrong way to go, but I'm happy to hear counter-arguments.

[koniarik]

Can I ask what this waits for? If there is something I can help it I am willing to do it.

Looking at this further, I actually think this is the wrong thing to do (though others might disagree with me). This PR is essentially abusing the "system" connotations of -isystem to disable warnings in gtest headers (see the gcc manpage for more information about system headers).

There are 2 problems with that:

1. If you don't want the see these warnings, then just don't enable the warnings, and/or fix the warnings in the upstream gtest code. We'd also accept PRs to https://github.com/ament/googletest , assuming the relevant fixes are also headed to the upstream.

Those are not warnings, those are errors because I use very strict settings for my code that I do not expect external headers to follow strictly. The -Wconversion is prime example of that.
And that by itself is exactly something that I want to follow, but would not force the "changes" to the code upstream as it's sideeffect is that it makes the code noisy (you have to convert everything explicitly) and I can understand why some projects avoid it.

Generally, just the fact that I want to enforce some compiler options in my project does not mean it makes sense for other projects. This implies that I need a way to tell the compiler to ignore my options for third-party libraries which are not valid with these options. (Does not mean the code does not work!)

2. Marking these header files as system has consequences for overlays (if I'm understanding the gcc manpage properly). Essentially, the manpage says you should use -I to override system header files, which is what we are doing here. This also means that someone should be able to introduce their own version of the gtest libraries in an overlay, and have that used. If we switch this to -isystem, I don't think that will work anymore.

Why it should not work? The manpage explicitly states order at which the libraries are searched for. gcc scans -isystem includes before default system libraries. It also specifies order at which this happens (left-to-right) which I believe makes it quite deterministic even for some "weird" or "rare" situations.

The way I understand that sentece in the manpage is: You can use -I for the overrides, but you should not do it for vendor headers and use -isystem instead to do the overriding. This in spirit of what is written on the "system headers" section of documentation, when they motivate existence of -isystem due to: "The header files declaring interfaces to the operating system and runtime libraries often cannot be written in strictly conforming C. "
I believe the point of that warning is simply to warn you that if you override with -I your code may not compile (or introduce a lot of warnings, but I personally use -Werror, so everything is error for me)

So I think this is the wrong way to go, but I'm happy to hear counter-arguments.

Just the -Werror I am using would be impossible to use in my project because it would most propably break compilation with a lot of third-party library I would like to use. Hence I include them as system libraries to avoid these problems.

EDIT: As a sidenote: I managed to spin up my custom "_add_gtest" macro is copy of ament_add_gtest() with just this change applied quite easily given that ament is documented pretty well, in case somebody wants to solve this too.

[hidmic]

Can I ask what this waits for? If there is something I can help it I am willing to do it.

Speaking as the one assigned, I haven't had the time to validate this patch doesn't break gtest overlays. I'd think hope not, but it's best if we double check. I'll try to squeeze a bit of time this week.

[clalancette]

I'm worried about this change because we actually used to use SYSTEM for these headers in the past, and we reverted it. See #175 and #184 for some details (also ros/catkin#428 for a ROS 1 discussion of the same).

[sloretz]

If I remember properly, the main reason we don't use system includes is because overlays then don't work right. But this is based on a vague memory from a while ago, so it bears testing again.

I think this is worth looking at again. Since ROS 2 switched to imported targets instead of CMake variables (ros2/ros2#904), all includes from different ROS packages are passed as -isystem. The reasons against in -isystem when using Catkin and earlier versions of ROS 2 may be reversed now, because using -I changes the search order with respect to other ROS packages using -isystem.

[hidmic]

Circling back. @sloretz is right but based on ros2/ros2#1150 I think include directories and overlays have to be rethought.

[clalancette]

@sloretz With your recent investigations and fixes for overlays, do you think we still need this PR?

[sloretz]

@sloretz With your recent investigations and fixes for overlays, do you think we still need this PR?

Yes. From the perspective of overriding packages, the -I and -isystem distinction are no longer going to be a problem. They were a problem in ROS 1 because catkin mitigates the include directory search order problem by ordering the include directories according to the workspace order. Using -isystem could cause underlay include directories to jump to the front and break the ordering. In ROS 2 the packages will have unique include directories, so it does not matter if they're re-ordered or passed via -I or -isystem.

-isystem makes sense to me from a modern CMake perspective because by default include directories from imported targets are added as -isystem. If rcutils::rcutils include directories are included via -isystem to rcl, then why not gtest's headers as well?

[kunaltyagi]

Is there still any issue preventing the PR from getting merged?

[v-lopez]

Can it be added to gmock too?
https://github.com/ament/ament_cmake/blob/rolling/ament_cmake_gmock/cmake/ament_add_gmock.cmake#L70

[maspe36]

Is there a reason this PR hasn't been merged yet?

[clalancette]

Is there a reason this PR hasn't been merged yet?

Actually, we went a slightly different way here in #408, where we made this SYSTEM PRIVATE instead. So I think this is redundant and unnecessary now, I'm going to close this out.