Add filtering to ament_clang_tidy #280
Conversation
tylerjw
force-pushed
the
patch-1
branch
2 times, most recently
from
bfa183f
to
664c00b
Compare
There was a problem hiding this comment.
This looks very useful, especially the diff filter. Just some thoughts from my side:
Can't the repo path be filtered by providing source path suffixes passed on to clang-tidy or simply by running a subshell in the target directory? Otherwise, it would be really practicable to just filter by package names and not the repo folder (or not...?). Instead of catkin_topological_order better use ament's resource index using ament_index_python.
| return filtered_dbs | ||
|
|
||
|
|
||
| def filter_diff(compilation_dbs, repo_path, diff_head): |
There was a problem hiding this comment.
Only supporting git and searching for the binary explicitly is too restrictive IMO, using vcs-branch/vcs-diff removes a lot of logic from here and offers support for other vcs tools. https://github.com/dirk-thomas/vcstool
There was a problem hiding this comment.
Because of the nature of vcs it has to implement only the features common ammong version control systems. A second issue is that vcs is intended to be run in a source directory and crawl for repos. I need to make a query against one specific repo. As a result of the requirements on vcstool there is no interface in vcs for asking for the diff against a specific branch, filtering the types of diffs, or limiting results to just filenames, or limiting the diff to a directory within a repo.
For context here is a typical git diff command I want to run:
git diff --name-only --diff-filter=MA master..HEAD package_src_dir
This change adds two types of filtering to which files are tested with clang-tidy. The motivation behind this is clang-tidy itself can take a really long time to execute and as a result, we'd like to limit the number of files it tests to those changed in a given PR for CI for moveit. This adds three arguments to ament_clang_tidy: The first one allows filtering the packages tested to a single repo. This is useful in CI to limit the tests to the given repo you are executing on. Also, if it finds a .clang-tidy config file in the root directory of the repo it will use that instead of the default clang-tidy config. **known issue**: the implementation of this depends on on `catkin_topological_order`. Is there an ament/ros2 friendly tool for doing the same thing? This one uses git to compare the current state of the repo with some previous version and test only the files changed. This enables us to use clang-tidy in ci with large projects like moveit because it limits the scope of the clang-tidy test to the packages that were built. This depends on the `filter-repo-path` feature above because the git commands used to figure out what files changed need to be run from the repo directory. I feel the least strong about this feature. I added this because I wanted it while debugging what clang-tidy command was actually being executed. I think this could be a useful argument in general for python scripts that execute external programs. I'm working to improve the ci used by moveit and moveit2. I wrote this change to help with the migration away from travis we are going through with moveit (hence the catkin dependency). Because this script is not easily available in ros1 (asfaik) I have this PR to submit the contents of it with this change into a tools directory in moveit for our own use (moveit/moveit#2470). I'd appreciate advice if there is a better approach I should use to get ament scripts that are useful on ros1 projects. Signed-off-by: Tyler Weaver <[email protected]>
@henningkayser this is not a feature of the ament index as far as I can tell from the documentation and poking around at the code in it. ament index builds a an index in the filsystem of install locations and is used for resolving dependencies when linking or loading. This is very different from ansering the question "What packages exist in this source directory and it's children?" For that question there is |
|
I've updated this PR so it passes CI lint checks. I still don't have a solution for the two things Henning pointed out. As these changes (as they exist now) would introduce dependencies that are not desirable to have in an ament_lint (colcon or catkin), is there a better aproach to getting these features? Should they be contributed somewhere else? |
| if args.filter_repo_path: | ||
| repo_clang_tidy = args.filter_repo_path + '/.clang-tidy' | ||
| if os.path.exists(repo_clang_tidy): | ||
| args.config_file = repo_clang_tidy | ||
|
|
There was a problem hiding this comment.
This sort of default behavior I could see being an issue if someone specified a --config option and it was different from one found in the root of the repo they wanted to filter on. Lmk what you think. I'm not really attached to this feature and would happily remove it.
There was a problem hiding this comment.
Yeah, I think it would be confusing if specifying --filter-repo-path would change the config that is used.
If I recall correctly, currently the globally common .clang-tidy config would be used (unless --config is specified) regardless of the presence of a local .clang-tidy.
We could change it so that a local .clang-tidy would always take precedence, regardless of whether or not --filter-repo-path was specified.
|
Following up from my post at: https://answers.ros.org/question/368682/is-there-an-ament-version-of-catkin_topological_order/?answer=368873#post-id-368873
ament_clang_tidy may be the longest running linter/test at the moment, but the others can and will add up over time so it's better to have a system to deal with this generically rather than making every underlying tool aware of the repositories and code layout. |
|
@tfoote thankyou for the thoughtful response on this. Is there a specific place where you think this functionality would be welcome and make the most sense? Would it be best to implment this as a script that uses colcon or as a PR to some part of colcon? |
|
Part of why I was hoping to build this functionality into a standalone script outside of colcon test is that I want to use this on a ros1 repo (moveit) in CI. Unless ament_lint (and related packages) were released as a ros1 package I doubt I'll be able to use the mechanism of depending on tests in the package.xml to depend on an ament based test. This would require all our users who want to run our tests to have a source install of all the ament packages. This leads me to my follow-on question, is there a way to tell |
|
My overall suggestion is that at the higher level you want to only invoke the tests for the repositories that are of interest. colcon as the build tool already has a sophisticated set of command line options for including and excluding packages from the build and tests based on name and dependencies. These are all implemented via extensions. You could contribute another extension which would allow colcon to be aware of the repository from which a package comes from and filter the test invocations based on that detection. And that could likely be combined with the other filiters in useful manners, for example test all packages that are from repository X or have a dependency on packages from repository X to get coverage of downstream packages too. |
Description
This change adds two types of filtering to which files are tested with clang-tidy. The motivation behind this is clang-tidy itself can take a really long time to execute and as a result, we'd like to limit the number of files it tests to those changed in a given PR for CI for moveit.
This adds three arguments to ament_clang_tidy:
filter-repo-path
The first one allows filtering the packages tested to a single repo. This is useful in CI to limit the tests to the given repo you are executing on. Also, if it finds a .clang-tidy config file in the root directory of the repo it will use that instead of the default clang-tidy config.
known issue: the implementation of this depends on on
catkin_topological_order. Is there an ament/ros2 friendly tool for doing the same thing? update:colcon listprovides this same functionality, however I doubt ament wants that dependency either.filter-diff
This one uses git to compare the current state of the repo with some previous version and test only the files changed. This enables us to use clang-tidy in ci with large projects like moveit because it limits the scope of the clang-tidy test to the packages that were built. This depends on the
filter-repo-pathfeature above because the git commands used to figure out what files changed need to be run from the repo directory.verbose
I feel the least strong about this feature. I added this because I wanted it while debugging what clang-tidy command was actually being executed. I think this could be a useful argument in general for python scripts that execute external programs.
Background
I'm working to improve the ci used by moveit and moveit2. I wrote this change to help with the migration away from travis we are going through with moveit (hence the catkin dependency). Because this script is not easily available in ros1 (asfaik) I have this PR to submit the contents of it with this change into a tools directory in moveit for our own use (moveit/moveit#2470). I'd appreciate advice if there is a better approach I should use to get ament scripts that are useful on ros1 projects.