Add clang tidy to ament linters #155
Conversation
Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
|
|
||
| bin_names = [ | ||
| 'clang-tidy', | ||
| 'clang-tidy-6.0', |
There was a problem hiding this comment.
You may want to add clang-tidy-3.8 here, as that is the version on Xenial.
There was a problem hiding this comment.
Why is the version from Xenial relevant here? Our current target platform is Bionic which ships 6.0.
There was a problem hiding this comment.
For ROS 2, yes. For another project that we are considering using this on, they still target Xenial. The other linters also generally support older versions, so it seems like we should follow that trend.
There was a problem hiding this comment.
I would expect significant differences between version 6.0 and 3.8. So I am not sure if supporting such wide spread is feasible.
The other linters also generally support older versions, so it seems like we should follow that trend.
That is not really an argument that we haven't taken the time to update other places...
| return | ||
|
|
||
|
|
||
| def find_executable(file_names): |
There was a problem hiding this comment.
Hm. The duplication of find_executables, get_files, get_xunit_content, etc. with other ament linters is pretty unfortunate. However, I see that is how we have done it for all of the linters, and I think it deserves a different patch. I'll request that you open an issue to investigate how to share more code between them.
There was a problem hiding this comment.
I was thinking the same thing implementing this section - I'll open the issue shortly
There was a problem hiding this comment.
#157 is the new issue.
Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
| return | ||
|
|
||
|
|
||
| def find_executable(file_names): |
There was a problem hiding this comment.
#157 is the new issue.
Signed-off-by: John Shepherd <johnshepherd96@yahoo.com>
|
|
||
| include("${ament_cmake_clang_tidy_DIR}/ament_clang_tidy.cmake") | ||
|
|
||
| ament_register_extension("ament_lint_auto" "ament_cmake_clang_tidy" |
There was a problem hiding this comment.
@JShep1 @clalancette this is unfortunate as it may prevent us from backporting without breaking havoc on packages that already rely on ament_lint_auto :(
There was a problem hiding this comment.
Is there a strong reason to have it hooked up? If not, and @dirk-thomas agrees, I'd say we remove it to preclude any issues once the patch release is out.
There was a problem hiding this comment.
A package must be find_package()-ing this in order to trigger the extension being registered. So I don't see how this change would be a problem - even with ament_lint_auto since no package depends on this new package yet.
There was a problem hiding this comment.
A package must be
find_package()-ing this in order to trigger the extension being registered.
I see it commented out in ament_lint_common, yes.
So I don't see how this change would be a problem - even with ament_lint_auto since no package depends on this new package yet.
That's true of our packages. Could it ever be a problem if downstream packages start depending on it? If that's too far fetched, then I withdraw my concerns and suggestion.
There was a problem hiding this comment.
If packages start depending on it they probably intend to use it?
There was a problem hiding this comment.
Argh, I wrote half of it. I meant to say that it may be a problem if downstream packages start depending on it while building core packages that depend on ament_lint_auto from source.
There was a problem hiding this comment.
How would that effect downstream packages in any way?
There was a problem hiding this comment.
A package must be find_package()-ing this in order to trigger the extension being registered.
If a downstream package depends on this linter, it'll get registered as an ament_lint_auto extension. If it gets registered as an ament_lint_auto extension, it'll get hooked up as a linter for packages that depend on ament_lint_auto. That is the case for core packages as well. Causing lint failures if building those from source. Or at least that's my train of thought.
There was a problem hiding this comment.
No package I am aware of declares a linter package as an exported dependency. They are usually only test dependencies.
There was a problem hiding this comment.
Alright, not a problem then :)
Adds
ament_clang_tidyfunctionality to ament with default Google configuration checks enabled.