[FR]: Add clang-tidy

[alexeagle]

What is the current behavior?

No response

Describe the feature

Followup to #101 suggested by @jsharpe

[HarveyJMartin]

Hi, has there been any progress on the addition of this ?

[jsharpe]

I don't think anyone is actively working on this at the moment but checkout https://github.com/erenon/bazel_clang_tidy for something that might meet your needs / the basis for a contribution into this ruleset.

[peakschris]

@alexeagle do you have any guidance as to how this should be done? Should rules_lint reference bazel_clang_tidy, or should the aspect from bazel_clang_tidy be copied and pasted into rules_lint? (i note kind offer from bazel_clang_tidy owner, and also that bazel_clang_tidy seems to be under active development by the community)

Do you think clang-tidy fixes should be integrated similar to eslint with the patch files output and used for fixes?

[alexeagle]

Right, I forgot about erenon/bazel_clang_tidy#35

I imagine that bazel_clang_tidy would be replaced by clang_tidy in rules_lint, so I wouldn't want to add a dependency from this repo to that one. Instead I'd suggest using their implementation as a reference, maybe some copy-paste code but only with understanding it.

Yes, fixes should be applied the same way we do for ruff and eslint already, just by running clang-tidy in its fix mode, and the resulting patch produced as an action by rules_lint.

[peakschris]

I'm attempting to work on this, and could use some advice:

• I've done the integration, but no report files are being generated when I run. I'm new to aspects, and don't know how to add debug information or how to trace their execution -- print statements seem to be ignored.
• I'm also unclear how to handle the single-input clang-tidy vs the design for rules_lint which seems to process multiple files at a time. I've put something together but it's probably wrong or not optimal and advice would be welcome.

The repo should work, if someone has time to clone and try it out and give me some advice, or even just scan through the code I'd be hugely appreciative!

More info on my questions/issues in /README.md

Thanks! Chris
https://github.com/peakschris/rules_lint/tree/clang-tidy

[peakschris]

Don't worry, I figured it out. I had a required_providers = [CcInfo] in the aspect, removing it allows clang-tidy to run. I'll progress with improving the code and prepare for a PR.

[peakschris]

I've submitted #284 for review/discussion

[jsharpe]

@alexeagle @peakschris - giving this a go on my codebase where we have a forked version of the erenon rules running clang-tidy. I'll describe issues I run into below, but let me know if you'd prefer me to open separate issues.

#298 is a small doc update for this
#299 Fixes finding bash when its not in /usr/bin

Next error I'm running into is it not finding some of the includes so I suspect the construction of the command-line isn't quite correct; this is primarily with RBE.
With local execution it gets a bit further, but still has some missing include paths, but also seems to be missing the c++ standard flag which I'm passing via the cxx_standard attribute in the llvm module extension.

[peakschris]

@jsharpe thanks for trying this and for your fixes.

My environment was windows/MSVC and linux/g++, so I can imagine there might be some differences. With MSVC I needed to pass INCLUDE through as an envvar for it to see system includes (via .bazelrc) and for both msvc and g++ the C++ standard is set via cxxopts. For both of these I imagine LLVM is a bit different. If you'd be able to dig into the issues that would be great, otherwise I can have a look if you can create me a minimal reproducer?

[jsharpe]

#300 should fix the setting of the c++ standard and fixes to some extent the include issues as it properly propagates the include flags.

[jsharpe]

I think there's also a few inputs to the shell action missing - notably the cc_toolchain.all_files depset. Also I think compilation_context.headers doesn't include the textual headers by default - in our fork of the original aspect we are constructing a depset like:
inputs = depset(direct = [infile] + compilation_context.direct_headers + compilation_context.direct_textual_headers, transitive = [compilation_context.headers])
The other thing I've noted in the clang-tidy action is it passes both env and use_default_shell_env = True - env will be ignored on older versions of bazel where https://bazel.build/reference/command-line-reference#flag--incompatible_merge_fixed_and_default_shell_env has not yet been implemented / flipped. I doubt this will be much of an issue but just wanted to note that it is relying on this flag being on.

[peakschris]

@jsparpe thank you for your fixes

I've submitted #304 which improves env handling, providing the env expected by the regular compiler toolchain to clang-tidy. I've also removed use of use_default_shell_env - thank you for your note on this.

[alexeagle]

FYI @peakschris I just talked to a company that would like to use this, and likely can fund some of the development work, is that interesting for you?

[peakschris]

@alexeagle, thanks for asking, I'm not available for work, but anything that can be done to make this feature better would be great. YGM.