Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integration with bazel-compile-commands-extractor #20

Open
aminya opened this issue May 6, 2022 · 10 comments
Open

Integration with bazel-compile-commands-extractor #20

aminya opened this issue May 6, 2022 · 10 comments

Comments

@aminya
Copy link

aminya commented May 6, 2022

bazel-compile-commands-extractor can generate compiled_commands.json files. I tried to hot patch the shell binary and add -p $PROJECT_ROOT to the clang-tidy command. However, it seems that still clang-tidy fails to find some headers.

https://github.com/hedronvision/bazel-compile-commands-extractor

The issue seems to be a mismatch between the config and compilation mode. bazel-compile-commands-extractor accepts extra flags like -- --config=debug --compilation_mode=dbg, but bazel_clang_tidy uses fastbuild by default.

@aminya
Copy link
Author

aminya commented May 11, 2022

I wonder if @cpsauer can help expose some of the things that bazel_clang_tidy needs. It seems that adding -p project_root doesn't fix the missing header errors.

@cpsauer
Copy link

cpsauer commented May 11, 2022

Hey, @aminya!

My understanding is that clangd embeds clang-tidy these days, providing feedback as you edit if you want. So if you're already using clangd and generating a compile_commands.json with our tool, maybe that'd be a good way to get automated style feedback! Would love to hear your experience with that if you end up trying it.

[It looks like the goal of this project is a little different. They're looking to accommodate folks without a compile_commands.json (or clangd) and provide the style feedback as a build-style error. That's great stuff, but you've got the compile_commands.json and clangd already, I think. (All that said, I'm no clang-tidy expert (yet)--nor experienced with this project, so please tell me if I'm wrong or it there's a key integration we should be making.)]

Cheers,
Chris

@cpsauer
Copy link

cpsauer commented May 12, 2022

P.S. For generated files, you'll want to have flags match when you build and when you use the tools! It's definitely a tricky thing to handle well for tool builders.

@aminya
Copy link
Author

aminya commented May 14, 2022

My issue with clangd's clang-tidy is that it doesn't show all the issues in the workspace to me. I need to open each file to trigger it. It also doesn't seem to work when a function is templated. I have to call that template in my tests to trigger those warnings. That's why running clang-tidy form CLI shows more warnings that are not shown by clangd.

@cpsauer
Copy link

cpsauer commented May 14, 2022

Oh, got it. Makes sense.

To confirm, is the draw of this tool, then, over just running clang-tidy directly w/ the compile_commands.json, speed, since it takes care of caching via bazel?

Seems like some of the same questions remain, though: Is this tool designed to take a compile_commands.json file if you supply one?

@erenon
Copy link
Owner

erenon commented May 14, 2022

It is not, there's no need for a json file

@cpsauer
Copy link

cpsauer commented May 14, 2022

Hey, Thaler! Great to meet another Bazel toolmaker. Seems like a great thing you've built here.

@aminya
Copy link
Author

aminya commented May 19, 2022

I realized that with the help of hedronvision/bazel-compile-commands-extractor, running clang-tidy becomes just a matter of specifying the .cpp files (for example using git ls-files). It works accurately without any missing headers!

# first generate compile_commands.json
bazel run refresh_compile_commands -- --config=debug --compilation_mode=dbg
# run clang-tidy on cpp files
files=$(git ls-files --exclude-standard | grep -E '\.(cpp|c|cc|cxx)$')
clang-tidy -p ./ $files

# -p ./ specifies the path of the folder that compile_command.json is generated

It might be worthwhile adding this to the readme files.

@cpsauer
Copy link

cpsauer commented May 20, 2022

@aminya, yeah! That's what I meant. You mean adding it to our README, yeah?
Glad to hear getting the compile commands solves the header things for you.
I assume it doesn't have the same caching as it would under this tool. But fast enough for you? Or are we trying to find a way to unite the two or add caching to our tool? Do we understand what was going wrong with the header finding here otherwise?

@cpsauer
Copy link

cpsauer commented May 24, 2022

@aminya, I'm creating an issue to track over at hedronvision/bazel-compile-commands-extractor#52

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants