Update lsp.config to invoke roslyn-language-server directly#607
Update lsp.config to invoke roslyn-language-server directly#607JoeRobich wants to merge 4 commits into
Conversation
Due to the behavior of dotnet SDK resolution when running in repos which use a global.json, we are not gaurenteed that the choosen SDK will be new enough to support the `dotnet dnx` command, which we were using to install and run the roslyn-language-server. Instead, we make having roslyn-language-server installed a prerequisite so that we can invoke it directly.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR changes the .NET plugin to launch Roslyn's language server as a preinstalled executable instead of using dotnet dnx, to avoid SDK-resolution problems in repositories that pin older SDKs with global.json.
Changes:
- Replaces the C# LSP launch command in
plugins/dotnet/lsp.jsonwith directroslyn-language-serverexecution. - Updates the plugin README to make global installation of
roslyn-language-servera prerequisite. - Documents a new setup flow based on installing the Roslyn language server tool globally.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| plugins/dotnet/lsp.json | Switches the C# LSP server command from dotnet dnx to direct roslyn-language-server execution. |
| plugins/dotnet/README.md | Updates setup instructions and prerequisites to match the new Roslyn server launch model. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dibarbet
left a comment
dibarbet
left a comment
There was a problem hiding this comment.
Think this is the best we can do without dotnet/sdk#51085
|
|
||
| if ($currentVersion -ne $latestVersion) { | ||
| Write-Host "[$ToolName] Updating from $currentVersion to $latestVersion..." | ||
| dotnet tool update --global --prerelease $ToolName --add-source $NuGetSource |
There was a problem hiding this comment.
Currently update fails due to the LSP being launched by copilot prior to the hook running. This does not impact the user experience so decided to leave it in. If github/copilot-cli#3112 is addressed, then we get the benefit of auto-updating the LSP.
| { | ||
| "version": 1, | ||
| "hooks": { | ||
| "sessionStart": [ |
There was a problem hiding this comment.
So does this mean we'll always run this at the start of a session, even if the LSP server isn't going to be used?
There was a problem hiding this comment.
Yes this gets run at the beginning of every session.
|
|
||
| echo "[$TOOL_NAME] Latest available version: $latest_version" | ||
|
|
||
| if [[ "$current_version" != "$latest_version" ]]; then |
There was a problem hiding this comment.
Is there a reason we're not just shelling out to dotnet tool update no matter what and skipping all our own version checking?
There was a problem hiding this comment.
But I was trying to be smart. I will update.
Due to the behavior of dotnet SDK resolution when running in repos which use a global.json, we are not gaurenteed that the choosen SDK will be new enough to support the
dotnet dnxcommand which we were using to install and run the roslyn-language-server. Instead, we make having roslyn-language-server installed a prerequisite so that we can invoke it directly.