Improve plugin exception logging #6368
Conversation
Fixes the issue of Logger.HasLoggedErrors leaking between API calls
In this case the exception is not what the cache printed, but MSBuild's message that the cache logged an error
cdmihai
force-pushed
the
improvePluginExceptionLogging
branch
from
2856843
to
a617a42
Compare
… user errors w.r.t. project cache
cdmihai
force-pushed
the
improvePluginExceptionLogging
branch
from
373b11f
to
252e023
Compare
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
There's a glob include for the entire ProjectCache directory above.
| cacheItems.Count == 1, | ||
| "OnlyOneCachePluginMustBeSpecified", | ||
| string.Join("; ", cacheItems.Select(ci => ci.PluginPath))); | ||
| if (cacheItems.Count != 1) |
There was a problem hiding this comment.
Could this (and others) be wrapped in a new VerifyThrow instead?
There was a problem hiding this comment.
Could be, but it may end up looking uglier since there's specific details like the exception type and what specific args to send to its particular constructor.
There was a problem hiding this comment.
I was considering commenting similar, maybe the next iteration creates some plugin-specific ErrorUtilities method.
There was a problem hiding this comment.
The exception creation patterns are so particular to ProjectCacheException I'd rather just have factory methods on the type. We could pass in lambdas to ErrorUtilities but I think that would just complicate it without much benefit.
| { | ||
| cacheResult = await _projectCachePlugin.GetCacheResultAsync(buildRequest, logger, _cancellationToken); | ||
| } | ||
| catch (Exception e) |
There was a problem hiding this comment.
Looking at what this actually means, just filter out critical exceptions?
There was a problem hiding this comment.
It needs to be Exception because the cache can throw any exception type if it malfunctions. Filtering is done in HandlePluginException to avoid repeating it.
Context
Treat project cache exceptions similar to how MSBuild treats exceptions from
ILogger.InitializeandILogger.Shutdown. This avoids the "file an issue against the MSBuild team" console messages.Treat errors logged by the project cache similar to how MSBuild treats errors logged by tasks. Which is to check whether they logged an error and if yes, shut the build down (project cache does not do "continue on error").
Changes Made
ProjectCacheExceptionwhich gets handled similarly toLoggerExceptionTesting