-
Notifications
You must be signed in to change notification settings - Fork 110
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
Improvements to ament_lint_clang_tidy. #316
Improvements to ament_lint_clang_tidy. #316
Conversation
ce49bd8
to
c28e885
Compare
Without this default ament_clang_tidy does not execute properly. WIP: WHY? Signed-off-by: Steven! Ragnarök <[email protected]>
The "default" clang-tidy binary on focal is 10. So this search order should find clang-tidy 10 as clang-tidy, then clang-tidy-11, then lastly clang-tidy-6.0. I don't actually know if this is the correct behavior considering the package.xml dependency is for plain clang-tidy which will be platform dependent. Signed-off-by: Steven! Ragnarök <[email protected]>
c28e885
to
6f39d0b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks okay to me, once you feel good about the reason a default is needed.
Okay so I spent some time refreshing myself on these changes. I have some reservations about the hook script having a default argument if the default instead ought to be pushed further up, either in the ament_clang_tidy() cmake function or in the ament_clang_tidy script. ament_clang_tidy defaults to looking in I am less concerned with the hook containing a default argument after scrolling through some of the other hook scripts. Both ament_cppcheck and ament_cpplint have default variables. |
Hey @nuclearsandwich , I was curious if there was an ETA on this PR? I'm looking into adding |
I don't have an exact ETA but I did dig into this issue a bit further and re-assure myself that this approach is at least valid although I wish I could find a way to push the appropriate default argument down into the called CMake function rather than the hook script but I didn't find a way that worked as I expected. |
ament_cmake_clang_tidy isn't enabled by default in ament_lint_common so won't be used by default in any ROS 2 packages, but I wanted to make sure that the packages still build and that tests still run coherently with these changes, especially on non-linux platforms. So this CI run runs test_rclcpp tests along with building and testing ament_cmake_clang_tidy. |
Without this default in the hook script ament_clang_tidy does not execute properly.
This is still in draft while I determine why a default is needed in CMake when the python script wrapping clang-tidy should be handling the default instead.
The second commit updates the script to check for more than just clang-tidy-6.0.