Auto-configure compile-commands-dir for clangd

[petr-tik]

trafficstars

Fix #2278

Depends on fixing lsp-workspace-root to be available when opening a new workspace

Tested locally on projects with and without a compile_commands.json file.

[petr-tik]

@yyoncho as discussed here, feel free to mark this PR as WIP or wait for lsp-workspace-root to support this workflow, wanted to get your thoughts as it's my first real elisp contribution.

[petr-tik]

@nbfalcon @yyoncho thanks for such a timely and thorough code review! I will try to address it properly over the weekend - you guys rock. I think i might be able to dedicate some time to clangd and rust-analyzer (work and play respectively) clients, so if you think my contributions are useful - i would like to continue

[yyoncho]

I think i might be able to dedicate some time to clangd and rust-analyzer (work and play respectively) clients, so if you think my contributions are useful

Feel free to pick whatever task you want and ask for help/clarification/etc. We try to make contributions to the project as pleasant as possible.

[petr-tik]

We try to make contributions to the project as pleasant as possible.

You succeed in making contributions as pleasant as possible.

Is it worth tagging issues with relevant client tags? eg. clangd rust-analyzer pyls microsoft-python-server

etc

[petr-tik]

I rebased onto upstream/master, so my CI should run now.

I was thinking about adding a small cpp project with a CMakeLists file to run integration tests similar to what python has.

I don't see how useful it will be here but might be useful in other features.

[kiennq]

Btw, did you take a look at initializationOptions.compilationDatabasePath extension from clangd? https://clangd.llvm.org/extensions.html

[petr-tik]

Coming back to this PR after a bit of a delay.

Btw, did you take a look at initializationOptions.compilationDatabasePath extension from clangd? https://clangd.llvm.org/extensions.html

Yes, I didn't write all my reasons not to include it down, but from memory some of them were: keeping compilation database path in the lsp-clients-clangd-args enables setting it through .dir-locals.el cmdline args will show up in lsp-log and make it easier for people to file informative bug reports about clangd misbehaving The client-args variable already existed, whereas nothing like that existed for init options for clangd

[kalj]

@petr-tik How is this going? I too am looking forward to this :+1:

[petr-tik]

@petr-tik How is this going? I too am looking forward to this +1

what a trip down memory lane! I should have done something explicit with this PR though.

@kiennq had a good point about this PR performing a potentially expensive check.
https://github.com/emacs-lsp/lsp-mode/pull/2303/files#r521761801

since then clangd added the logic to automatically look up $PROJECT_ROOT/build/ https://clangd.llvm.org/installation.html#compile_commandsjson

and enabled configuring clangd (per project or system-wide) with the name of build directory https://clangd.llvm.org/config#compilationdatabase

I would love lsp-mode or clangd to work better out of the box, but I feel that imposing a performance penalty on startup for all users is too high a cost for this.

Maybe lsp-clangd can send a warning/info message to the user if clangd fails to find compile_commands.json and refer to documentation to help the user configure their workspace.

Thoughts?

[yyoncho]

Maybe lsp-clangd can send a warning/info message to the user if clangd fails to find compile_commands.json and refer to documentation to help the user configure their workspace.

Thoughts?

That warning should be implemented on the server because generally lsp-mode is not aware of that file. But also, note that for hello world apps it will be confusing and in the end, it will be confusing for the beginners...

[sambrightman]

I feel this PR still has some legs. Whilst clangd now checks build/compile_commands.json there remain quite a few variations (e.g. build_xyz/, build/releasemode/, build/generatortype, out/). It feels like restricting the search to a few prefixes and 2 or 3 levels of depth (both configurable?) would cover a very high percentage of cases not currently covered whilst also greatly reducing the initialisation cost.

[yyoncho]

I feel this PR still has some legs. Whilst clangd now checks build/compile_commands.json there remain quite a few variations (e.g. build_xyz/, build/releasemode/, build/generatortype, out/). It feels like restricting the search to a few prefixes and 2 or 3 levels of depth (both configurable?) would cover a very high percentage of cases not currently covered whilst also greatly reducing the initialisation cost.

@sambrightman we can go in this direction. I am closing this PR for now. If someone is interested can go with @sambrightman suggestion.