Propagate dotnet run-api errors more clearly #79445
Conversation
|
@jasonmalinowski @dibarbet for review |
| /// <param name="HasLoadErrors"> | ||
| /// Whether there were errors in the process of loading the project file itself. | ||
| /// When this is true, we may still want to proceed with design-time build in order to provide partial information about the project. | ||
| /// When this is false, <see cref="RemoteProjectFile.GetDiagnosticLogItemsAsync"/> also needs to be separately checked, to decide if evaluation was successful. |
There was a problem hiding this comment.
A couple things
- Is it not possible to have
GetDiagnosticLogItemsAsynchandle this internally? - If we call
GetDiagnosticLogItemsAsyncwhentrue, will it throw, or will it just no-op if the project failed to load?
There was a problem hiding this comment.
- It may be possible but cumbersome. RemoteProjectFile today just contains an RpcClient and an int handle. It lives in a layer which is downstream from / oblivious to the "virtual project" concept.
GetDiagnosticLogItemsis not expected to throw regardless of the value ofHasLoadErrors. Which is kinda confusing. But basically I am trying to propagate out the fact that run-api can fail, and even if things "appear to be working" with the fallback virtual project, we may still want to signal to user that something is wrong.
| { | ||
| // https://github.com/dotnet/roslyn/issues/78618: falling back to this until dotnet run-api is more widely available | ||
| _logger.LogInformation($"Failed to obtain virtual project for '{documentPath}' using dotnet run-api. Falling back to directly creating the virtual project."); | ||
| _logger.LogError($"Failed to obtain virtual project for '{documentPath}' using dotnet run-api. Falling back to directly creating the virtual project."); |
There was a problem hiding this comment.
this is still only available in .net10 right, which hasn't fully released? Should this still be a warning?
There was a problem hiding this comment.
Yes...actually, this is kinda annoying, because we probably still want to distinguish "SDK too old" cases, from "run-api is present, but its results were completely messed up". No need to report anything in the "SDK too old" case.
I will have to take a look at how to make that distinction.
| protected sealed record RemoteProjectLoadResult(RemoteProjectFile ProjectFile, ProjectSystemProjectFactory ProjectFactory, bool IsMiscellaneousFile, BuildHostProcessKind Preferred, BuildHostProcessKind Actual); | ||
| /// <param name="HasLoadErrors"> | ||
| /// Whether there were errors in the process of loading the project file itself. | ||
| /// When this is true, we may still want to proceed with design-time build in order to provide partial information about the project. |
There was a problem hiding this comment.
The semantics described here are a bit reversed of what I'd imagine we would do -- I would have expected 'true' means 'we know there are already errors, no reason to go further'.
Is the easier approach here just to return a set of diagnostics that are concatenated onto the existing things that get reported?
3 participants
Effect of this change is that if dotnet run-api fails we will start showing a toast to tell user to look at the C# logs, just like we would for other design time build errors.