Added a message of enabled logs, and their paths.#13577
Added a message of enabled logs, and their paths.#13577AlesProkop wants to merge 8 commits intodotnet:mainfrom
Conversation
|
@dotnet-policy-service agree company="Microsoft" |
There was a problem hiding this comment.
Pull request overview
This PR improves MSBuild’s user-facing logging by surfacing which loggers are enabled (e.g., /bl, /fl) and where their output files are being written, addressing confusion called out in #12261.
Changes:
- Introduces
LoggerRegisteredEventArgsto publish information about enabled loggers (name/verbosity/output paths). - Adds
IFileOutputLoggerand updatesBinaryLogger/FileLoggerto expose their output file paths. - Updates TerminalLogger/ParallelConsoleLogger to collect these events and print output paths at build end; adds new localized resource strings and unit tests.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Framework/LoggerRegisteredEventArgs.cs | New Framework event args type used to report enabled logger details |
| src/Build/BackEnd/Components/Logging/LoggingServiceLogMethods.cs | Emits “enabled logs” message + logger registration events during build start |
| src/Build/Logging/IFileOutputLogger.cs | New internal interface for loggers that produce a primary output file |
| src/Build/Logging/FileLogger.cs | Implements IFileOutputLogger to expose the log file path |
| src/Build/Logging/BinaryLogger/BinaryLogger.cs | Implements IFileOutputLogger to expose the binlog path |
| src/Build/Logging/TerminalLogger/TerminalLogger.cs | Captures logger-registration events and prints clickable paths in summary |
| src/Build/Logging/ParallelLogger/ParallelConsoleLogger.cs | Captures logger-registration events and prints paths in summary |
| src/Build/Resources/Strings.resx | Adds new resource strings for enabled logs / output file paths |
| src/Build/Resources/xlf/Strings.*.xlf | Adds corresponding localization entries (state=new) |
| src/Build/Microsoft.Build.csproj | Includes the new IFileOutputLogger.cs compile item |
| src/Build.UnitTests/BackEnd/LoggingServicesLogMethod_Tests.cs | Adds unit tests for enabled log names and file path reporting |
| ? fileLogger.OutputFilePath | ||
| : null; | ||
|
|
||
| IReadOnlyList<string> additionalPaths = actualLogger is BinaryLogger bl |
There was a problem hiding this comment.
Can you please explain the meaning of this AdditionalFilePaths? Can we maybe just add those to the OutputFilePath turning it to OutputFilePaths for all loggers?
There was a problem hiding this comment.
A binary logger has this property so it can have multiple output paths. I am not sure if it is used but wanted to support it. I agree with your change, implemented it in the last commit.
| [Serializable] | ||
| public class LoggerRegisteredEventArgs : BuildMessageEventArgs | ||
| { | ||
| protected LoggerRegisteredEventArgs() |
There was a problem hiding this comment.
Why do we need this constructor protected?
There was a problem hiding this comment.
After the changes, I changed it to internal. I think we do not want it public because an empty LoggerRegisteredEventArgs could be created which would be meaningless.
| /// </summary> | ||
| internal Dictionary<(int nodeId, int contextId), string> propertyOutputMap = new Dictionary<(int nodeId, int contextId), string>(); | ||
|
|
||
| private readonly List<LoggerRegisteredEventArgs> _registeredLoggers = new List<LoggerRegisteredEventArgs>(); |
There was a problem hiding this comment.
nit: Prefer new() syntax per modern C# conventions
| return; | ||
| } | ||
|
|
||
| if (e is LoggerRegisteredEventArgs loggerEvent) |
There was a problem hiding this comment.
The showOnlyErrors/showOnlyWarnings early returns before this capture, so under /clp:ErrorsOnly (or WarningsOnly) the loggers are never recorded and the summary silently omits the file-paths section.
There was a problem hiding this comment.
This should not be a problem anymore, since I changed the inheritance.
| /// Arguments for the logger registered event, containing one or more logger registrations. | ||
| /// </summary> | ||
| [Serializable] | ||
| public sealed class LoggerRegisteredEventArgs : BuildMessageEventArgs |
There was a problem hiding this comment.
As this event inherits from BuildMessageEventArgs, every third-party logger now receives it via MessageRaised (possibly with a null Message) and binlogs round-trip it lossily as a plain message. I think deriving from BuildStatusEventArgs (or at least guaranteeing a non null fallback message and bumping the binlog format) would be safer.
There was a problem hiding this comment.
All right, thank you. Implemented that in the last commit.
Fixes #12261
Context
There was no clear message in the logs when using either the
/blor the/flflag that would indicate the location of the files that are being created by these logs. Also, there was no consistent way of telling which logs were enabled.Changes Made
LoggingServiceLogMethods.cs
Added a method LogLoggersUsed(bool fileNames) which lists the names of all enabled logs in a diagnostic verbosity. Added new EventArgs which registers every enabled logger with information. This is then used for logging clickable links of paths that were created by loggers.
LoggerRegisteredEventArgs.cs
New EventArgs used for registering enabled loggers. It cointains infromation about the logger, such as the name of the logger, verbosity on which the logger operates on, file paths to files created by the logger and possible additional output file paths.
ParallelConsoleLogger.cs
Added handling of the LoggerRegisteredEventArgs.
TerminalLogger.cs
Added handling of the LoggerRegisteredEventArgs.
FileLogger.cs
Added a way to read the path to the file created by the filelogger, similarly to how binlog operates.
Testing
Added unit tests to LoggingServicesLogMethod_Tests.cs.