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

Improvements to ament_lint_clang_tidy. #316

Merged

Conversation

nuclearsandwich
Copy link
Contributor

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.

@nuclearsandwich nuclearsandwich self-assigned this Aug 4, 2021
Without this default ament_clang_tidy does not execute properly.
WIP: WHY?

Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>
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 <steven@nuclearsandwich.com>
@nuclearsandwich nuclearsandwich marked this pull request as draft August 4, 2021 18:40
Copy link
Contributor

@audrow audrow left a 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.

@nuclearsandwich
Copy link
Contributor Author

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 os.curdir and when run via ament_cmake_clang_tidy the working directory is the source directory. In this default state for an out-of-tree build it will fail since the compile commands database file required by clang-tidy is in the build directory rather than the source directory. But in a local test where I changed the working directory to the build directory it fails to locate source files properly.

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.

@mwcondino
Copy link
Contributor

Hey @nuclearsandwich , I was curious if there was an ETA on this PR? I'm looking into adding ament_clang_tidy into a project and it would be a little more ergonomic with these proposed changes merged.

@nuclearsandwich
Copy link
Contributor Author

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.

@nuclearsandwich nuclearsandwich marked this pull request as ready for review August 27, 2021 17:36
@nuclearsandwich
Copy link
Contributor Author

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

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.

@nuclearsandwich nuclearsandwich merged commit 1f5c6bb into ament:master Aug 31, 2021
@nuclearsandwich nuclearsandwich deleted the nuclearsandwich/fix-clang-tidy branch August 31, 2021 14:48
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.

None yet

3 participants