Report better warnings and errors if build hosts exit abnormally #71909
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This avoids having to flip to trace mode just to find the cause of the failure.
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
I'm not sure why this test is directly tesing MSBuildProjectLoder when we instead could have used the API that literally every other test uses.
This connects our various error reporting systems so failures of the build hosts can get passed along back to the existing reporting mechanism for MSBuildWorkspace failures.
jasonmalinowski
force-pushed
the
log-buildhost-process-exits
branch
from
56b4868
to
19751d7
Compare
jasonmalinowski
removed
the
untriaged
jasonmalinowski
commented
Feb 1, 2024
Comment on lines
+62
to
+66
| _solutionServices = workspace.Services.SolutionServices; | ||
| _diagnosticReporter = new DiagnosticReporter(workspace); | ||
| _loggerFactory = DiagnosticReporterLoggerProvider.CreateLoggerFactoryForDiagnosticReporter(_diagnosticReporter); | ||
| _pathResolver = new PathResolver(_diagnosticReporter); | ||
| _projectFileExtensionRegistry = new ProjectFileExtensionRegistry(_solutionServices, _diagnosticReporter); |
There was a problem hiding this comment.
Trying to keep both constructors working was a getting a bit convoluted -- the nullable parameters to the other constructor existed only to make it possible to even write this as a :this() chain, and adding more nullable parameters to keep it going was just getting sillier too. So I'm just splitting into two, which ensures that the mainline use of the internal constructor is never getting a null from the other parts of the system.
dibarbet
approved these changes
Feb 5, 2024
ghost
added this to the 
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reports warnings or errors through the ILogger passed to the BuildHostProcessManager if a process exits abnormally, and includes the stderr of the process. Without this you'd be able to manually turn on tracing in VS Code, but that required extra steps to diagnose. This also allows us to pass warnings and errors through to MSBuildWorkspace, which otherwise didn't pass along this information.
Commit-at-a-time might be helpful, since an unrelated test got cleaned up during this change since it was written differently than the rest of all the tests.