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

[Core] Add bazel run :refresh_compile_commands for clangd #47964

Merged
merged 10 commits into from
Oct 30, 2024

Conversation

dayshah
Copy link
Contributor

@dayshah dayshah commented Oct 10, 2024

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 :(

@jjyao jjyao added the go add ONLY when ready to merge, run all tests label Oct 11, 2024
@rynewang
Copy link
Contributor

Hi, what's difference vs

ray/WORKSPACE

Lines 36 to 38 in 61a4220

# Tools to generate `compile_commands.json` to enable awesome tooling of the C language family.
# Just run `bazel run @hedron_compile_commands//:refresh_all`
load("@hedron_compile_commands//:workspace_setup.bzl", "hedron_compile_commands_setup")
?

@dayshah
Copy link
Contributor Author

dayshah commented Oct 12, 2024

Hi, what's difference vs

ray/WORKSPACE

Lines 36 to 38 in 61a4220

# Tools to generate `compile_commands.json` to enable awesome tooling of the C language family.
# Just run `bazel run @hedron_compile_commands//:refresh_all`
load("@hedron_compile_commands//:workspace_setup.bzl", "hedron_compile_commands_setup")

?

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.

@rynewang
Copy link
Contributor

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

Signed-off-by: dayshah <[email protected]>
@dayshah
Copy link
Contributor Author

dayshah commented Oct 12, 2024

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

@rynewang
Copy link
Contributor

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.

@dayshah
Copy link
Contributor Author

dayshah commented Oct 14, 2024

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

@anyscalesam anyscalesam added triage Needs triage (eg: priority, bug/not-bug, and owning component) core Issues that should be addressed in Ray Core Devprod labels Oct 16, 2024
@jjyao jjyao removed the triage Needs triage (eg: priority, bug/not-bug, and owning component) label Oct 16, 2024
@dayshah dayshah requested a review from rynewang October 28, 2024 23:53
@rynewang rynewang enabled auto-merge (squash) October 29, 2024 17:18
@rynewang rynewang merged commit 7d912d6 into ray-project:master Oct 30, 2024
5 checks passed
Jay-ju pushed a commit to Jay-ju/ray that referenced this pull request Nov 5, 2024
…t#47964)

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.

Signed-off-by: dayshah <[email protected]>
Co-authored-by: Ruiyang Wang <[email protected]>
JP-sDEV pushed a commit to JP-sDEV/ray that referenced this pull request Nov 14, 2024
…t#47964)

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.

Signed-off-by: dayshah <[email protected]>
Co-authored-by: Ruiyang Wang <[email protected]>
@dayshah dayshah deleted the compile_commands branch November 15, 2024 04:35
mohitjain2504 pushed a commit to mohitjain2504/ray that referenced this pull request Nov 15, 2024
…t#47964)

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.

Signed-off-by: dayshah <[email protected]>
Co-authored-by: Ruiyang Wang <[email protected]>
Signed-off-by: mohitjain2504 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Issues that should be addressed in Ray Core Devprod go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants