mesonbuild / vscode-meson

Meson for VisualStudio Code
Apache License 2.0
99 stars 49 forks source link

Setup compile commands directory for the clangd extension #210

Closed wolfpld closed 6 months ago

wolfpld commented 7 months ago

This adds the compile_commands.json setup for vscode-clangd.

The main drawback here is that we are overwriting workspace configuration for clangd arguments. A solution for this is provided in https://github.com/clangd/vscode-clangd/pull/544, but that PR has not received any attention so far.

Another problem is that clangd is not automatically restarted when its configuration is changed, so a manual restart is needed. It is solved in https://github.com/clangd/vscode-clangd/pull/544 by creating a listener, but for now we need to workaround by manually issuing a clangd.restart command. I believe we are racing with clangd init with this, as I've rarely seen some odd clangd warnings, but these didn't seem to be of major consequence.

wolfpld commented 7 months ago

Note that the clangd race condition can be reproduced by creating the following shell script:

#!/bin/sh
sleep 10
clangd "$@"

And then setting the clangd.path in VS Code preferences to point to it. If the user manually executes the clangd.restart command from the command palette during the sleep, this is the result:

[Error - 00:37:37] Server initialization failed.
Error: command 'clangd.applyFix' already exists
    at i.registerCommand (/home/wolf/.vscode-server/bin/1a5daa3a0231a0fbba4f14db7ec463cf99d7768e/out/vs/workbench/api/node/extensionHostProcess.js:125:138275)
    at Object.registerCommand (/home/wolf/.vscode-server/bin/1a5daa3a0231a0fbba4f14db7ec463cf99d7768e/out/vs/workbench/api/node/extensionHostProcess.js:140:20532)
    at N1.register (/home/wolf/.vscode-server/extensions/llvm-vs-code-extensions.vscode-clangd-0.1.25/out/bundle.js:37:71984)
    at N1.initialize (/home/wolf/.vscode-server/extensions/llvm-vs-code-extensions.vscode-clangd-0.1.25/out/bundle.js:37:71547)
    at gf.initializeFeatures (/home/wolf/.vscode-server/extensions/llvm-vs-code-extensions.vscode-clangd-0.1.25/out/bundle.js:39:19266)
    at gf.doInitialize (/home/wolf/.vscode-server/extensions/llvm-vs-code-extensions.vscode-clangd-0.1.25/out/bundle.js:39:7616)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at gf.start (/home/wolf/.vscode-server/extensions/llvm-vs-code-extensions.vscode-clangd-0.1.25/out/bundle.js:39:4636)
[Error - 00:37:37] Clang Language Server client: couldn't create connection to server.
Error: command 'clangd.applyFix' already exists
    at i.registerCommand (/home/wolf/.vscode-server/bin/1a5daa3a0231a0fbba4f14db7ec463cf99d7768e/out/vs/workbench/api/node/extensionHostProcess.js:125:138275)
    at Object.registerCommand (/home/wolf/.vscode-server/bin/1a5daa3a0231a0fbba4f14db7ec463cf99d7768e/out/vs/workbench/api/node/extensionHostProcess.js:140:20532)
    at N1.register (/home/wolf/.vscode-server/extensions/llvm-vs-code-extensions.vscode-clangd-0.1.25/out/bundle.js:37:71984)
    at N1.initialize (/home/wolf/.vscode-server/extensions/llvm-vs-code-extensions.vscode-clangd-0.1.25/out/bundle.js:37:71547)
    at gf.initializeFeatures (/home/wolf/.vscode-server/extensions/llvm-vs-code-extensions.vscode-clangd-0.1.25/out/bundle.js:39:19266)
    at gf.doInitialize (/home/wolf/.vscode-server/extensions/llvm-vs-code-extensions.vscode-clangd-0.1.25/out/bundle.js:39:7616)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at gf.start (/home/wolf/.vscode-server/extensions/llvm-vs-code-extensions.vscode-clangd-0.1.25/out/bundle.js:39:4636)

I do not think we should be concerned with bugs in proper command handling / queuing in third-party extensions.

tristan957 commented 6 months ago

Can you rebase this on main. I basically cherry-picked what you had.

wolfpld commented 6 months ago

Rebased.

tristan957 commented 6 months ago

Sorry for forcing another rebase on you :(

wolfpld commented 6 months ago

Rebased.

tristan957 commented 6 months ago

Can you squash the commits?

wolfpld commented 6 months ago

Done.

wolfpld commented 6 months ago

Are there any remaining issues that would block the merge?

tristan957 commented 6 months ago

I just haven't gotten a chance to review and test.

wolfpld commented 6 months ago

To consider: The CMake VS Code extension solves this problem by copying compile_commands.json from the active build directory to a user-specified location, such as the workspace root, where clangd can pick it up without having to modify the workspace settings.

One big downside of this approach is it modifies .vscode/settings.json in user's source tree, and some projects does track that file in git...

This becomes more problematic if you have a multi-root workspace. In this case, the roots are enumerated in the same file that contains the settings, so you can't even choose not to include them in the repository.

Unfortunately, I won't be working on this anymore due to how things like https://github.com/mesonbuild/meson/pull/5859 are handled. Meson is basically useless if you want to work on Windows, or if you want to configure the build directory from an IDE like VS Code, or really do anything outside of the one and only allowed use case.

tristan957 commented 6 months ago

Meson is basically useless if you want to work on Windows

Because nobody who works on Meson uses Windows, or Mac for that matter, on a regular basis. If you want better Windows support, you need to step up. There are exactly 3 people who get paid to work on Meson that I know of, and that is only part of the time. None of the companies paying those people have a vested interest in Meson on Windows, or if they do, it works well enough for them.

if you want to configure the build directory from an IDE like VS Code

This extension is best effort, and is all volunteer work. Please provide more ideas on how this experience can be better.

one and only allowed use case

What is this use case?

wolfpld commented 6 months ago

I have linked a PR where @xclaesse explains the problems quite well and proposes a solution. In fact, there are feature requests aligned with this PR dating back to 2014, such as https://github.com/mesonbuild/meson/issues/9.

As far as I can see, the problem is not, as you conveniently suggest, that "nobody who works on Meson uses Windows" or " [it] is all volunteer work", but rather that there is someone who is able to stop things from moving forward if their only argument is "I don't like this".

tristan957 commented 6 months ago

Please, continue to call me a liar.

there is someone who is able to stop things from moving forward if their only argument is "I don't like this"

Accessing environment variables from Meson scripts is intentionally not supported and will not be added in the foreseeable future.

Environment variables are terrible for any sort of configuration because they are mutable global state. If your setup depends on environment variables being set, then running any sort of command that causes reconfiguration from a different terminal that does not have those envvars set (or from an IDE or any of a thousand of different options) breaks your setup in unobvious ways that are at worst incredibly difficult to debug.

Using envvars for configuration is a code smell. Don't use them whenever possible. Convert those to project options or something similar instead. It is the only reliable solution.

If all you get out of that entire comment is "I don't like it," I have to assume you didn't actually read the comment.

At the end of the day, I don't care whether you use Meson. No need to go on a monologue here in an unrelated PR, in an unrelated project.

eli-schwartz commented 6 months ago

Unfortunately, I won't be working on this anymore due to how things like mesonbuild/meson#5859 are handled. Meson is basically useless if you want to work on Windows, or if you want to configure the build directory from an IDE like VS Code, or really do anything outside of the one and only allowed use case.

I don't see why this has specifically to do with Windows. That being said, ever since https://github.com/mesonbuild/meson/commit/18b96cd0692255b30f8f0597cbf4af89d142a93d you can use cpp = '@DIRNAME@/bin/my-cxx-compiler' and have the cross file calculate all paths relative to the location of the cross file itself.

wolfpld commented 6 months ago

I don't see why this has specifically to do with Windows.

There is no portable way to pass a path to the vcpkg installation directory where the pkg-config binary is stored. Adding the pkg-config provided by vcpkg to the PATH is impractical because there are many triplets, e.g. x64-windows or x64-windows-static, which should be selectable on the project side.

Cross files are not generally available in various third-party SDKs. This requires the user to manually fiddle with how things are installed. Note that some SDKs may be installed in trees that are not writable by the user, and may have version numbers in their paths.