separate the tracking parts of TL into a reusable base, so that TL becomes a 'renderer'#12587
separate the tracking parts of TL into a reusable base, so that TL becomes a 'renderer'#12587
Conversation
baronfel
added
the
Area: Terminal Logger
baronfel
force-pushed
the
tl-logger-base
branch
from
57f75d7 to
9939288
Compare
…st cares about rendering
baronfel
force-pushed
the
tl-logger-base
branch
from
9939288 to
cf306ab
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the TerminalLogger to extract common project tracking logic into a new base class ProjectTrackingLoggerBase. The goal is to enable reusability for other logger implementations that need to track build state.
Key changes:
- Introduces
ProjectTrackingLoggerBase<TEvalData, TNodeData, TProjectData, TBuildData>as a generic base class for loggers that track project state - Makes several internal types public to support extensibility:
TerminalColor,TerminalMessageSeverity,TerminalBuildMessage,TerminalNodeStatus,TerminalProjectInfo,EvalProjectInfo,StopwatchAbstraction - Creates a new
TerminalBuildDataclass to encapsulate build-level state - Refactors TerminalLogger to inherit from the base class and override virtual methods
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Framework/Logging/TerminalColor.cs | Changed enum from internal to public for API extensibility |
| src/Build/Microsoft.Build.csproj | Added ProjectTrackingLoggerBase.cs to compilation |
| src/Build/Logging/TerminalLogger/TerminalProjectInfo.cs | Removed ProjectContext parameter and Id property, simplified to use EvalProjectInfo directly |
| src/Build/Logging/TerminalLogger/TerminalNodeStatus.cs | Changed class from internal to public for API extensibility |
| src/Build/Logging/TerminalLogger/TerminalMessageSeverity.cs | Changed enum from internal to public for API extensibility |
| src/Build/Logging/TerminalLogger/TerminalLogger.cs | Refactored to inherit from ProjectTrackingLoggerBase and override virtual methods instead of implementing direct event handlers |
| src/Build/Logging/TerminalLogger/TerminalBuildMessage.cs | Changed record from internal to public for API extensibility |
| src/Build/Logging/TerminalLogger/TerminalBuildData.cs | New class to encapsulate build-level state (error counts, warnings, restore state) |
| src/Build/Logging/TerminalLogger/StopwatchAbstraction.cs | Changed class from internal to public for API extensibility |
| src/Build/Logging/ProjectTrackingLoggerBase.cs | New abstract base class providing common project tracking infrastructure for loggers |
| /// </summary> | ||
| private void BuildFinishedHandler(object sender, BuildFinishedEventArgs e) | ||
| { | ||
| OnBuildFinished(e, _projectDataByProjectContextId.Values.ToArray(), _buildData!); |
There was a problem hiding this comment.
Using LINQ's .ToArray() on dictionary values can cause unnecessary allocations. According to the coding guidelines, performance is critical and LINQ should be avoided. Consider using a manual array copy or passing the Values collection directly if the consumer can accept an ICollection<TProjectData> instead.
| } | ||
| } | ||
|
|
||
| public void YieldNode(BuildEventArgs e) |
There was a problem hiding this comment.
The YieldNode method is declared as public but appears to be an internal implementation detail. Consider making it private or protected to avoid exposing internal node management to external consumers.
| public void YieldNode(BuildEventArgs e) | |
| private void YieldNode(BuildEventArgs e) |
|
|
||
| #region Protected helpers | ||
|
|
||
| protected int? GetNodeIdForEvent(BuildEventArgs args) => args?.BuildEventContext is null ? null : NodeIndexForContext(args.BuildEventContext); |
There was a problem hiding this comment.
The method GetNodeIdForEvent returns a nullable int without documentation explaining that null indicates an invalid node, and doesn't validate that the computed index is within bounds. Callers like StartNode and YieldNode in TerminalLogger use this to access _nodes[nodeId] without bounds checking. Consider adding range validation or documenting the expected behavior.
| protected int? GetNodeIdForEvent(BuildEventArgs args) => args?.BuildEventContext is null ? null : NodeIndexForContext(args.BuildEventContext); | |
| /// <summary> | |
| /// Returns the zero-based node index for the given event, or <c>null</c> if the event or its context is <c>null</c> | |
| /// or if the node ID is not valid (less than or equal to zero). | |
| /// Callers should check for <c>null</c> before using the result as an array index. | |
| /// </summary> | |
| /// <param name="args">The build event arguments.</param> | |
| /// <returns> | |
| /// The zero-based node index, or <c>null</c> if the event/context is <c>null</c> or the node ID is invalid. | |
| /// </returns> | |
| protected int? GetNodeIdForEvent(BuildEventArgs args) | |
| { | |
| var context = args?.BuildEventContext; | |
| if (context is null || context.NodeId <= 0) | |
| { | |
| return null; | |
| } | |
| return NodeIndexForContext(context); | |
| } |
| private TerminalNodeStatus? GetNodeForEvent(BuildEventArgs e) | ||
| { | ||
| var node = GetNodeIdForEvent(e); | ||
| if (node is int nodeId && _nodes[nodeId] is TerminalNodeStatus status) |
There was a problem hiding this comment.
Array access _nodes[nodeId] can throw IndexOutOfRangeException if nodeId is outside the valid range. While NodeIndexForContext subtracts 1 from context.NodeId, there's no validation that the result is within [0, NodeCount). Add bounds checking before array access to prevent runtime exceptions.
| if (node is int nodeId && _nodes[nodeId] is TerminalNodeStatus status) | |
| if (node is int nodeId | |
| && nodeId >= 0 | |
| && nodeId < _nodes.Length | |
| && _nodes[nodeId] is TerminalNodeStatus status) |
| var node = GetNodeIdForEvent(e); | ||
| if (node is int nodeId) | ||
| { | ||
| _nodes[nodeId] = status; |
There was a problem hiding this comment.
Array access _nodes[nodeId] can throw IndexOutOfRangeException if nodeId is outside the valid range. While NodeIndexForContext subtracts 1 from context.NodeId, there's no validation that the result is within [0, NodeCount). Add bounds checking before array access to prevent runtime exceptions.
| /// Number of build warnings. | ||
| /// </summary> | ||
| private int _buildWarningsCount; | ||
| // Tracking dictionaries and node array are now inherited from BuildTrackerLogger |
There was a problem hiding this comment.
The comment references 'BuildTrackerLogger' but the actual base class name is 'ProjectTrackingLoggerBase'. This could confuse developers reading the code.
| /// True if restore failed and this failure has already been reported. | ||
| /// </summary> | ||
| private bool _restoreFailed; | ||
| // BuildStartTime is now inherited from BuildTrackerLogger |
There was a problem hiding this comment.
The comment references 'BuildTrackerLogger' but the actual base class name is 'ProjectTrackingLoggerBase'. This could confuse developers reading the code.
| // BuildStartTime is now inherited from BuildTrackerLogger | |
| // BuildStartTime is now inherited from ProjectTrackingLoggerBase |
| /// not restoring. | ||
| /// </summary> | ||
| private ProjectContext? _restoreContext; | ||
| // Build error/warning counts and restore state are now inherited from BuildTrackerLogger |
There was a problem hiding this comment.
The comment references 'BuildTrackerLogger' but the actual base class name is 'ProjectTrackingLoggerBase'. This could confuse developers reading the code.
|
|
||
| // Mark node idle until something uses it again | ||
| if (_restoreContext is null) | ||
| if (buildData.RestoreContext is null) |
There was a problem hiding this comment.
| string duration = (e.Timestamp - buildData.BuildStartTime).TotalSeconds.ToString("F1"); | ||
| string buildResult = GetBuildResultString(e.Succeeded, buildData.BuildErrorsCount, buildData.BuildWarningsCount); |
There was a problem hiding this comment.
Variable buildData may be null at this access as suggested by this null check.
| string duration = (e.Timestamp - buildData.BuildStartTime).TotalSeconds.ToString("F1"); | |
| string buildResult = GetBuildResultString(e.Succeeded, buildData.BuildErrorsCount, buildData.BuildWarningsCount); | |
| string duration; | |
| string buildResult; | |
| if (buildData != null) | |
| { | |
| duration = (e.Timestamp - buildData.BuildStartTime).TotalSeconds.ToString("F1"); | |
| buildResult = GetBuildResultString(e.Succeeded, buildData.BuildErrorsCount, buildData.BuildWarningsCount); | |
| } | |
| else | |
| { | |
| duration = "0.0"; | |
| buildResult = GetBuildResultString(e.Succeeded, 0, 0); | |
| } |
|
@copilot Please resolve the merge conflicts. There are apparently 21 separate conflicts. |
|
@richlander I've opened a new pull request, #13138, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
This pull request has been automatically closed because it has been open for more than 180 days with no recent activity. If you believe this work is still relevant, please feel free to reopen or create a new pull request. Thank you for your contribution!
|
|
This pull request has been automatically closed because it has been open for more than 180 days with no recent activity. If you believe this work is still relevant, please feel free to reopen or create a new pull request. Thank you for your contribution! Note 🔒 Integrity filter blocked 39 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
|
This pull request has been automatically closed because it has been open for more than 180 days with no recent activity. If you believe this work is still relevant, please feel free to reopen or create a new pull request. Thank you for your contribution! Note 🔒 Integrity filter blocked 43 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
This makes it easier to implement other Loggers - you just need to pick out what kinds of data you need from evaluation/projects, and then render warnings, errors, etc however you want during certain lifecycle events. The thought is that all of the data required to track/associate/trigger the lifecycle warnings is kept by the 'tracker', so your use-case-specific logger can mostly ignore that.
This is low-priority ATM, but something that may be useful for us as we think about how Loggers might need to change to more easily react to LLMs and other tools. For example, @richlander has a prototype logger that emits markdown - I'm pretty sure that logger doesn't understand the relationship between 'outer' builds and 'inner' like TL does. If it built on this base and then it could more easily get that understanding for free.
I kind of think of this interface as a step towards a more user-friendly Logger programming model - for loggers that care about rendering the 'intent' of the build and reshaping/papering over MSBuild specifics in favor of a more user-centric model this base can hide a lot of the guts.