Disable fallback lsp server #2090
Conversation
The clojure-lsp fallback server does not work as expected on Windows machines. The function to start the fallback server has now been changed to return early until support for Windows can be safely added. Additionally the logic for `"always-use-first-workspace-root"` now always starts the lsp server in the first project root instead of starting the fallback server if the first project root is not a valid clojure project. As much as possible the code for working with the fallback server has been left along in order to facilitate re-enabling these code-paths once Windows support has been figured out. Closes BetterThanTomorrow#2088 Fixes BetterThanTomorrow#2087
Never saw that exception, but seems like the project-root is not absolute (missing the C:). |
@julienvincent I'm guessing that af1761b changed that. Good find. I think we've been bitten by this same issue (or something similar) elsewhere before, unfortunately. Has this been tested on Windows? By the way, I updated the PR checklist to cross out the unchecked boxes as I'm guessing you left them unchecked because they didn't apply. |
The early return I am referring to here is actually this: 9f61baf#diff-a744c42f2b8787edf067397d52c9ccd6f4f300323d1537b80b92027b3058d7dbR161 The changes from af1761b are fixing a separate issue which was resulting in an infinite loop on Windows (reported on slack).
@ericdallo In this case we are starting the lsp server with an absolute path (including the C:). I'm trying to figure out if this is an issue in clojure-lsp and I should raise an issue there/debug it there, or if this is maybe an issue higher up (like in the vscode languageclient lib for example)? What do you think? |
The api method for finding an LSP client for a given document URI was written to stop traversing when encountering the OS root. This logic assumed POSIX paths and did not work correctly on Windows platforms. This updates the clojure-lsp client ids to use the uri.fsPath which is platform agnostic and updates the root path comparison in the lsp api to compare in a platform agnostic manner.
@bpringe Yes, this was actually developed on a Windows VM :) And we have the original reporter testing these changes out as we go, which is very helpful. There is still one more issue reported (see slack thread) which I would like to catch before this can be merged. However I am not really able to reproduce it as of yet. |
julienvincent
force-pushed
the
regression/disable-tmp-lsp
branch
from
af1761b
to
4ac9237
Compare
The `Uri.parse` method is for parsing full URI's that include a 'scheme' component. Our usage was 'fine' for POSIX systems as it was non-strict and would fallback to a `file://` scheme however on Windows it would interpret the leading `C:\` as the scheme. This commit replaces all our usages of `parse` with `file` where we are using known file paths to remove this ambiguity.
Ok this should now be fixed in 5877356 - just waiting on confirmation from the user. |
|
Thanks for providing, @julienvincent! |
|
Cool, @PEZ I think this is good to go unless you can think of anything left to do? |
|
Releasing now! 🚀 |
Ah, whoops. I had seen that before but I think I was looking at a single commit when I made that comment. Anyway, good stuff here! |
The clojure-lsp fallback server does not seem to work as expected on Windows machines. It's a bit non-deterministic to test as I was sometimes able to get it to work and sometimes not. There definitely seems to be some behavioural differences though.
The function to start the fallback server has now been changed to return early until support for Windows can be safely added.
Additionally the logic for
"always-use-first-workspace-root"now always starts the lsp server in the first project root instead of starting the fallback server if the first project root is not a valid clojure project.As much as possible the code for working with the fallback server has been left alone in order to facilitate re-enabling these code-paths once Windows support has been figured out.
The issue reported in #2087 talks more about being unable to start the REPL. This I was not able to reproduce at all however my intuition tells me that this was a red herring caused by the Calva extension not booting/initialising (as a result of the fallback server not booting).
The issue with starting the clojure-lsp server in a temp dir seems to be an upstream issue in the clojure-lsp project itself and seems to have something to do with Java path comparisons. See logs:
Server Logs
With
java.lang.IllegalArgumentException: 'other' is different type of Pathseeming to be produced when two paths don't share the same root. My guess is that this has something to do with the omission/inclusion ofC:\in some of the paths (see below excerpt) though I don't really know. Maybe @ericdallo can add some insight here?Closes #2088
Fixes #2087
Checklist
devbranch. (Or have specific reasons to target some other branch.)published. (Sorry for the nagging.)[Unreleased]entry inCHANGELOG.md, linking the issue(s) that the PR is addressing.[ ] Added to or updated docs in this branch, if appropriate[ ] Figured if the change might have some side effects and tested those as well.[ ] If I am fixing just part of the issue, I have just referenced it w/o any of the "fixes” keywords.[ ] Created the issue I am fixing/addressing, if it was not present.npm run prettier-format)npm run eslintbefore creating your PR, or runnpm run eslint-watchto eslint as you go).