search

ros-visualization / python_qt_binding

http://wiki.ros.org/python_qt_binding
BSD 3-Clause "New" or "Revised" License
34 stars 54 forks source link

Silent compiler warnings via -isystem includes #102

Open rhaschke opened 3 years ago

rhaschke commented 3 years ago

rviz' CI is frequently failing because of compiler warnings in include files external to the actual source tree. While Debian Buster uses an older gcc version and thus isn't affected, warnings on the ROS buildfarm for Ubuntu Focal turn into failures in github's CI.

The obvious solution is to mark external includes using -isystem instead of -I. However, this causes issues if /usr/include is included like this as well - either directly or indirectly via relative paths or symlinks. Hence, the second change of this PR filters out these paths.

rhaschke commented 3 years ago

What happens if /usr/include is included as an -isystem include?

Sorry for not providing this info in first place. See e.g. here on stackoverflow.

wjwwood commented 3 years ago

I'll just mention that we've had issues in the past with -isystem being used in combination with -I in the past. Basically if it was -IA -IB -IC and foo.hpp existed in all three, you'd take it from -IC, but if you changed it to -IA -isystemB -IC then it might change which version of foo.hpp was selected. I'm not sure that's exactly right (might have the scenario a bit wrong), and it only came up in overlaying scenarios, but we did have problems with it at some point and actually removed the use of -isystem in a lot of places.

This was years and years ago, so maybe it's worth doing now, but I just wanted to bring it up.

After looking, I guess @sloretz you're already aware of this, based on this: https://github.com/ros2/pybind11_vendor/issues/9

And it leads to somewhat fragile proposed workarounds like this: https://github.com/ros2/rosbag2/pull/623

Basically -isystem has been really annoying in the past.

It's more annoying to do so, but I think disabling warnings around include statements within rviz's code base is safer, but that's definitely a personal preference kind of choice.

rhaschke commented 3 years ago

@wjwwood, is the problem explained by this note in the gcc docs?

If a standard system include directory, or a directory specified with -isystem, is also specified with -I, the -I option is ignored. The directory is still searched but as a system directory at its normal position in the system include chain.

This suggests that if you specify an include path both as -I and -isystem, it will be ignored for the high-priority -I search and only considered later when processing all -isystem includes. For me, that reads like all external include paths should be marked consequently with -isystem.

wjwwood commented 3 years ago

It could be that using -isystem for "everything external" could work, but that's not the case right now. Using it sometimes is the issue I think though.

sloretz commented 3 years ago

It could be that using -isystem for "everything external" could work, but that's not the case right now. Using it sometimes is the issue I think though.

IIUC in ROS 2 everything is -isystem because that's the default for imported CMake targets.

Since this PR is ROS 1, IIRC catkin uses CMake variables and does some workspace aware include directory ordering. The order in the gcc docs means that probably breaks when -I or -isystem are mixed.

Sorry for not providing this info in first place. See e.g. here on stackoverflow.

Ah got it. It seems to be this issue: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70129 . CMake itself works around that as of https://gitlab.kitware.com/cmake/cmake/-/merge_requests/2716 . One comments says appending /usr/include to CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES will work around the issue in the meantime.

rhaschke commented 3 years ago

Note that the sip stuff doesn't use cmake. It directly creates a make file and this PR suggests replacing all -I with -isystem. Hence, there is no risk to mix them - everything is -isystem in the end (only for compiling the sip bindings). So, I think it would be safe to apply this PR. Of course, sed should be replaced by python...