Allow excluding checkers from ament_lint_auto

[jpsamper2009]

This PR implements the enhancement proposed in #132. It consists of two commits:

1. A bug fix to the way ament_lint_auto_find_test_dependencies detected unused arguments
2. Adding AMENT_LINT_AUTO_EXCLUDE variable to ament_execute_extension

• See Add option to exclude packages in ament_execute_extensions: ament_cmake#165 also
• Example: Check copyright of files where possible: ros/class_loader#123

How to test

• Take a package like rclcpp which uses ament_lint_auto_find_test_dependencies (https://github.com/ros2/rclcpp/blob/master/rclcpp/CMakeLists.txt#L140) :

1. Introduce a couple of style error in a source file, run colcon build, colcon test --packages-select rclcpp and see that the cpplint and uncrustify tests fail

2. Exclude one of the checkers:

list(APPEND AMENT_LINT_AUTO_EXCLUDE
  ament_cmake_uncrustify
)
ament_lint_auto_find_test_dependencies()

• Run colcon build --packages-select rclcpp, colcon test --packages-select rclcpp and see that the uncrustify test does not exist any more

3. Find and add the ament_cmake_uncrustify check explicitly, excluding the file with the style errors:

list(APPEND AMENT_LINT_AUTO_EXCLUDE
  ament_cmake_uncrustify
)
ament_lint_auto_find_test_dependencies()
find_package(ament_cmake_uncrustify)

set(good_sources "${rclcpp_SRCS}")
list(REMOVE_ITEM good_sources <bad-file>)
ament_uncrustify(${good_sources})

• uncrustify should succeed

4. Exclude ament_cmake_cpplint

list(APPEND AMENT_LINT_AUTO_EXCLUDE
  ament_cmake_uncrustify
  ament_cmake_cpplint
)
ament_lint_auto_find_test_dependencies()
find_package(ament_cmake_uncrustify)

set(good_sources "${rclcpp_SRCS}")
list(REMOVE_ITEM good_sources <bad-file>)
ament_uncrustify(${good_sources})

• cpplint test does not exist anymore

5. Add the cpplint test (before ament_lint_auto), but use the FILTERS option to suppress the error message:

find_package(ament_cmake_cpplint)
ament_cpplint(FILTERS "-foo")

list(APPEND AMENT_LINT_AUTO_EXCLUDE
  ament_cmake_uncrustify
  ament_cmake_cpplint
)
ament_lint_auto_find_test_dependencies()

find_package(ament_cmake_uncrustify)
set(good_sources "${rclcpp_SRCS}")
list(REMOVE_ITEM good_sources <bad-file>)
ament_uncrustify(${good_sources})

• cpplint test succeeds

[jpsamper2009]

@dirk-thomas I think I've addressed all your comments. I have created the following PR (ament/ament_index#31) as an example of using the argument

[dirk-thomas]

I don't think the current approach with the global flag works well in a kind of scenarios. E.g. a package finds on of the linters before (when the new global flag isn't set yet).

How about just storing the excluded linter names in a global variable and instead of blindly triggering the extension point with ament_execute_extensions(ament_lint_auto) iterate over the registered extensions and skip the ones from the exclude list? That avoids all custom magic and find package handling.

[jpsamper2009]

@dirk-thomas Are you suggesting that we replace the call to ament_execute_extensions(ament_lint_auto) with

the contents of ament_execute_extensions.cmake + some logic for excluded packages

OR

to modify ament_execute_extensions.cmake to look for a global variable with a list of excluded packages?

[dirk-thomas]

Copying the logic is not great. Also making this function rely on global state doesn't seem nice. How about adding an optional keyword argument to ament_execute_extensions to skip a set of given package names?

[jpsamper2009]

@dirk-thomas The issue with relying on ament_execute_extensions is that the tests would still be registered but not executed, which means that you would still not be able to register a test a customer test because there would already be a test with that name

[dirk-thomas]

If a certain linter is being excluded it shouldn't define any test so I don't see why the test should be registered in that case? Also each CMake function adding a linter test has a TESTNAME keyword argument to define your own test name (though based on the previous sentence I don't think this should be necessary if the linter is excluded).

[jpsamper2009]

@dirk-thomas I've pushed a new proposed solution based on our discussions above. This requires changes to ament_cmake too: ament/ament_cmake#165

I have tested it with our own packages, but I don't seem to find a good package in ros2 to show this working. What would be the best way to provide an example?

[dirk-thomas]

grep -R "_FOUND TRUE)" through the ROS 2 workspace finds a few candidates - either of them would be a good example:

• class_loader
• kdl_parser
• test_security

[jpsamper2009]

@dirk-thomas Sounds good. See ros/class_loader#123

[jpsamper2009]

@dirk-thomas This is ready for a review

[jpsamper2009]

@dirk-thomas One last review, hopefully 😄

[dirk-thomas]

One last review, hopefully 😄

All good 👍 Thanks for iterating on it.

[dirk-thomas]

I added a version dependency for this since the new argument is only available as of version 0.7.0: 8b6c495