LSP support for multi-project and multi-workspace #2020

julienvincent · 2023-01-12T15:24:01Z

Right now Calva only supports opening clojure projects from the root of the project. This will be where the LSP process is started and the clojure LSP needs a deps.edn/project.clj file to exist at it's root in order to provide proper completions.

This also means that multi-workspace setups in VSCode is not really supported.

This commit refactors the LSP implementation to allow provisioning multiple LSP clients at different roots. Dependent systems can then request the LSP client governing a particular file/directory.

This change simultaneously allows opening directories that contain multiple projects as well as setting up multi-workspace vscode configurations.

The LSP initialization logic is also altered such that the server is started when a clojure file is opened rather than when Calva initializes. This prevents poluting non-clojure projects with .lsp and .clj-kondo directories.

UX

Commands

The LSP lifecycle commands have been reworked to make a bit more sense within the new architecture:

The start command now list all discovered clojure project roots in the workspace, allowing the user to pick where they want to start an LSP client.

This is especially useful in mono-repos where a user might want to pick if they want to start the LSP at the project root or within a specific sub-package (or maybe a combination of both).

The restart/stop commands now list the directories mapping to running LSP clients and allows the user to select the client they want to restart/stop.

Status Bar and Menu

The lsp status bar item now tracks the currently active editor. As the user changes the active window the status bar will show the status of the lsp governing the file they have open.

When the user clicks the status bar item we now show a single type of LSP menu instead of dynamically changing it based on the status of the client. This is because there are now multiple LSP clients running.

Checklist

src/lsp/client/client.ts

julienvincent · 2023-01-13T17:23:37Z

FYI I will be rebasing all these commits back together once approved before merging.

PEZ · 2023-01-14T17:45:31Z

Thanks for sharing of your work and time with Calva! I love the changes!

Some initial thoughts after having tried it very briefly with the test-data workspace which has several example projects in projects. clojure-lsp starts at activation with:

"project-root-uri": "file:///Users/pez/Projects/calva/test-data"

Which I think is fine. There's a deps.edn there.

Opening projects/pirate-lang/src/pez/pirate_lang.clj and then choosing to start a project, I get this menu:

Comments on this menu:

Pre-select: It's a bit hard to scan the menu to find the pirate-lang project. It would be good if it was pre-selected. Like it is with the Jack-in/connect menu. (Actually, I think maybe this is more of something quirky around what is started and not. Sometimes things are pre-selected, sometimes not...)
Project type:I like that this menu is showing the project type, but I think it is better it just shows the project files found there. It could be several, and we don't know which one determines the project type.

General Ux comments:

A running servers lists: It's a bit tricky to understand what servers are running. I can pull up a list using the Stop the Clojure LSP server and deduce it that way. I think a menu showing all projects, indicating if they have a server running or not, might make sense.
A servers control menu: Having fiddled around a bit more I am starting to think it will get tricky to keep track of all possible states with the current approach with separate start/stop/restart commands. Both for us and for the user. How about we have one command named something like Clojure LSP servers? This list would have buttons for
- starting any server that is not running
- info, stopping, restarting any server that is running
Status bar item: Status bar real estate is precious. I think we should stick with just showing the status icon and clojure-lsp. The tooltip shows the status and the users can learn what the indicator icon means that way.

I think I found some bugs:

Stop command: When no servers are running there is still a Stop item in the clojure-lsp menu, as well as a Stop the Clojure LSP server command in the command palette.
Server info:
- Trying to get server info for the pirate-lang server when it is runnin gives an error that there is no server running for the current opened file, even though I have a file for that project opened.
- When no servers are running there is still a Server info command item in the clojure-lsp menu, as well as a Calva Diagnostics: Clojure LSP Server info* command in the command palette.
I found some more quirks that I can't reproduce or describe now. But maybe we should first decide around the general Ux approach before trying to fix the current one.

src/extension-test/integration/suite/jack-in-test.ts

PEZ · 2023-01-14T17:51:17Z

I'll look more at this, and the code changes later tonight.

julienvincent · 2023-01-14T18:28:04Z

Opening projects/pirate-lang/src/pez/pirate_lang.clj and then choosing to start a project, I get this menu:

Two things on this point:

The "start lsp server" menu will only list projects where there are no LSP servers currently running.
There was a bug with the preselection logic which was a result of it trying to preselect a directory with an already running lsp, which was then filtered out the list later. Fixed in f9ee58b.

A running servers lists: It's a bit tricky to understand what servers are running.

Yea this does make sense, I can add this.

Trying to get server info for the pirate-lang server when it is runnin gives an error that there is no server running for the current opened file, even though I have a file for that project opened.

Thanks, fixed in e29c53c

Status bar real estate is precious. I think we should stick with just showing the status icon and clojure-lsp. The tooltip shows the status and the users can learn what the indicator icon means that way.

Ok, changed in f197bc4

PEZ · 2023-01-14T21:28:56Z

The clojure-lsp statusbar item disappears when some view that is not in the project is focused. Like the an Output channel.

PEZ · 2023-01-14T21:46:38Z

When I stop the default/repo root server and then open some file, the server starts again.
Sometimes (I don't know how to repro it reliably) the statusbar item does not update automatically when I stop the last running server, or start the first. I need to trigger an update, like clicking in an Output channel.
Stopping the default server sets the status item to disconnected, even if there are other servers running. On some later update it shows the status as active/running.
The show server info command does not let me choose which server to show info for.
I can't get server-info for the pirate-lang project's server. The other projects and the default server works.
- This might not be new, come to think of it...
  - Indeed, if I use dev and open pirate-lang as a workspace it also doesn't work. So disregard this one. 😄

PEZ · 2023-01-14T22:52:17Z

Single-project workspaces:

I can't Jack-in, getting an error: The "path" argument must be of type string. Received undefined (Both deps and Leiningen projects)
Stopping the server I get a menu to select which project. It's probably fine, but I think might be a bit weird for many when it starts happening. For Jack-in we skip this menu if there is only one project root.
Stopping the server and then starting it again doesn't work (nothing happens)

PEZ · 2023-01-14T22:59:33Z

I guess this tells us we need an integration test for single-project workspaces. Not sure how to do that, though, maybe we need two different runs.

package.json

bpringe · 2023-01-15T21:54:55Z

@julienvincent Thanks for working on this. I started to review but figured I'd wait until some of the feedback @PEZ provided is addressed first.

bpringe · 2023-01-15T22:13:33Z

@julienvincent In the future, please leave the relevant checklist from the PR template in the PR description. It helps us make sure everything we want to check/do per PR is checked/done.

julienvincent · 2023-01-16T11:31:52Z

In the future, please leave the relevant checklist from the PR template in the PR description. It helps us make sure everything we want to check/do per PR is checked/done.

Sure, I have re-added it now.

julienvincent · 2023-01-16T12:12:56Z

When I stop the default/repo root server and then open some file, the server starts again

This is expected as the calva.enableClojureLspOnStart is probably enabled. The logic now boots the LSP when clojure files are opened.

Stopping the default server sets the status item to disconnected, even if there are other servers running. On some later update it shows the status as active/running.

Resolved in a94457b

Stopping the server I get a menu to select which project. It's probably fine, but I think might be a bit weird for many when it starts happening. For Jack-in we skip this menu if there is only one project root.

Resolved in 947c56b

I can't Jack-in, getting an error: The "path" argument must be of type string. Received undefined (Both deps and Leiningen projects)

I can't reproduce this - what exactly are you doing to trigger? And in which project?

Stopping the server and then starting it again doesn't work (nothing happens)

Also can't reproduce this, need some more info

The show server info command does not let me choose which server to show info for.

Resolved in fa2d38f

PEZ · 2023-01-16T12:29:45Z

When I stop the default/repo root server and then open some file, the server starts again

This is expected as the calva.enableClojureLspOnStart is probably enabled. The logic now boots the LSP when clojure files are opened.

Expected, maybe, but surprising. 😄 If I stop it, I expect it to stay stopped. I think we need a setting like calva.enableClojureLspOnFileOpen which is disabled by default. Or, remove the behaviour, and let people start sub-servers manually.

I can't Jack-in, getting an error: The "path" argument must be of type string. Received undefined (Both deps and Leiningen projects)

I can't reproduce this - what exactly are you doing to trigger? And in which project?

It's in single-project repos. Opening any project from test-data/projects in a Window of their own reproduces the issue for me. (Or did before, I haven't tried with the latest commits.)

Stopping the server and then starting it again doesn't work (nothing happens)

Also can't reproduce this, need some more info

Again, single project workspaces. Did you catch that heading?

julienvincent · 2023-01-16T12:36:09Z

Again, single project workspaces. Did you catch that heading?

Yes I tried in a single project workspace - it works on my machine haha

It's in single-project repos. Opening any project from test-data/projects in a Window of their own reproduces the issue for me. (Or did before, I haven't tried with the latest commits.)

I really can't reproduce this (before or after latest commits).

julienvincent · 2023-01-16T12:42:40Z

Expected, maybe, but surprising. 😄 If I stop it, I expect it to stay stopped. I think we need a setting like calva.enableClojureLspOnFileOpen which is disabled by default. Or, remove the behaviour, and let people start sub-servers manually.

So with the new architecture there isn't really a concept of a sub-server - all servers are treated the same and I think this is a good thing. It makes sense to me to rename the setting as you say - although I would leave it on by default as that closest matches the existing Calva behaviour.

julienvincent · 2023-01-16T12:51:05Z

A servers control menu: Having fiddled around a bit more I am starting to think it will get tricky to keep track of all possible states with the current approach with separate start/stop/restart commands. Both for us and for the user. How about we have one command named something like Clojure LSP servers? This list would have buttons for

starting any server that is not running

info, stopping, restarting any server that is running

I added something to this effect now in 995b865. Instead of buttons I used a two phase selection menu which I think is more accessible to keyboard users.

Give it a try let me know what you think.

PEZ · 2023-01-16T12:53:20Z

All servers are not treated equally on startup. When I open test-data as a folder, the server for test-data is started, even if I have a file from a sub-folder open at startup. Maybe we shouldn't. Maybe that in multi-project repos, calva.enableClojureLspOnStart should start whatever servers where running when the workspace was last open. And then no server starts automatically on file open.

julienvincent · 2023-01-16T13:02:40Z

All servers are not treated equally on startup. When I open test-data as a folder, the server for test-data is started, even if I have a file from a sub-folder open at startup.

This is the same behaviour when opening a file after startup. The current auto-start logic works by finding the root most valid clojure project. So even if opening a subdirectory it should still start the server at the root (if it is a valid project).

I thought this made the most sense as a default behaviour for automatic starting as it feels like the most desirable action. But I'm not too sure, maybe it needs to be more configurable

Maybe that in multi-project repos, calva.enableClojureLspOnStart should start whatever servers where running when the workspace was last open. And then no server starts automatically on file open

Interesting.. this might be a nice behaviour. How would we keep track?

PEZ · 2023-01-16T16:31:04Z

Interesting.. this might be a nice behaviour. How would we keep track?

There is global storage where we can save things keyed on workspace. But I guess the question isn't about what is used to keep track, but rather how we make sure the right state is saved? ... Tricky to get right, probably... Maybe we can do something less magic, and let the user choose which servers should autostart?

PEZ · 2023-01-16T16:35:08Z

We could consider waiting with this, and either:

See what feedback we get on the current behaviour
Remove the start-on-file-open and let calva.enableClojureLspOnStart continue to autostart the same server it would autostart before this PR.

julienvincent · 2023-01-16T16:49:48Z

I tracked down the flaky status-bar reporting and fixed it in 6c66e7e

julienvincent · 2023-01-16T16:56:59Z

Personally I think we should wait on it and see what feedback we get. I think the current behaviour is pretty close to how Calva works now in most cases.

I think the only noticeable difference in behaviour for single workspaces projects is that the lsp server will auto-start itself even if it has been stopped (but only if a new files is opened). I don't think this will have too much of an impact.

The main differences lie in the multi-workspace/project workflows which aren't currently supported (and so unlikely anyone is using Calva like this)

Remove the start-on-file-open and let calva.enableClojureLspOnStart continue to autostart the same server it would autostart before this PR.

The issue with this approach is that we would be back to not supporting multi-workspace/multi-project as the server Calva was starting was not correctly located.

We could try locate all projects and start a server there but I think this would be a bad idea as we would potentially be spawning a lot of processes that aren't used.

julienvincent · 2023-01-16T16:59:34Z

Maybe that in multi-project repos, calva.enableClojureLspOnStart should start whatever servers where running when the workspace was last open

We could potentially detect where servers were started previously by searching for .lsp folder locations.

I think though that maybe better to do this type of behaviour optimisation in a follow up project.

PEZ · 2023-01-24T13:14:51Z

But all these intermediary commits (!fixup, !amend for example) are directives to be rebased back into their referenced commit.

I see. I've been wondering about those. 😄 Missed the first memo. For the record, I'd prefer just regular commits telling what they contain, rather than directives. So that I can read the conversation thread here easily. And we seldom do much clean-up of the commit history. We just merge it all in, as is.

Right now Calva only supports opening clojure projects from the root of the project. This will be where the LSP process is started and the clojure LSP needs a deps.edn/project.clj file to exist at it's root in order to provide proper completions. This also means that multi-workspace setups in VSCode is not really supported. This commit refactors the LSP implementation to allow provisioning multiple LSP clients at different roots. Dependent systems can then request the LSP client governing a particular file/directory. This change simultaneously allows opening directories that contain multiple projects as well as setting up multi-workspace vscode configurations. The LSP initialization logic is also altered such that the server is started when a clojure file is opened rather than when Calva initializes. This prevents poluting non-clojure projects with .lsp and .clj-kondo directories. Closes BetterThanTomorrow#934 Closes BetterThanTomorrow#1706

This commit updates the `enableClojureLspOnStart` setting to allow specifying different auto-start behaviours. The setting now accepts the values: - "when-workspace-opened-use-workspace-root" - "when-file-opened-use-furthest-project" - "never" - true - false And has slightly different behaviours in each case. The value `true` is now an alias for `"when-workspace-opened-use-workspace-root"`, and `false` for `"never"`. When set to `"when-workspace-opened-use-workspace-root"` the clojure-lsp will be automatically started in the root of all open/added workspaces when calva initializes and whenever a workspace is added to vscode. When set to `"when-file-opened-use-furthest-project"` then the clojure-lsp will be automatically started whenever a clojure file is opened. The server will be started in the furthest possible valid clojure project or will fall back to starting in the workspace root if no valid clojure project is found. When set to `"never"` the clojure-lsp will never automatically be started and is instead left up to the user to explicitly start it in a directory.

julienvincent · 2023-01-24T13:38:56Z

For the record, I'd prefer just regular commits telling what they contain, rather than directives. So that I can read the conversation thread here easily

The conversation thread should still read the same - the intermediary commits referenced in comments are still viewable.

julienvincent · 2023-01-24T13:39:35Z

I've rebased - unless there are any further concerns I think this is ready to go :) will be giving it a final test over today.

PEZ · 2023-01-24T13:47:17Z

The conversation thread should still read the same - the intermediary commits referenced in comments are still viewable.

Well, I've been seeing !fixup ... and wondered what was committed, so not really reading the same.

PEZ · 2023-01-24T13:47:56Z

And we generally try to avoid force pushes, just FYI. 😄

PEZ · 2023-01-26T06:59:05Z

Here we go! 🚀 Thanks for this hard and most excellent work, @julienvincent! 🙏 ❤️ I enjoyed it, and learnt a lot in the process.

This resolves a regression from BetterThanTomorrow#2020 which prevents certain vscode operations such as organise-imports and add-missing-import from functioning correctly. This was a result of disabling the automatic command registrations being performed by the lsp client. Originally it was assumed that these commands were unused, however they are used internally by vscode to handle their native organise-imports and related operations. This is now fixed by manually registering the missing commands with the added functionality of multi-client support. Addresses BetterThanTomorrow#2041 Fixes BetterThanTomorrow#2040

The clojure-lsp refactor project in BetterThanTomorrow#2020 reworked the way the project picker menus work, adding additional information on a second line indicating which files contributed to a folder being picked as a valid clojure project. This turned out significant hurt the UX experience in workspaces with many clojure projects. This commit removes the additional information from the picker menus, shrinking the list items to a single line. Some other smaller changes included: - Projects are now grouped by their workspace root which should help make it easier to sort through the list visually. - Project folders are now sorted within their groups to make the list more deterministic. - Project folders show the path relative to the workspace root instead of the absolute path. This should help make scanning the list easier by removing unnecessary/duplicate information. Addresses BetterThanTomorrow#2041 Closes BetterThanTomorrow#2043

This resolves a regression from BetterThanTomorrow#2020 which prevents certain vscode operations such as organise-imports and add-missing-import from functioning correctly. This was a result of disabling the automatic command registrations being performed by the lsp client. Originally it was assumed that these commands were unused, however they are used internally by vscode to handle their native organise-imports and related operations. This is now fixed by manually registering the missing commands with the added functionality of multi-client support. Addresses BetterThanTomorrow#2041 Fixes BetterThanTomorrow#2040

The clojure-lsp refactor project in BetterThanTomorrow#2020 reworked the way the project picker menus work, adding additional information on a second line indicating which files contributed to a folder being picked as a valid clojure project. This turned out significant hurt the UX experience in workspaces with many clojure projects. This commit removes the additional information from the picker menus, shrinking the list items to a single line. Some other smaller changes included: - Projects are now grouped by their workspace root which should help make it easier to sort through the list visually. - Project folders are now sorted within their groups to make the list more deterministic. - Project folders show the path relative to the workspace root instead of the absolute path. This should help make scanning the list easier by removing unnecessary/duplicate information. Addresses BetterThanTomorrow#2041 Closes BetterThanTomorrow#2043

This resolves a regression from BetterThanTomorrow#2020 which prevents certain vscode operations such as organise-imports and add-missing-import from functioning correctly. This was a result of disabling the automatic command registrations being performed by the lsp client. Originally it was assumed that these commands were unused, however they are used internally by vscode to handle their native organise-imports and related operations. This is now fixed by manually registering the missing commands with the added functionality of multi-client support. Addresses BetterThanTomorrow#2041 Fixes BetterThanTomorrow#2040

The clojure-lsp refactor project in BetterThanTomorrow#2020 reworked the way the project picker menus work, adding additional information on a second line indicating which files contributed to a folder being picked as a valid clojure project. This turned out significant hurt the UX experience in workspaces with many clojure projects. This commit removes the additional information from the picker menus, shrinking the list items to a single line. Some other smaller changes included: - Projects are now grouped by their workspace root which should help make it easier to sort through the list visually. - Project folders are now sorted within their groups to make the list more deterministic. - Project folders show the path relative to the workspace root instead of the absolute path. This should help make scanning the list easier by removing unnecessary/duplicate information. Addresses BetterThanTomorrow#2041 Closes BetterThanTomorrow#2043

Fix clojure-lsp regressions from #2020

A regression introduced in BetterThanTomorrow#2020 and BetterThanTomorrow#2046 prevented the clojure-lsp and repl jack-in operations from working correctly on Windows hosts. This fixes the way we handle Uri's to make the unix and windows behaviours consistent. Fixes BetterThanTomorrow#2054

julienvincent force-pushed the feature/multi-project-support branch from 2a96b9c to 00f555b Compare January 12, 2023 17:11

julienvincent marked this pull request as ready for review January 13, 2023 16:00

julienvincent changed the title ~~LSP support for multi-project and multi-workspace [WIP]~~ LSP support for multi-project and multi-workspace Jan 13, 2023

julienvincent commented Jan 13, 2023

View reviewed changes

src/lsp/client/client.ts Show resolved Hide resolved

julienvincent force-pushed the feature/multi-project-support branch from 3b93fef to 248ad06 Compare January 14, 2023 14:01

PEZ reviewed Jan 14, 2023

View reviewed changes

src/extension-test/integration/suite/jack-in-test.ts Show resolved Hide resolved

bpringe reviewed Jan 15, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

julienvincent added 8 commits January 24, 2023 13:21

Add new clojure-lsp management menu

2776e1a

Update clojure-lsp related documentation

e1914f4

Fix jack-in init relying on undefined behaviour

ab68472

Disable built-in lsp statusbar icon

fa22931

Update CHANGELOG.md

0201085

Add clojure.tmLanguage.json to .prettierignore

87ab72c

julienvincent force-pushed the feature/multi-project-support branch from ec782ad to 87ab72c Compare January 24, 2023 13:35

PEZ merged commit 68dabd9 into BetterThanTomorrow:dev Jan 26, 2023

julienvincent deleted the feature/multi-project-support branch January 26, 2023 13:12

julienvincent mentioned this pull request Jan 30, 2023

Fix clojure-lsp regressions from #2020 #2046

Merged

17 tasks

PEZ added a commit that referenced this pull request Feb 1, 2023

Merge pull request #2046 from julienvincent/fix/lsp-regressions

50f677b

Fix clojure-lsp regressions from #2020

julienvincent mentioned this pull request Feb 2, 2023

Fix handling of project paths on Windows hosts #2060

Merged

17 tasks

julienvincent mentioned this pull request Feb 15, 2023

Rollback clojure-lsp auto-start behaviour default #2077

Merged

15 tasks

PEZ mentioned this pull request Apr 23, 2023

"Resolve macro as" menu buttons are unreadable #2156

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LSP support for multi-project and multi-workspace #2020

LSP support for multi-project and multi-workspace #2020

julienvincent commented Jan 12, 2023 •

edited

Loading

julienvincent commented Jan 13, 2023

PEZ commented Jan 14, 2023

PEZ commented Jan 14, 2023

julienvincent commented Jan 14, 2023

PEZ commented Jan 14, 2023

PEZ commented Jan 14, 2023

PEZ commented Jan 14, 2023

PEZ commented Jan 14, 2023

bpringe commented Jan 15, 2023

bpringe commented Jan 15, 2023 •

edited

Loading

julienvincent commented Jan 16, 2023

julienvincent commented Jan 16, 2023

PEZ commented Jan 16, 2023

julienvincent commented Jan 16, 2023

julienvincent commented Jan 16, 2023

julienvincent commented Jan 16, 2023

PEZ commented Jan 16, 2023

julienvincent commented Jan 16, 2023 •

edited

Loading

PEZ commented Jan 16, 2023

PEZ commented Jan 16, 2023

julienvincent commented Jan 16, 2023

julienvincent commented Jan 16, 2023

julienvincent commented Jan 16, 2023

PEZ commented Jan 24, 2023

julienvincent commented Jan 24, 2023 •

edited

Loading

julienvincent commented Jan 24, 2023

PEZ commented Jan 24, 2023

PEZ commented Jan 24, 2023 •

edited

Loading

PEZ commented Jan 26, 2023

LSP support for multi-project and multi-workspace #2020

LSP support for multi-project and multi-workspace #2020

Conversation

julienvincent commented Jan 12, 2023 • edited Loading

UX

Commands

Status Bar and Menu

Checklist

julienvincent commented Jan 13, 2023

PEZ commented Jan 14, 2023

PEZ commented Jan 14, 2023

julienvincent commented Jan 14, 2023

PEZ commented Jan 14, 2023

PEZ commented Jan 14, 2023

PEZ commented Jan 14, 2023

PEZ commented Jan 14, 2023

bpringe commented Jan 15, 2023

bpringe commented Jan 15, 2023 • edited Loading

julienvincent commented Jan 16, 2023

julienvincent commented Jan 16, 2023

PEZ commented Jan 16, 2023

julienvincent commented Jan 16, 2023

julienvincent commented Jan 16, 2023

julienvincent commented Jan 16, 2023

PEZ commented Jan 16, 2023

julienvincent commented Jan 16, 2023 • edited Loading

PEZ commented Jan 16, 2023

PEZ commented Jan 16, 2023

julienvincent commented Jan 16, 2023

julienvincent commented Jan 16, 2023

julienvincent commented Jan 16, 2023

PEZ commented Jan 24, 2023

julienvincent commented Jan 24, 2023 • edited Loading

julienvincent commented Jan 24, 2023

PEZ commented Jan 24, 2023

PEZ commented Jan 24, 2023 • edited Loading

PEZ commented Jan 26, 2023

julienvincent commented Jan 12, 2023 •

edited

Loading

bpringe commented Jan 15, 2023 •

edited

Loading

julienvincent commented Jan 16, 2023 •

edited

Loading

julienvincent commented Jan 24, 2023 •

edited

Loading

PEZ commented Jan 24, 2023 •

edited

Loading