Fix ament_clang_tidy

[mm318]

Changes made

• The argument is now meant to point towards a compile_commands.json file or a directory that contains it, instead of source files
• Unit tests source files are currently skipped because gtest has linter violations that the ROS2 package developer wouldn't have control over

How to use/test

Follow the README to install necessary dependencies (for example on Ubuntu 18.04, do apt install clang-tidy-6.0 clang-tools-6.0 python-yaml).
In a workspace that contains the packages that you want to lint (or want to test the linter on):

• Build/rebuild with colcon build --cmake-args -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
• Run ament_clang_tidy build/
  • Expect to see a list of linter warnings/errors printed to stdout
  • Prior to this fix, all of the linter errors are just complaints about missing header files
• Run ament_clang_tidy build/ --xunit-file test.xml
  • Expect to see a list of linter warnings/errors printed to stdout
  • Expect to see the same list of warnings/errors in the outputted test.xml file

[dirk-thomas]

Maybe you can elaborate why removing existing options like --fix-errors or --header-filter is necessary to address the question about how to use this linter?

[mm318]

--fix-errors is not implemented in this PR. I was thinking of adding it in a separate PR. By the way, clang-tidy can't automatically fix all the issues that it flags.

The way the code in this PR sets up --header-filter by default works, and I think if the user were to override it, the user would either get too many irrelevant linter warnings/errors or too few relevant ones. I could put this option back, though I would then recommend against using it.

[mm318]

Hi @dirk-thomas, can you please take a look again?

[dirk-thomas]

The current patch does multiple unrelated changes (e.g. change semantic of the paths argument, use multiprocessing, unrelated style changes, change invoked scripts and arguments, etc.), which make it more complex and unnecessary complicated to review. Please split the contribution into separate PRs each focus on a single change.

[claireyywang]

@mm318 friendly ping

[mm318]

I will get to it before end of next week. Thanks.

[zmichaels11]

@mm318 any status updates on this?

[thomas-moulard]

@mm318 any status updates on this?

[mm318]

I have updated the pull request by reducing the amount of changes.

[claireyywang]

@mm318 Thanks for contributing! Can you plz include more information on the PR description about the final list of changes this PR introduces, how to test and what to expect from testing?

[mm318]

Only use clang-tidy 6.0 for now, because it is the most ubiquitous, the default version of clang-tidy on Ubuntu Bionic seems to be a moving target, and the parsing of linter warning/error messages has very recently been made difficult by an update to the default version of clang-tidy on Ubuntu Bionic. This is also consistent with instructions in the README that's part of this pull request.

[dirk-thomas]

The master branch is not targeting Ubuntu Bionic anymore but Ubuntu Focal. And the default version of clang-tidy in Focal is 10.0. So I am not sure it makes sense to target version 6 here...

[mm318]

Will the master branch be backported to the branches that target Ubuntu Bionic, for example for the Dashing distro? Since the clang-tidy-6.0 apt package is valid for both Ubuntu Bionic and Focal, I think we should stick with clang-tidy 6.0 for now, then maybe backport, then target clang-tidy 10.0 (along with other features that I have in mind for this package).

[dirk-thomas]

Changes need to be explicitly backport through PRs. It won't happen automatically.

Using version 10 on Focal should happen before the upcoming freeze date. I don't mind which way that happens (target version 10 in this PR and then make a backport PR for Eloquent / Dashing which uses version 6) orland this PR targeting 6 and followingup with a PR to switch to version 10 with the intention to only backport the first.

[mm318]

Let's land this PR targeting clang-tidy 6.0, with a follow-up PR to switch to clang-tidy 10.0.

[mm318]

Hi @claireyywang, I have updated the pull request description with more information. Thanks.

[claireyywang]

Got a KeyError when testing on demos repo, traceback see below:

Traceback (most recent call last):
  File "/home/claire/ros2_ws/install/ament_clang_tidy/bin/ament_clang_tidy", line 11, in <module>
    load_entry_point('ament-clang-tidy', 'console_scripts', 'ament_clang_tidy')()
  File "/home/claire/ros2_ws/build/ament_clang_tidy/ament_clang_tidy/main.py", line 198, in main
    report[current_file].append(copy.deepcopy(data))
KeyError: '/usr/share/opensplice/cmake/../../../include/opensplice/dcps/C++/SACPP/mapping/UFL.h'

Also the process took more than 5 mins to output all the warnings, is that expected?

[mm318]

Hi @claireyywang,

Got a KeyError when testing on demos repo

I have fixed the issue that you found.

Also the process took more than 5 mins to output all the warnings, is that expected?

Yes. It takes this long because the demos repo has 14 packages and I have removed the multiprocessing feature from this pull request to reduce the scope of changes. Also, clang-tidy is a more sophisticated and compute intensive linter than the other linters.

Thanks!

[claireyywang]

CI
Linux
Linux-aarch64
OSX
Windows

[claireyywang]

LGTM

[mm318]

Thanks for merging!

[claireyywang]

Thanks for merging!

np!