Silent compiler warnings via -isystem includes #102
Conversation
There was a problem hiding this comment.
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.
What happens if /usr/include is included as an -isystem include?
| @@ -92,6 +99,7 @@ function(build_sip_binding PROJECT_NAME SIP_FILE) | |||
| add_custom_command( | |||
| OUTPUT ${SIP_BUILD_DIR}/Makefile | |||
| COMMAND ${PYTHON_EXECUTABLE} ${sip_SIP_CONFIGURE} ${SIP_BUILD_DIR} ${SIP_FILE} ${sip_LIBRARY_DIR} \"${INCLUDE_DIRS}\" \"${LIBRARIES}\" \"${LIBRARY_DIRS}\" \"${LDFLAGS_OTHER}\" \"${EXTRA_DEFINES}\" | |||
| COMMAND sed -i 's/ -I/ -isystem/g' ${SIP_BUILD_DIR}/Makefile | |||
There was a problem hiding this comment.
I think there are windows users, and it seems like requiring sed would be a problem there. Can we do this in sip_configure.py after generating the makefile?
There was a problem hiding this comment.
Sure that will be possible. But, I will not have cycles to do that in any near future. I am using the present patch since several months already and just barely managed to file the PR now.
| foreach(dir ${${PROJECT_NAME}_INCLUDE_DIRS} ${PYTHON_INCLUDE_DIRS}) | ||
| get_filename_component(dir_real "${dir}" REALPATH BASE_DIR ${sip_SOURCE_DIR}) | ||
| # filter out /usr/include, which must not be included via -isystem | ||
| if (NOT "${dir_real}" STREQUAL "/usr/include") |
There was a problem hiding this comment.
I don't understand why filtering /usr/include is necessary, but I'm willing to try it in Noetic and see if it causes issues.
Sorry for not providing this info in first place. See e.g. here on stackoverflow. |
|
I'll just mention that we've had issues in the past with 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: ros2/pybind11_vendor#9 And it leads to somewhat fragile proposed workarounds like this: ros2/rosbag2#623 Basically 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. |
|
@wjwwood, is the problem explained by this note in the gcc docs?
This suggests that if you specify an include path both as |
|
It could be that using |
IIUC in ROS 2 everything is 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
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 |
|
Note that the |
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
-isysteminstead of-I. However, this causes issues if/usr/includeis 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.