Add clang-tidy to CI

[vyasr]

Description

This PR adds clang-tidy checks to our CI. clang-tidy will be run in nightly CI via CMake. For now, only the parts of the code base that were already made compliant in the PRs leading up to this have been enabled, namely cudf source and test cpp files. Over time we can add more files like benchmarks and examples, add or subtract more rules, or enable linting of cu files (see https://gitlab.kitware.com/cmake/cmake/-/issues/25399). This PR is intended to be the starting point enabling systematic linting, at which point everything else should be significantly easier.

Resolves #584

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[vyasr]

Now that CI is passing here, have a number of questions that need to be addressed about how we want clang-tidy to work.

1. For consistency with all of our other linting, I've set up clang-tidy via pre-commit. This has many benefits, including making the linter completely self-contained so that anyone can run it locally and it installs the necessary dependencies as long as the user is able to run a CMake configure (so they must have compilers set up). However, clang-tidy doesn't really make sense to run as a pre-commit check because depending on the files that were changed it could take a very long time to complete. As a result, I've set it up as a manual hook that users have to run explicitly with pre-commit run clang-tidy --hook-stage manual. Does that approach seem fine to everyone?
2. clang-tidy doesn't really work well with the "run on changed files" approach that we normally prefer. That is because clang-tidy is based on a semantic analysis of the entire compilation of a TU. If a commit only changes headers, then clang-tidy won't do anything because there is no compilation command associated with a header. Conversely, if a later commit changes source files that include that header, it will suddenly surface the errors from previous changes. There isn't really a great way to handle that. One potential mitigating option would be to apply the logic that we do for the copyright check where we always check all files that are modified in a PR relative to the base branch. That still wouldn't catch all cases, but it would probably be much better. If we like that, I would suggest that we omit that from this PR and then work on adding a clang-tidy hook to https://github.com/rapidsai/pre-commit-hooks/ that can share the logic that we have already developed there.
3. If we run clang-tidy on all files in CI, which is how we run all our other pre-commit hooks, the runs are going to take ~3-4 hours. That is about as long as the entire CI pipeline takes on good runs (those that don't touch core headers and involve recompilation of large parts of the codebase, i.e. where we have a high rate of sccache hits). I don't think that we want to accept that cost right now. If we choose to implement the suggestion in 2 above, we could ameliorate this by temporarily using a bigger CI node for the clang-tidy checks until we have the more efficient diffing solution in place. That should reduce the time significantly. How do others feel about the tradeoffs here?
4. pre-commit can't be manually run with arguments, but we do need a way to explicitly run clang-tidy on a subset of files. Up until now I've been hacking this by editing the new run-clang-tidy.sh script to only run on that subset, but I'd love to hear ideas on better solutions. We could make that script accept arguments to run the desired subset, but then we wouldn't get the environment management for free (the right version of clang-tidy automatically installed) so I don't think that's good enough by itself.

P.S. the run-clang-tidy.py script that was already present before this PR was pretty buggy and had numerous issues, so I've opted to bypass it for now. The main feature it could offer is the ability to run clang-tidy on cu files, but that has been disabled for a long time anyway so I haven't bothered to try and enable it with the hook either. I've left the file in with the expectation that we could use it to make iterative improvements to our process and hopefully add that kind of coverage in as well eventually.

P.P.S. the cugraph-ops repo has an improved version of the above script that works better in some cases, but it has many warts of its own that we have struggled with in the past. I would prefer to constructively build up something that we can maintain well going forward rather than try to figure that version out from its current state.

[KyleFromNVIDIA]

I definitely don't want to run clang-tidy on the entire code base, or even individual files, every time I commit. clang-tidy should really be run as part of a CMake build - that's how CMake runs clang-tidy on itself. Even a manual hook seems a bit much, since clang-tidy does full compilation. Something like clang-format is better-suited for a pre-commit hook.

[KyleFromNVIDIA]

See https://gitlab.kitware.com/cmake/cmake/-/jobs/10338803 for an example of how CMake handles running clang-tidy in CI.

[vyasr]

• I assume that there's an add_custom_target or similar being used for clang-tidy, then?
• How do you control what version of clang-tidy is used? I don't want to be sensitive to the local version if we can help it because rules can change frequently.
• How do you decide what to run on? I see WARNING: clang-tidy-fixes.patch: no matching files. Ensure that the artifact path is relative to the working directory (/builds/cmake ci/long file name for testing purposes) , so I'm assuming it's using some logic to decide which files to run on? That seems like the same question we need to address one way or another.
• Does the way that CMake runs clang-tidy mean that you're basically compiling twice (one real run, one clang-tidy) on every CMake build?

[vyasr]

Based off some offline discussion, we're going to move from a pre-commit hook to invoking clang-tidy via CMake and then run it on all files in a nightly job. I'll update this PR accordingly.

[vyasr]

Some notes for reviewers:

• Here's what it's going to look like when a clang-tidy failure occurs. It's basically a compilation error.
• Here's the first fully successful run. Note that since it's a brand new build it's all sccache misses, so the runtime is long (~5 hrs) as expected.
• Once we have the cache populated, here's what a faster run looks like. If you were to watch this run line, you would see that libcudf.so finishes compiling within the first 10 mins or so, and the rest of the time is spent on the test files. The test file overhead being clang-tidy is consistent with my local experiments; if I run clang-tidy on the whole code base in parallel locally, I quickly start seeing all threads only operating on test files, so our source files get processed quickly relative to our tests. I suspect that this is because the tests include more headers and therefore involve a longer compilation chain.

[harrism]

Folks using an IDE that supports clang-tidy can see clang-tidy issues in real-time as you edit, BTW. So you don't have to wait for the nightly to fail. We should encourage this to avoid committing breaking changes.

[bdice]

All seems fine to me. Reminder to move the job to nightlies!

[vyasr]

All seems fine to me. Reminder to move the job to nightlies!

Done! Thanks for the review.

[vyasr]

/merge