Include order messed up

[MatthijsBurgh]

I had a custom version of a pkg, including dynamic_reconfigure configs, in my workspace. Though I also had the released apt version installed on my machine. The customized configs had additional attributes compared to the system version. During compilation it would raise errors of missing attributes.

In the CMakeLists.txt, the catkin_INCLUDE_DIRS were added as normal includes. /opt/ros/ROS_DISTRO/include is part of the catkin_INCLUDE_DIRS. The include dir of the generated config headers, ws/devel/.private/PKG_NAME/include is added as a SYSTEM BEFORE include, which I think is correct behaviour. Though this results in /opt/ros/ROS_DISTRO/include, which has the headers of system version of the pkg, getting priority over the include dir of the generated config headers, as all system includes come after all other includes.

So the only solution for this and to fix the correct overlaying of workspaces, is to always include catkin_INCLUDE_DIRS as a system include. This way the SYSTEM BEFORE include of the generated config headers will take priority over catkin_INCLUDE_DIRS. I think this should be applied in all ROS/CATKIN pkgs, but especially in pkgs using dynamic_reconfigure. So this doesn't require any change to the code, but I think this should be documented somewhere correctly.

So please add this to the documentation in a striking place.

[MatthijsBurgh]

Fiendly but serious ping ;)

[MatthijsBurgh]

I think this is related to #140 #182.

I think the implementation before #182 was correct, but required some additional documentation.

[MatthijsBurgh]

@mjcarroll I do think that #182 is no the correct way to go. It is indeed the easier solution for the user, but still not correct IMO.

So either revert #182 and add documentation on this issue or release the current version. As by just merging the PR the problem is not solved, as most people use the released version.

[mjcarroll]

@MatthijsBurgh Sorry for the delay, I'll take a look at this today.

[MatthijsBurgh]

@mjcarroll any update?

[MatthijsBurgh]

@mjcarroll ping ;)

[MatthijsBurgh]

@mjcarroll friendly ping ;)

[knorth55]

@MatthijsBurgh I had build issue introduced in #140 , and solved by #182 as #152.

140 uses /opt/ros/ include directories even for workspace build, which causes build error.

Just reverting #182 causes the build error again, I think.

FYI: https://discourse.ros.org/t/preparing-for-melodic-sync-2020-03-12/13207/7

151

152

[seebq]

Confirming #182 fixes this issue for us. CC @kakaday22

[gbiggs]

Thanks for the report.