- Notifications
You must be signed in to change notification settings - Fork714
Register restore commands even if DevKit is being used#8428
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 ourterms of service andprivacy 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?
Conversation
| @@ -21,7 +21,8 @@ let _restoreInProgress = false; | |||
| exportfunctionregisterRestoreCommands(context:vscode.ExtensionContext,languageServer:RoslynLanguageServer){ | |||
| if(getCSharpDevKit()){ | |||
| // We do not need to register restore commands if using C# devkit. | |||
| return; | |||
| // TODO: restore commands for FBPs still need to run, even if devkit is being used. | |||
| // return; | |||
| } | |||
| constrestoreChannel=vscode.window.createOutputChannel(vscode.l10n.t('.NET NuGet Restore')); | |||
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.
this is going to very unfortunately create a second nuget restore channel. Other than having devkit do the restore though I don't have a great alternative. But we should get an issue filed on this at the very least.
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.
Could we use the C# log for the time being? Had the O# extension not separated them, I am not sure I would have the distinction. OR, possibly I would have put all project system logging into the restore one.
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.
My preference would actually be to put these in the C# log. When we split across output windows it just becomes harder to tell the order that things occur in. Even when there are timestamps (which I am adding in this PR.)
However to reduce scope I am going to refrain from renaming or moving which output channel we use here.
Also, what isl10n? Is this just a super weird abbreviation? An 'l' then 10 letters then an 'n' to make 'localization'?
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.
n 'l' then 10 letters then an 'n' to make 'localization'?
Yup!
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.
Agreed, lets do the C# window then. Better than a duplicate output window.
…ther Dev Kit is currently loaded
| constrestoreChannel=vscode.window.createOutputChannel(vscode.l10n.t('.NET NuGet Restore')); | ||
| context.subscriptions.push( | ||
| vscode.commands.registerCommand('dotnet.restore.project',async(_request):Promise<void>=>{ | ||
| if(getCSharpDevKit()){ | ||
| appendLineWithTimestamp(restoreChannel,"Not handling command 'dotnet.restore.project' from C# extension, because C# Dev Kit is expected to handle it."); |
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.
Interestingly, when C#+CDK are loaded together to begin with, I cannot get these lines to run, but then if I unload+reload CDK, restart language server, (but do not reload window), I can get them to run.
I think this is related to some other mechanism unregistering the "dotnet restore" commands when CDK is loaded. The ".NET: Restore all projects" and ".NET: Restore projects" commands are just not there when CDK is loaded.
Thankfully since the last handler is for a language server message, not for a VS Code command, it will get hit regardless of whether CDK is loaded, when the project system says there are unresolved assets.
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.
The general goal and philosophy of the change is: deviate behavior as late as possible, and as little as possible, and make it clear that the deviation is occurring, when the thing we are supposed to do, depends on Dev Kit being loaded or not. I think this is something we should look at doing with other calls togetCSharpDevKit() also.
This comment was marked as resolved.
This comment was marked as resolved.
Unsure what to do with the following failure:https://dev.azure.com/dnceng-public/public/_build/results?buildId=1099482&view=ms.vss-test-web.build-test-results-tab&runId=30068902&resultId=100010&paneView=debug
Basically what I am observing in the running product is that these commands aren't available to run when CDK is loaded. If you unload CDK they appear, reload and they disappear. So I am wondering if something about how we are detecting the commands in this context, is out of sync with the VS Code editor. |
@RikkiGibson you need to remove the enablement in the package.json to ensure it is enabled in all contexts - Line 1852 in341a80a
|
Uh oh!
There was an error while loading.Please reload this page.
The bug we observed with restore is: we have a new, specific restore behavior for file-based apps, which is implemented only in the base C# extension. When CDK is also loaded, the client simply never responds to
projectNeedsRestoremessages and similar from the server.But CDK doesn't handle restore of file-based apps. So the effect we see is: when both C#+CDK are loaded together in VS Code (from a fresh "reload window" or fresh startup), restore will just not work. The client will silently not initiate the restore process.
I doubt that just always attaching the restore handlers anyways is the right solution. But, as a short term solution, we could simply attach the handlers and say:if CDK is loaded, then only do the restore for file-based apps, while assuming CDK continues to handle projects and solutions.
Wouldn't it be most reasonable to "forward" the message to CDK, and have some way of deciding if CDK handled it, and if they did not, then we handle it? Sorta like how UI event handlers bubble events up? Maybe this doesn't work because CDK has its own completely different mechanism for watching for relevant changes and doing the right thing automatically.
@jasonmalinowski@dibarbet