-
Notifications
You must be signed in to change notification settings - Fork 754
Commands to install/uninstall roslyn-language-server for the Copilot CLI #9344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
74e515c
411a8d1
29ad016
45ed73a
ecda5a5
6f84fed
4140a06
684a7e6
b673308
db67bfb
2e434ea
0efb779
60b0ab3
b92d796
426c84a
6890590
878d50d
e1b403e
849edb5
a12608b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| { | ||
| "lspServers": { | ||
| "csharp": { | ||
| "command": "dotnet", | ||
| "args": [ | ||
| "dnx", | ||
| "roslyn-language-server", | ||
| "--yes", | ||
| "--prerelease", | ||
| "--", | ||
| "--stdio", | ||
| "--autoLoadProjects" | ||
| ], | ||
| "fileExtensions": { | ||
| ".cs": "csharp", | ||
| ".razor": "aspnetcorerazor", | ||
| ".cshtml": "aspnetcorerazor" | ||
| }, | ||
| "warmupTimeoutMs": 120000 | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ import { SolutionSnapshotProvider } from './lsptoolshost/solutionSnapshot/soluti | |
| import { BuildResultDiagnostics } from './lsptoolshost/diagnostics/buildResultReporterService'; | ||
| import { getComponentFolder } from './lsptoolshost/extensions/builtInComponents'; | ||
| import { RazorLogger } from './razor/src/razorLogger'; | ||
| import { IExperimentationService } from 'vscode-tas-client'; | ||
|
|
||
| export function activateRoslyn( | ||
| context: vscode.ExtensionContext, | ||
|
|
@@ -33,6 +34,7 @@ export function activateRoslyn( | |
| eventStream: EventStream, | ||
| csharpChannel: vscode.LogOutputChannel, | ||
| reporter: TelemetryReporter, | ||
| experimentationService: IExperimentationService | undefined, | ||
| csharpDevkitExtension: vscode.Extension<CSharpDevKitExports> | undefined, | ||
| getCoreClrDebugPromise: (languageServerStarted: Promise<any>) => Promise<void> | ||
| ): CSharpExtensionExports { | ||
|
|
@@ -78,6 +80,8 @@ export function activateRoslyn( | |
| logDirectory: context.logUri.fsPath, | ||
| determineBrowserType: BlazorDebugConfigurationProvider.determineBrowserType, | ||
| experimental: { | ||
| getTreatmentVariable: <T extends boolean | number | string>(name: string, configId = 'vscode') => | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is actually using / plan to be using this export? Doesn't appear to be for tests, so maybe we delete? |
||
| experimentationService?.getTreatmentVariable<T>(configId, name), | ||
| sendServerRequest: async (t, p, ct) => await languageServerExport.sendRequest(t, p, ct), | ||
| sendServerRequestWithProgress: async (t, p, pr, ct) => | ||
| await languageServerExport.sendRequestWithProgress(t, p, pr, ct), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,9 @@ | |
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| import * as vscode from 'vscode'; | ||
| import * as fs from 'fs/promises'; | ||
| import * as os from 'os'; | ||
| import * as path from 'path'; | ||
| import { RoslynLanguageServer } from './server/roslynLanguageServer'; | ||
| import reportIssue from '../shared/reportIssue'; | ||
| import { getDotnetInfo } from '../shared/utils/getDotnetInfo'; | ||
|
|
@@ -22,6 +25,11 @@ import { TelemetryEventNames } from '../shared/telemetryEventNames'; | |
| import { registerCollectLogsCommand } from './logging/collectLogs'; | ||
| import { ObservableLogOutputChannel } from './logging/observableLogOutputChannel'; | ||
| import { RazorLogger } from '../razor/src/razorLogger'; | ||
| import { getUninstalledCopilotLspConfigContent, getUpdatedCopilotLspConfigContent } from './copilotLspConfig'; | ||
|
|
||
| const installCopilotLspCommand = 'dotnet.installCopilotLsp'; | ||
| const uninstallCopilotLspCommand = 'dotnet.uninstallCopilotLsp'; | ||
| const packagedCopilotLspConfigPath = path.join('redist', 'lsp-config.json'); | ||
|
|
||
| export function registerCommands( | ||
| context: vscode.ExtensionContext, | ||
|
|
@@ -92,5 +100,118 @@ function registerExtensionCommands( | |
| context.subscriptions.push( | ||
| vscode.commands.registerCommand('csharp.showOutputWindow', async () => outputChannel.show()) | ||
| ); | ||
| context.subscriptions.push( | ||
| vscode.commands.registerCommand(installCopilotLspCommand, async () => { | ||
| const lspConfigPath = path.join(os.homedir(), '.copilot', 'lsp-config.json'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think we should probably also check if they have a plugin installed with an lsp.json configured for C#
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than doing manual configuration this way, should we just be installing one of our existing plugins that contains the registration?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit - could we extract this into a separate file & function (similar to other command registrations) |
||
| const extensionLspConfigPath = path.join(context.extension.extensionPath, packagedCopilotLspConfigPath); | ||
| let packagedContent: string; | ||
| try { | ||
| packagedContent = await fs.readFile(extensionLspConfigPath, 'utf8'); | ||
| } catch (error) { | ||
| const nodeError = error as NodeJS.ErrnoException; | ||
| void vscode.window.showErrorMessage( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit - I don't necessarily think it's worth catching this error - we don't expect this to fail, and if it does the command execution should show a modal dialog if it does. Doesn't feel necessary to wrap into a special localized exception |
||
| vscode.l10n.t('Failed to read packaged Copilot LSP config: {0}', nodeError.message) | ||
| ); | ||
| return; | ||
| } | ||
| let currentContent: string | undefined; | ||
|
|
||
| try { | ||
| currentContent = await fs.readFile(lspConfigPath, 'utf8'); | ||
| } catch (error) { | ||
| const nodeError = error as NodeJS.ErrnoException; | ||
| if (nodeError.code !== 'ENOENT') { | ||
| void vscode.window.showErrorMessage( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| vscode.l10n.t('Failed to read Copilot LSP config: {0}', nodeError.message) | ||
| ); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| let updateResult: { shouldWrite: boolean; updatedContent?: string }; | ||
| try { | ||
| updateResult = getUpdatedCopilotLspConfigContent(currentContent, packagedContent); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to before, I don't think we expect this to throw, so imho it is fine to let this bubble up without wrapping into a nice exception |
||
| void vscode.window.showErrorMessage(vscode.l10n.t('Failed to update Copilot LSP config: {0}', message)); | ||
| return; | ||
| } | ||
|
|
||
| if (!updateResult.shouldWrite || !updateResult.updatedContent) { | ||
| void vscode.window.showInformationMessage( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like this should be a modal dialog - the user explicitly invoked a command, so it makes sense to be noisy |
||
| vscode.l10n.t( | ||
| 'Copilot LSP config already contains a C# LSP mapping for .cs files. No changes were made.' | ||
| ) | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await fs.mkdir(path.dirname(lspConfigPath), { recursive: true }); | ||
| await fs.writeFile(lspConfigPath, updateResult.updatedContent, 'utf8'); | ||
| void vscode.window.showInformationMessage( | ||
| vscode.l10n.t('Updated Copilot LSP config at {0}.', lspConfigPath) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would it be worth opening the config file after we write it, so the user can see the change? |
||
| ); | ||
| } catch (error) { | ||
| const nodeError = error as NodeJS.ErrnoException; | ||
| void vscode.window.showErrorMessage( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as above - could throw a nice exception and let it bubble up to the command handling |
||
| vscode.l10n.t('Failed to write Copilot LSP config: {0}', nodeError.message) | ||
| ); | ||
| } | ||
| }) | ||
| ); | ||
| context.subscriptions.push( | ||
| vscode.commands.registerCommand(uninstallCopilotLspCommand, async () => { | ||
| const lspConfigPath = path.join(os.homedir(), '.copilot', 'lsp-config.json'); | ||
|
|
||
| let currentContent: string | undefined; | ||
| try { | ||
| currentContent = await fs.readFile(lspConfigPath, 'utf8'); | ||
| } catch (error) { | ||
| const nodeError = error as NodeJS.ErrnoException; | ||
| if (nodeError.code !== 'ENOENT') { | ||
| void vscode.window.showErrorMessage( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same nit on throwing (and below) |
||
| vscode.l10n.t('Failed to read Copilot LSP config: {0}', nodeError.message) | ||
| ); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| let uninstallResult: { shouldWrite: boolean; updatedContent?: string }; | ||
| try { | ||
| uninstallResult = getUninstalledCopilotLspConfigContent(currentContent); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| void vscode.window.showErrorMessage( | ||
| vscode.l10n.t('Failed to uninstall Copilot LSP config: {0}', message) | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| if (!uninstallResult.shouldWrite || !uninstallResult.updatedContent) { | ||
| void vscode.window.showInformationMessage( | ||
| vscode.l10n.t( | ||
| 'Copilot LSP config does not contain a C# LSP mapping for .cs files. No changes were made.' | ||
| ) | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await fs.writeFile(lspConfigPath, uninstallResult.updatedContent, 'utf8'); | ||
| void vscode.window.showInformationMessage( | ||
| vscode.l10n.t( | ||
| 'Removed C# LSP mapping(s) for .cs files from Copilot LSP config at {0}.', | ||
| lspConfigPath | ||
| ) | ||
| ); | ||
| } catch (error) { | ||
| const nodeError = error as NodeJS.ErrnoException; | ||
| void vscode.window.showErrorMessage( | ||
| vscode.l10n.t('Failed to write Copilot LSP config: {0}', nodeError.message) | ||
| ); | ||
| } | ||
| }) | ||
| ); | ||
| registerCollectLogsCommand(context, languageServer, outputChannel, csharpTraceChannel, razorLogger); | ||
| } | ||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something we expect is happening automatically during a build or something we need to do on a regular basis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should happen automatically on build - otherwise a build could randomly fail if the config changes.