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

Add ament_cmake_clang_tidy to ament_lint_common #300

Draft
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

mm318
Copy link
Member

@mm318 mm318 commented Feb 14, 2021

Closes #283

This pull request will make ament_clang_tidy one of the default linters that get run during colcon test.

@audrow audrow self-assigned this Feb 19, 2021
@audrow
Copy link
Contributor

audrow commented Feb 19, 2021

Let me know when this is ready for a review.

Do you think that this will add a bunch of warnings downstream? Would it make sense to run CI for all downstream packages?

@mwcondino
Copy link
Contributor

Is this still being actively worked? I'd be interested in picking it up if not

@mm318
Copy link
Member Author

mm318 commented Sep 10, 2021

Sure @mwcondino, any help would be appreciated!

I think this involves more than just turning on clang-tidy though, such as assessing the impact on the building of current packages.

@nuclearsandwich
Copy link
Contributor

I've been doing some work to get clang-tidy running for a subset of ROS packages. An example build from that work is here: https://build.ros2.org/view/Rci/job/Rci__clang-tidy_ubuntu_focal_amd64/6/

Adding additional tools to ament_lint_common will, in my opinion, constitute a major version level change for the ament_lint packages and require a lot of communication to downstream consumers outside of the ROS 2 core.

And regarding the core, I do not think that clang-tidy can be added to ament_lint_common without a massive effort to review and squash existing warnings in the ROS 2 core packages. Some review is happening now and so we can expect some improvement from the state listed in the linked job but I don't think making clang_tidy part of ament_lint_common is likely during the ROS Humble development phase.

However, one of the conflicts this PR currently has is due to #316 which includes the same change in the CMake hook to properly set the script in the CMake build directory. I still have to work with @audrow to coordinate a release of ament_lint but if you're already using ament_lint_common and ament_lint_auto, you can add <test_depend>ament_cmake_clang_tidy</test_depend> and ament_lint_auto should find and hook it just like the linters in ament_lint_common.

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:14
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.

ament_cmake_clang_tidy: No compilation database files found
4 participants