Skip to content
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

reenable cppcheck for rosbag2_transport #461

Merged
merged 2 commits into from
Jul 17, 2020
Merged

Conversation

Karsten1987
Copy link
Collaborator

@Karsten1987 Karsten1987 commented Jul 15, 2020

I wasn't aware of it and not sure why it got disabled in the first place, but with the following patch cppcheck passes successfully locally on my OSX.
Alternatively, one can place a // cppcheck-suppress unknownMacro above the two RCLCPP macros.

This PR was inspired by ros2/rclpy#577

CI (only testing cppcheck within rosbag2_transport):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987 Karsten1987 self-assigned this Jul 15, 2020
@Karsten1987
Copy link
Collaborator Author

it makes me a bit nervous that the GitHub actions (at least the linters) are all passing even though cppcheck was clearly complaining wo/ this patch.
@emersonknapp Do we have to bump them to a new version?

@emersonknapp
Copy link
Collaborator

I just checked and realized that the linters are running the version from Eloquent - #463 should clear that up

@emersonknapp
Copy link
Collaborator

I was realizing on #463 that I had to disable cppcheck for rosbag2_transport in the Action configuration - I tried incorporating this fix, but invoking the linter from outside of CMake seems to not take it into account. I think I'll need to leave the action-ros-lint cppcheck configuration disabled, and just expect to see it in the regular builds.

@Karsten1987
Copy link
Collaborator Author

Would you prefer to use the alternative solution to this problem and explicitly disable the unknownMacro for the two places inline? I have no problem with that either.

@emersonknapp
Copy link
Collaborator

emersonknapp commented Jul 17, 2020

I think I like that better. Linters feel like they should be able to operate without being involved in the build system.

If the macro really couldn't be found - the build wouldn't pass

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
@Karsten1987
Copy link
Collaborator Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Karsten1987
Copy link
Collaborator Author

Seems like the windows jenkins CI had a hiccup here. Given that I am testing only cppcheck and it's passing on all other platforms, I consider this good to go. Merging ...

@Karsten1987 Karsten1987 merged commit 8e71e9c into master Jul 17, 2020
@delete-merged-branch delete-merged-branch bot deleted the reenable_cppcheck branch July 17, 2020 23:37
@emersonknapp
Copy link
Collaborator

emersonknapp commented Jul 17, 2020

Looks like Jenkins triggered the rebuild too? Build Status

I don't fully understand how they tie together, but usually when the worker just crashes like that, I see an identical build that doesn't seem like a human actually started it.

emersonknapp pushed a commit that referenced this pull request Feb 2, 2021
* reenable cppcheck

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* suppress unknown macro inline

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
emersonknapp pushed a commit that referenced this pull request Feb 17, 2021
* reenable cppcheck

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* suppress unknown macro inline

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants