fix!: remove hacky pipe start and prefer stdio #120
Conversation
|
We might have some spawn timing issues But hopefully the server should spawn faster without having to generate the pipe separately first I will do some testing before and we can do synchronized merges |
|
Sounds great! |
|
Can't we start using lspconfig now that the starting mechanism is standard? This plugin could turn into something akin to https://github.com/Hoffs/omnisharp-extended-lsp.nvim that would support non-standard protocols. |
|
That is a possibility, but I am a bit unsure if that will be good enough, because finding the root dir and actually attaching to a solution/project is not as easy as lspconfig would like it to be I think. But maybe that can just be overriden here or something🤔 I didn't think of that yet |
|
@seblj I've tried out my changes in dotnet/roslyn#76437 on this branch, everything appears to work, abit diagnostics are very laggy like #93 (comment) when working both on the entire dotnet/roslyn solution and the LanguageServer project itself. Do you want to give it a shot yourself too? |
|
I can try, but do you notice a difference between this PR and connecting with a pipe? That seems weird |
|
I mean, it is definitely not the fastest especially when working with the entire solution, but I don't notice any difference between this PR and master🤷♂️ It also gets significantly faster for me when only attaching to the project. I see I don't really have a good way to only attach to a project now that is a part of a solution, but that is potentially something for another PR. I just modified the code directly to make it attach to the project |
|
Nah mate, I was just incredibly tired when testing I think, was running a debug build of the ls among other silly config mistakes. The Language Server project still takes a while to startup, but that is just it the language server itself being generally slow to boot I think. |
|
Yup that is true |
seblj
force-pushed
the
bye-hack
branch
3 times, most recently
from
e0cc4f5 to
e5b7b62
Compare
|
Looks like dotnet/roslyn#76437 is merged and latest artifact already later than that commit. So it should be available now with the latest build, right? |
|
Yes I have been subscribed to that and noticed that. I will probably coordinate with @tris203 soon to agree on how we should go ahead with this. I was actually also thinking about parsing the version once and notify the users with some message that they need to install the latest version of roslyn instead of just silently failing. Otherwise I guess I would get issues about it not working I have actually been daily driving this branch since the PR was created, and if someone else wants to do it, feel free to do so. I will keep this branch updated with potential changes in the main branch. Hopefully I will merge this sometime next week |
|
I will do some testing with rzls We may need to wait for the new Roslyn version to make it into Vscode as the nix packaging relys on that |
|
Yeah that is fine by me👍 |
|
Im just working on this now from the rzls side. Where we emit the event for Roslyn initialised. Could we also set a global variable? |
|
Yes sure! Feel free to either create a PR to that branch or just send a patch here and I will apply when I have time |
|
I'm not sure the latest release in the crashdummyy repo has the `--stdio``` flag I can't seem to get it to work. Will try again after dinner https://github.com/Crashdummyy/roslynLanguageServer/releases/tag/4.14.0-1.25060.6 EDIT: scrap all of that, its breaking change for rzls, as we override the roslyn config. I've have addressed that in the rzls.nvim PR/my adjustment PR here |
|
I think I am ready when you are @tris203. I added a very basic check for if the server supports |
|
My only hold up is that the changes haven't been tagged for a release into vscode yet For people on something like Nix (like me) there isn't a build that supports |
|
Yeah no rush for me😄 just let me know when you are ready |
|
@tris203 fyi I had something like this for personal use some time ago: https://github.com/konradmalik/nixpkgs-extra/tree/643cce606e49b0759f38ff20ae0dfe95a47ad23a/pkgs/roslyn-ls This allowed to use upstream roslyn in a very similar way to how it was auto-installed in roslyn.nvim. The link to check versions was: https://dev.azure.com/azure-public/vside/_artifacts/feed/vs-impl |
yeah i have a flake that builds rzls for razor dev which i could easily add a razor-ls overlay but compiling on my laptop is slow and i am lazy |
aren't we all 🥲 |
- Added a check for the `--stdio` argument in `config.lua` to ensure it is present in the configuration. If not, it will be added and a warning notification will be shown. - Moved the `RoslynInitialized` autocmd execution in `lsp.lua` to ensure it runs before refreshing diagnostics. - Added global variable for checking if roslyn is initialized
@tris203
Just a heads up that I am planning to do something like some time after the PR for stdio support is merged dotnet/roslyn#76437
I will of course wait with this until some time after it is merged, and also wait so you can potentially do some changes so that I am not breaking your plugin.
Do you have any concerns about removing this code once it supports stdio?
I built the PR from source, and just tried it out, and with very minimal testing it seems like it works good👍
Should also hopefully fix #68