-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rmw_cyclonedds GitHub actions broken #220
Comments
Might be related/useful: ros2/ros2#942 |
See ros2/rosidl_typesupport_connext#57 for an example fix |
@ivanpauno thx! might you be able to? Most contributors in Netherlands have holiday. Let me know if not and we'll figure it out |
@ivanpauno That looks like a workaround, not a fix. Why doesn't cppcheck find it in the first place? |
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? |
Include paths of dependencies are not being passed automatically to cppcheck per discussion in ament/ament_lint#117. It doesn't look ideal, but it doesn't look to bad either. |
I guess the version of cppcheck installed in both platforms is different. |
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. |
AFAIU, cpplint only does style checking and cppcheck is intended to detect bugs. I wouldn't deactivate a linter only because of a one line workaround. |
@ivanpauno alternate solution: ament/ament_lint#268
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. |
Hi all, After using cppcheck(v2.1), there seems a workaround (by using I think the following source code might be unreasonable, ( ament/ament_lint#117 (comment) ) |
@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.
The text was updated successfully, but these errors were encountered: