Fix restore for 'dotnet run app.cs' in IDE #78990
Conversation
|
@jasonmalinowski @dibarbet Please take a look |
...erver/Microsoft.CodeAnalysis.LanguageServer/LanguageServer/Handler/Restore/RestoreHandler.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LoadedProject.cs
Outdated
Show resolved
Hide resolved
…ageServer/Handler/Restore/RestoreHandler.cs Co-authored-by: Jason Malinowski <[email protected]>
| @@ -35,13 +38,16 @@ public async Task<RestorePartialResult[]> HandleRequestAsync(RestoreParams reque | |||
| var restorePaths = GetRestorePaths(request, context.Solution, context); | |||
| if (restorePaths.IsEmpty) | |||
| { | |||
| _logger.LogInformation($"Restore was requested but no paths were provided."); | |||
There was a problem hiding this comment.
I'm not sure this is necessary - it should already be reported here - https://github.com/dotnet/vscode-csharp/blob/main/src/lsptoolshost/projectRestore/restore.ts#L104
Same for all the rest of the logs
There was a problem hiding this comment.
Logging basic info for when a restore starts/finishes in the C# channel helps determine if restore and design time builds are happening in the expected order. Even if the restore logs had timestamps, jumping between the two output windows to figure that out is cumbersome.
Restore is an infrequent enough and significant enough event that IMO writing it to this channel is going to help our dogfooders as they encounter bugs. It seems good to keep detailed information about restore failures, etc. in its own channel, though.
Indicating the ".NET NuGet Restore" channel in the info message here would also help with discoverability. Basically when dogfooders encounter problems, they will start investigating at the base C# log window, and having a breadcrumb in there to indicate which channel to look at for more info is going to help.
There was a problem hiding this comment.
Logging basic info for when a restore starts/finishes in the C# channel helps determine if restore and design time builds are happening in the expected order. Even if the restore logs had timestamps, jumping between the two output windows to figure that out is cumbersome.
This seems like it should be under debug logging (would be fine with that). For whatever it is worth, the restore channel I believe should be automatically shown if there is a failure.
Indicating the ".NET NuGet Restore" channel in the info message here would also help with discoverability.
You mentioning this made me realize something else. When you're using C# devkit, there are going to be two nuget restore channels (one from devkit restoring the sln, one from us here). That is definitely going to be confusing, and previously we did ensure ours wouldn't show up when using devkit.
I wonder if at some point we need to see if we can utilize devkit to do the restore (so it can all go through their service).
There was a problem hiding this comment.
For whatever it is worth, the restore channel I believe should be automatically shown if there is a failure.
I didn't find this to be the case. When I introduced a bad #:package directive it only showed the following toast (did also check the "drawer" but didn't see any other error toasts).
Clicking it goes to the C# logs.
There was a problem hiding this comment.
Ah, that is likely something we could fix (would have to take a look).
|
@jasonmalinowski @dibarbet This is ready for another review pass |
...erver/Microsoft.CodeAnalysis.LanguageServer/LanguageServer/Handler/Restore/RestoreHandler.cs
Outdated
Show resolved
Hide resolved
...erver/Microsoft.CodeAnalysis.LanguageServer/LanguageServer/Handler/Restore/RestoreHandler.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LoadedProject.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LoadedProject.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LoadedProject.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LoadedProject.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Chatted with @RikkiGibson to explain next steps so approving on that understanding.
|
It looks like a bunch of test jobs timed out |
Closes #78944
The file change handler needs to make sure to reload the project if the project assets file was the thing that changed.
I am curious if something about the reloading of ordinary projects was causing this bug to not occur in ordinary project scenarios, but, I don't know at the moment.