Pass rcutils_include_dirs to cppcheck

[claireyywang]

This fixes the rclpy's unknown macro error on osx and windows, but it may not be the best strategy, cuz it will require manually pass in include_dirs to cppcheck in all other repos listed on this test report https://ci.ros2.org/view/nightly/job/nightly_win_rel/1576/testReport/

We've encountered the same error before and managed to fix it in this ticket ament/ament_lint#116 I'm not sure why this is happening again.

Original issue ros2/ros2#942

[claireyywang]

windows CI against cppcheck2.0

[dirk-thomas]

This approach doesn't seem like the right solution. Maybe the original decision to not include dirs from other packages needs to be reconsidered (see ament/ament_lint#117 (comment)).

[jacobperron]

This approach doesn't seem like the right solution. Maybe the original decision to not include dirs from other packages needs to be reconsidered (see ament/ament_lint#117 (comment)).

IIRC, the reason I opted to not include headers from all dependencies was that the running time of cppcheck increase by a lot (e.g. the test timeout was easily exceeded). It still might be worth trying again; perhaps there have been performance improvements to cppcheck since then.

edit: also, we have to be careful adding headers from third-party libraries as cppcheck will report any errors in their headers as well.

[jacobperron]

This is the approach we've taken elsewhere (e.g. in rclcpp_action and rclcpp_lifecycle).

It used to be rare that we needed to give these hints, but if it starting to become a common occurrence maybe we should investigate alternatives.

[claireyywang]

It used to be rare that we needed to give these hints, but if it starting to become a common occurrence maybe we should investigate alternatives.

rclpy is one of the packages having this error. The bulk of errors from test report seem to be from a bunch of rosidl_typesupport and rcl packages. It's hard to estimate from the report how many are affected, but if this approach is the right way to go, I will need to start setting ament_cmake_cppcheck_ADDITIONAL_INCLUDE_DIRS for all those packages.

[claireyywang]

I guess another way is to pass all macro include_dirs used by the package to cppcheck in ament_lint, instead of just include_dirs of the package being tested, but I don't know enough cmake or build process to see the down side of this approach.

[dirk-thomas]

Instead of the classic CMake variable this should use the modern CMake target:

get_target_property(ament_cmake_cppcheck_ADDITIONAL_INCLUDE_DIRS rcutils::rcutils INTERFACE_INCLUDE_DIRECTORIES)

[dirk-thomas]

I am mostly concerned about the locality of a change. If a package starts using a macro it might need to update the cppcheck arguments (not great but not terrible either).

But that change shouldn't be exposed to downstream packages and if upstream packages change their implementation that shouldn't effect this package either.

The exception would be if an upstream package is already explicitly listed for additional include directories - at that point any future change in the upstream package might cause a regression in this package. That would be case which might bubble through the stack which would be really unfortunate.

Beside my minor inline comment this patch addresses a specific issue we are having (when unpinning) so I am fine to merge it (with the comment addressed). Maybe a ticket to follow up this to see if there is any way to automate this while not killing performance would be good to create.

[claireyywang]

CI again
linux
linux-aaaarch64
osx
windows