Fix TerminalLogger auto-detection under MSBuild Server on Unix - #14077
Conversation
|
/review |
|
MSBuild Target Authoring Conventions — LGTM No .targets or .props files were modified in this PR. The changes are limited to C# source files (NativeMethods.cs, OutOfProcServerNode.cs, and test files), so this dimension is not applicable. |
There was a problem hiding this comment.
Pull request overview
This PR fixes TerminalLogger auto-detection when MSBuild Server is enabled by ensuring ANSI/screen capability detection reflects the client terminal (transmitted TargetConsoleConfiguration) rather than the server node’s redirected stdout/stderr. This aligns -tl:auto behavior under the server with non-server behavior, particularly on Unix.
Changes:
- Added a
NativeMethods.ConsoleConfigurationOverrideseam soQueryIsScreenAndTryEnableAnsiColorCodescan use client-transmitted capabilities (and avoid mutating/restoring node console mode). - Set/cleared the override in
OutOfProcServerNodeper build request so logger selection occurs using client terminal state. - Added unit/E2E coverage validating the override behavior and guarding against
-tl:autoproducing TerminalLogger ANSI output when output is redirected.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Framework/NativeMethods.cs | Adds ConsoleConfigurationOverride and makes QueryIsScreenAndTryEnableAnsiColorCodes honor it (returning originalConsoleMode: null). |
| src/Build/BackEnd/Node/OutOfProcServerNode.cs | Sets the override from the client’s transmitted console configuration and clears it after the build. |
| src/Framework.UnitTests/NativeMethods_Tests.cs | New tests validating QueryIsScreenAndTryEnableAnsiColorCodes honors the override and returns null original console mode. |
| src/Build.UnitTests/TerminalLogger_Tests.cs | Regression test validating -tl:auto selection honors the transmitted console capabilities via the override. |
| src/MSBuild.UnitTests/MSBuildServer_Tests.cs | E2E regression guard that redirected output with server enabled does not contain TerminalLogger ANSI sequences. |
There was a problem hiding this comment.
24-Dimension Code Review — PR #14077
Fix TerminalLogger auto-detection under MSBuild Server on Unix
| Dimension | Result |
|---|---|
| Test Coverage & Completeness | (true,false) / (false,true) InlineData cases |
| Logging & Diagnostics Rigor | 💬 NIT — no trace call when override is applied |
| Documentation Accuracy | 💬 NIT — doc comment understates when the property is null |
✅ 21/24 dimensions clean — no BLOCKING or MAJOR findings.
Design correctness note
Two sub-agents independently raised a concern about the ~36-line gap between setting ConsoleConfigurationOverride (line 407) and the try/finally that clears it (line 444). A validation agent read the actual code and confirmed this is not a real bug: any exception thrown in that gap propagates to HandleServerNodeBuildCommandAsync, which sets _shutdownReason = NodeEngineShutdownReason.Error; the Run() loop returns immediately on anything other than BuildCompleteReuse, so the server process terminates before any subsequent build could observe the stale override. The design is sound.
Overall assessment
The fix is architecturally correct. The ConsoleConfigurationOverride seam is the right choice — consulting ConsoleConfiguration.Provider directly from NativeMethods would create a circular assembly dependency (Microsoft.Build.Framework → Microsoft.Build). The layer boundary is respected. The override is set/cleared per-build with proper exception safety (server-exit semantics), the property is internal, correctly documented, and allocation-free. The E2E test and unit tests provide meaningful coverage of the bug being fixed.
The only action item worth addressing before merge is the test-coverage gap (adding the two mixed InlineData cases) to harden the regression guard against future condition-simplification errors.
Generated by Expert Code Review (command) for issue #14077 · 2.2K AIC · ⊞ 29.8K ambient context
Comment /review to run again
|
Thanks for the review. Addressed in 60b69a9:
Build clean; Framework (5), |
Under MSBuild Server, TerminalLogger auto-detection runs in the server node, whose stdout is a redirected pipe. QueryIsScreenAndTryEnableAnsiColorCodes inspected the node's own console instead of the client's transmitted TargetConsoleConfiguration, so -tl:auto fell back to the console logger even from a real interactive terminal (reproduces on Unix; also mis-detected msbuild > file.txt on Windows). Add NativeMethods.ConsoleConfigurationOverride, set by the server node from the client's transmitted console configuration, and consulted first by QueryIsScreenAndTryEnableAnsiColorCodes. It is refreshed per build and cleared in a finally, so a reused server node never observes stale state. Fixes dotnet#13940 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- OutOfProcServerNode: widen try/finally to cover all setup after the override is set, and restore Console.Out/Error in the finally so a reused server node never observes a stale override or disposed writers on a failure path. Trace the transmitted console configuration for diagnosability. - TerminalLogger_Tests: add off-diagonal (true,false)/(false,true) cases to pin that both acceptAnsi and outputIsScreen are required for -tl:auto. - NativeMethods: clarify that ConsoleConfigurationOverride is also null while a server node is idle between builds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
60b69a9 to
07c10c0
Compare
…ild#14077) The SDK redistributed MSBuild 18.9.0-preview-26311-113 (source-dated 2026-06-11), which predates the TerminalLogger-under-MSBuild-Server fix (dotnet/msbuild#14077, merged 2026-06-17). With the server enabled by default, that bug caused integration tests capturing 'dotnet build' output to fail. Version.Details.props lagged behind Version.Details.xml (already at 26325-102); this aligns them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 499249b7-514f-4ecf-88bc-2466455069c5
Fixes #13940
Context
With MSBuild Server enabled (
MSBUILDUSESERVER=1/DOTNET_CLI_USE_MSBUILD_SERVER=true),-tl:auto(and the default) does not select TerminalLogger even from a real interactive terminal — it falls back to the classic console logger. The same invocation with the server disabled selects TL correctly. Reproduces on Unix (macOS/Linux); on Windows the inverse mis-detection happens (TL stays on undermsbuild > file.txt).Root cause: TerminalLogger auto-detection runs in
XMake.ProcessTerminalLoggerConfiguration, which under the server executes in the server node.NativeMethods.QueryIsScreenAndTryEnableAnsiColorCodes()inspects the current process'sConsole, but the node's stdout is a redirected pipe, so the decision uses the node's console state instead of the client's. The client already captures the real terminal's capabilities into aTargetConsoleConfigurationand transmits them (stored asConsoleConfiguration.Provider), but the query never consulted it.Changes
src/Framework/NativeMethods.cs— addConsoleConfigurationOverride. When set,QueryIsScreenAndTryEnableAnsiColorCodesreturns the client's transmitted screen/ANSI capabilities andnullfor the original console mode (the node must not change/restore its own console).src/Build/BackEnd/Node/OutOfProcServerNode.cs— set the override fromcommand.ConsoleConfigurationalongsideConsoleConfiguration.Provider, and clear it in afinally.This is a single shared seam:
XMake,TerminalLogger.CreateTerminalOrConsoleLogger*, andSimpleErrorLoggerall call the query and become correct under the server with no further changes. In-proc builds are unaffected (override isnull). It also corrects the Windows> file.txtcase.The override is set unconditionally from each build's own transmitted configuration before the build reads it, so a long-lived, reused server node never observes stale state across builds.
No ChangeWave: this is a correctness fix that aligns server behavior with the non-server (and documented default-TL) behavior, in an already opt-in scenario, with no new warnings/errors.
Testing
src/Framework.UnitTests/NativeMethods_Tests.cs— deterministic tests that the query honorsConsoleConfigurationOverride(incl.useStandardError), withnulloriginal console mode.src/Build.UnitTests/TerminalLogger_Tests.cs— positive regression test driving the realQueryIsScreenAndTryEnableAnsiColorCodespath via the override: a screen-capable transmitted config auto-selectsTerminalLogger; a redirected one selectsConsoleLogger. Verified it fails without the fix (returnsConsoleLogger) and passes with it.src/MSBuild.UnitTests/MSBuildServer_Tests.cs— E2E guard that-tl:autois not selected when the server build's output is redirected.Manually verified end-to-end on WSL (Ubuntu) under a sized pseudo-terminal:
-tl:auto-tl:auto-tl:onAlso confirmed that four alternating builds (TL-expected / not-expected) on the same reused server node each select the correct logger — no stale-state leakage.
Notes
The user-visible behavior requires a real TTY to observe, which headless CI cannot easily provide; the deterministic unit tests above exercise the exact fix via the same override seam the server node uses, without needing a terminal.