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

Setup compile commands directory for the clangd extension #210

Closed
wants to merge 1 commit into from
Closed

Setup compile commands directory for the clangd extension #210

wants to merge 1 commit into from

Conversation

wolfpld
Copy link
Contributor

@wolfpld wolfpld commented Nov 29, 2023

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 clangd/vscode-clangd#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 clangd/vscode-clangd#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
Copy link
Contributor Author

wolfpld commented Nov 29, 2023

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.

src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated
Comment on lines 140 to 141
const conf2 = vscode.workspace.getConfiguration("clangd");
conf2.update("arguments", [`--compile-commands-dir=${buildDir}`], vscode.ConfigurationTarget.Workspace);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you have clangd.arguments defined in your user settings and then this runs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workspace settings seem to override the user settings. I have both defined, but only the workspace provided argument is present in the ps aux output.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The downside is we override user settings that could be defining other stuff than the compile commands. Maybe we should wait for clangd to add a dedicated setting for this?

BTW, if someone knows a better trick than a workspace setting for communicating the path to compile_commands.json to other extensions, that would be great. 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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should wait for clangd to add a dedicated setting for this?

The PR for vscode-clangd is currently more than a month old, with zero comments so far. And it's not even that complicated... Currently I can't see it moving forward in a reasonable timeframe.

BTW, if someone knows a better trick than a workspace setting for communicating the path to compile_commands.json to other extensions, that would be great.

In theory, extensions should be able to expose their internal API to other extensions by returning data from the activation function.

https://code.visualstudio.com/api/references/vscode-api#extensions

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...

True.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compile_commands.json to other extensions, that would be great. 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...

I actually hate this about the Meson extension. For instance, I have "C_Cpp.intelliSenseEngine": "disabled",, so I don't even use the compilation database for the extension. We should check this setting to not edit the file.

I've also thought about just adding a setting to this extension whether or not to auto setup other extensions. It is really freaking annoying!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also thought about just adding a setting to this extension whether or not to auto setup other extensions. It is really freaking annoying!

I made this configurable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wolfpld I think we could be smarter here. If we can read the current setting, we can then check if the compile commands dir is already set. If not set, extend the current setting with the arguments to set the compile commands dir.

Something like this?

      let args = undefined;
      const wanted = `--compile-commands-dir=${buildDir}`;
      const current = conf.get("clangd.arguments");
      if (current && Array.isArray(current)) {
        // if wanted is already in current, leave args undefined to prevent a config update
        if (!current.includes(wanted)) {
          const found = current.findIndex((arg) => arg.startsWith("--compile-commands-dir="));
          if (found !== -1) {
            current[found] = wanted;
            args = current;
          } else {
            args = [...current, wanted];
          }
        }
      } else {
        args = [wanted];
      }

      if (args) {
        conf
          .update("clangd.arguments", args, vscode.ConfigurationTarget.Workspace)
          .then(
            () => vscode.commands.executeCommand("clangd.restart"),
            () => {},
          );
      }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps. I would need to test with my setup.

  // Clangd
  "clangd.arguments": [
    "--all-scopes-completion=false",
    "--clang-tidy",
    "--completion-style=bundled",
    "--enable-config",
    "--header-insertion=never",
    "--header-insertion-decorators",
    "--pch-storage=disk",
  ],

FWIW, the clangd extension seems to do a recursive search for compilation databases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, the clangd extension seems to do a recursive search for compilation databases.

It won't pick up the database if I don't change the default builddir to build in the meson extension (without this PR).

I also had some very unfun experience trying to figure out why the include path changes I made in meson.build were not picked up by clangd, while the project compiled just fine. It turned out that I had two build dirs, build and builddir. Clangd was reading build while the meson extension was updating builddir.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps. I would need to test with my setup.

Ok, I pushed it. Note that the clangd.arguments getter is not scoped to the workspace, so it will pick up user settings if any are set, and the workspace settings are empty.

src/extension.ts Outdated Show resolved Hide resolved
package.json Outdated
"type": "boolean",
"default": true,
"description": "Setup the rust-analyzer extension to work with Meson."
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we need a specific setting for each of them. What about a single boolean to disable modifying user settings at all? My use-case here is for projects that commit .vscode/settings.json into git, they don't want meson extension to make a diff in that file all the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thoughts were that you may want to have both the C++ and clangd extensions installed, but you may want to use only one of them for a particular project. There are also other considerations, for example, you may be fine with updating the C++ extension configuration, which goes through a dedicated setting, but you may not be fine with modifying the clangd arguments list, which is passed to an external executable.

I don't have a strong preference for one option or the other, but with the changes @tristan957 made, we explicitly tell users that we have support for this and that extension. This would go away if there was only one option to choose from.

What are your thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See latest master. We offer a boolean value or an array so all good!

@tristan957
Copy link
Contributor

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

@wolfpld
Copy link
Contributor Author

wolfpld commented Dec 6, 2023

Rebased.

@tristan957
Copy link
Contributor

Sorry for forcing another rebase on you :(

@wolfpld
Copy link
Contributor Author

wolfpld commented Dec 6, 2023

Rebased.

@tristan957
Copy link
Contributor

Can you squash the commits?

@wolfpld
Copy link
Contributor Author

wolfpld commented Dec 6, 2023

Done.

@wolfpld
Copy link
Contributor Author

wolfpld commented Dec 7, 2023

Are there any remaining issues that would block the merge?

@tristan957
Copy link
Contributor

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

@wolfpld
Copy link
Contributor Author

wolfpld commented Dec 13, 2023

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 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.

@tristan957
Copy link
Contributor

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?

@tristan957 tristan957 closed this Dec 13, 2023
@wolfpld
Copy link
Contributor Author

wolfpld commented Dec 13, 2023

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 mesonbuild/meson#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
Copy link
Contributor

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
Copy link
Member

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 mesonbuild/meson@18b96cd 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
Copy link
Contributor Author

wolfpld commented Dec 13, 2023

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.

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

Successfully merging this pull request may close these issues.

4 participants