ray icon indicating copy to clipboard operation
ray copied to clipboard

[Core] Add bazel run :refresh_compile_commands for clangd

Open dayshah opened this issue 1 year ago • 6 comments

Why are these changes needed?

Gives clangd the ability to fully index ray's c++ code after you run bazel run :refresh_compile_commands. Gives you lsp support + clang-tidy linting in ide to catch bad cpp practices in files based on rules already setup in .clang-tidy. https://github.com/ray-project/ray/blob/master/.clang-tidy A lot of these don't seem to be followed though and seem contrary to some of the style of existing code, but some are critical for not giving up performance gains off of small things. Trimming down to more important lints could be helpful.

Related issue number

Checks

  • [ ] I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • [ ] I've run scripts/format.sh to lint the changes in this PR.
  • [ ] I've included any doc changes needed for https://docs.ray.io/en/master/.
    • [ ] I've added any new APIs to the API Reference. For example, if I added a method in Tune, I've added it in doc/source/tune/api/ under the corresponding .rst file.
  • [ ] I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • [ ] Unit tests
    • [ ] Release tests
    • [ ] This PR is not tested :(

dayshah avatar Oct 10 '24 05:10 dayshah

Hi, what's difference vs https://github.com/ray-project/ray/blob/61a4220f564f1de740a4b2b0960be6bb661ed45e/WORKSPACE#L36-L38 ?

rynewang avatar Oct 12 '24 18:10 rynewang

Hi, what's difference vs

https://github.com/ray-project/ray/blob/61a4220f564f1de740a4b2b0960be6bb661ed45e/WORKSPACE#L36-L38

?

So I actually did try this but it fails because it isn't targeted specifically towards cpp code, so it tries to generate for the java code and fails. New one will just gen for the cpp code under ray_pkg. The new one also turns off generating for external sources by default, because it causes clangd to index almost 5k files instead of just 400 which takes like 30 mins vs. <5 mins. Just removed the comment there too, so people are directed towards the new command.

dayshah avatar Oct 12 '24 20:10 dayshah

Thanks. But if we disable external code indexing, will clangd lsp still work for e.g. asio or protobuf-generated code?

rynewang avatar Oct 12 '24 20:10 rynewang

Thanks. But if we disable external code indexing, will clangd lsp still work for e.g. asio or protobuf-generated code?

Ok so correction to my previous comment, this actually still causes a 4k index (i think i had some cache before), it still indexes the external headers just not cc files, so can still click into .pb.h files and other absl or boost headers. The full index without exclude_external_sources is close to 10k. And then there's one more possible option exclude_headers which would actually exclude the external headers which we don't want. Updated comment to reflect 2x, not 10x index time

dayshah avatar Oct 12 '24 23:10 dayshah

I think the external .cc file indexes are still useful for some devs since when debugging asio I sometimes click into them to see the details. But it slows down as you said. Would you mind making a command flag or another command to also enable external file indexing? Then I will merge this PR.

rynewang avatar Oct 13 '24 04:10 rynewang

I think the external .cc file indexes are still useful for some devs since when debugging asio I sometimes click into them to see the details. But it slows down as you said. Would you mind making a command flag or another command to also enable external file indexing? Then I will merge this PR.

makes sense, i added a second command so bazel run :refresh_compile_commands_external_sources will do it with the external sources while the standard command won't, comments for info are there too

dayshah avatar Oct 14 '24 02:10 dayshah