rmw_cyclonedds GitHub actions broken

[joespeed]

@hidmic @ThijsSassen Please help, thanks! It was pointed out to me today that rmw_cyclonedds GitHub actions are broken https://github.com/ros2/rmw_cyclonedds/actions Seems the actions are set up properly and there is a legit cppcheck failure we need to fix on OS X:
2020-08-14T17:21:06.0550080Z [8.838s] 2: [src/rmw_node.cpp:705]: (error: unknownMacro) There is an unknown macro here somewhere. Configuration is required. If RCUTILS_STRINGIFY is a macro then please configure it.

[christophebedard]

Might be related/useful: ros2/ros2#942

[ivanpauno]

See ros2/rosidl_typesupport_connext#57 for an example fix

[joespeed]

@ivanpauno thx! might you be able to? Most contributors in Netherlands have holiday. Let me know if not and we'll figure it out

[rotu]

@ivanpauno That looks like a workaround, not a fix. Why doesn't cppcheck find it in the first place?

[rotu]

Discovered this bug in trying to work around the issue: https://github.com/rotu/rmw_cyclonedds/actions/runs/232035117

It seems strange to me that rcutils is found in Linux but not on Mac/Windows. Is this caused by cppcheck 2.0 vs <2.0?

[ivanpauno]

@ivanpauno That looks like a workaround, not a fix. Why doesn't cppcheck find it in the first place?

Include paths of dependencies are not being passed automatically to cppcheck per discussion in ament/ament_lint#117.
The discussion was revisited and this patch was applied in ros2/rclpy#577.

It doesn't look ideal, but it doesn't look to bad either.
If you have any other proposal please open an issue in ament_lint repo.

[ivanpauno]

It seems strange to me that rcutils is found in Linux but not on Mac/Windows. Is this caused by cppcheck 2.0 vs <2.0?

I guess the version of cppcheck installed in both platforms is different.
I think that cppcheck <2.0 wasn't finding rcutils either, but it didn't require all macros to be configured (thus, there was no problem).

[rotu]

Problems at the interfaces of packages are the thorniest. If cppcheck can’t hope to detect such problems then it’s not very useful as a static analysis tool.

What value does cppcheck provide? In the current configuration does it detect anything cpplint doesn’t?

I think we should consider just disabling it.

[ivanpauno]

What value does cppcheck provide? In the current configuration does it detect anything cpplint doesn’t?

AFAIU, cpplint only does style checking and cppcheck is intended to detect bugs.
I haven't check how we're configuring both, but in general cpplint cannot replace cppcheck (clang-tidy could be a replacement, though some people combine both cppcheck and clang-tidy).

I wouldn't deactivate a linter only because of a one line workaround.
I think that opening an issue upstream to see how the problem can be solved in a better way is worth doing.

[rotu]

@ivanpauno alternate solution: ament/ament_lint#268

AFAIU, cpplint only does style checking and cppcheck is intended to detect bugs.

Yes, in theory. But that only holds true if it has enough context to detect such bugs.

I didn't recommend deactivating cppcheck because of a one-line workaround. I recommend it because including dependencies makes performance unacceptably bad and not including dependencies makes it unable to perform static analysis in a meaningful way.

I'm not sure why the performance hit was deemed unacceptably bad, but I suspect it's due to misuse of the tool (e.g. not passing definitions to cppcheck so it checks a combinatoric explosion of macro combinations) rather than its inherent limitations.

[iuhilnehc-ynos]

Hi all,

After using cppcheck(v2.1), there seems a workaround (by using ament_cmake_cppcheck_ADDITIONAL_INCLUDE_DIRS) to set '--include-dirs' for ament_cppcheck. But I think it's not good because there is already 'INCLUDE_DIRECTORYS' exist by find_package, why should I set ament_cmake_cppcheck_ADDITIONAL_INCLUDE_DIRS?

I think the following source code might be unreasonable, ( ament/ament_lint#117 (comment) )

https://github.com/ament/ament_lint/blob/58cd08c9f671eba22c9efc222b6f4476b11eec48/ament_cmake_cppcheck/cmake/ament_cmake_cppcheck_lint_hook.cmake#L69-L72