Add support for multi-root workspaces #810
Conversation
These changes add proper support for multi root workspaces with a clangd server per workspace folder. On a high level, we have a single language client per folder, and have to rework things so that various global objects (views, ...) are actually global and not instantiated per client anymore. The important use case this enables is where clangd is invoked within a container per process with all the dependencies installed in the container. To make this use case work in a multi-root setup, a clangd instance per workspace folder has to be spawned. Fixes clangd#38
|
This currently doesn't work because the second client tries to register microsoft/vscode-languageserver-node#607 and microsoft/vscode-languageserver-node#333 discuss this issue with microsoft/vscode-languageserver-node#333 suggesting to use random UUIDs instead of hardcoded command names but I'm not sure how this would work for clangd. |
| public getApi(version: number): unknown { | ||
| if (version === 1) { | ||
| return {languageClient: this.client}; | ||
| return {languageClient: this.context.clients.entries().next().value?.[1]}; |
There was a problem hiding this comment.
Should we print out a warning when API1 is used in combination with multiple roots?
|
|
||
| // Map a root ASTNode onto a VSCode tree. | ||
| class TreeAdapter implements vscode.TreeDataProvider<ASTNode> { | ||
| export class ASTProvider implements vscode.TreeDataProvider<ASTNode> { |
There was a problem hiding this comment.
What do these changes have to do with Multi-Root? I seem to be missing some context
| }); | ||
|
|
||
| client.start(); | ||
| console.log('Clang Language Server is now active!'); |
There was a problem hiding this comment.
Can you update this message to include the working directory? That way, it is clear to users that are less familiar with the internals of the extension why multiple LSPs are active.
| } | ||
|
|
||
| const client = clangdContext.getActiveClient(); | ||
| if (client === undefined) { |
There was a problem hiding this comment.
If your clangd crashed (or was explicitly killed by the user), the restart command used to start it again. Is this still the case?
| } | ||
|
|
||
| let folder = vscode.workspace.getWorkspaceFolder(document.uri); | ||
| if (folder === undefined) { |
There was a problem hiding this comment.
If you are in your file and do a go-to include of the standard library, will clangd still work on those headers?
| } | ||
|
|
||
| class TreeAdapter implements vscode.TreeDataProvider<InternalTree> { | ||
| export class MemoryUsageProvider implements vscode.TreeDataProvider<InternalTree> { |
There was a problem hiding this comment.
I'm wondering if making this feature workspace specific is the right thing to do. My first reaction would be to expect a tree node per worktree, such that you can get a global overview. What do you think?
|
Given the impact of this change, can you change the version number to 0.2.1 (or another patch number if that's already used) such that it triggers a pre-release version? |
|
have plan to merge this PR？ it's an useful function |
|
It does not work yet. Changes have to be made to clangd to make this possible at all. |
|
A couple of thoughts / questions:
|
4 participants
These changes add proper support for multi root workspaces with a clangd server per workspace folder.
On a high level, we have a single language client per folder, and have to rework things so that various global objects (views, ...) are actually global and not instantiated per client anymore.
The important use case this enables is where clangd is invoked within a container per process with all the dependencies installed in the container. To make this use case work in a multi-root setup, a clangd instance per workspace folder has to be spawned.
Fixes #38