Make TerminalLogger capable of acting as a ForwardingLogger, and use it to implement SourceRoot-based relative paths for out-of-tree projects

[baronfel]

Fixes #9806

Context

This is another take on making TL a forwarding logger. I've gotten it much better this time - it's working in multi-node setups consistently on my machine, and I was able to get the Target Outputs for targets that I care about after a bit of work. We can make this more efficient be filtering down the Target Outputs at the forwarding level maybe? Yep! That was easy enough to do.

Changes Made

• Made a Forwarding Terminal Logger
• Hooked it up in the distributed logger registration for multi node scenarios
• Plumbed through a knob for enabling Target Outputs to be logged in the build parameters, node configuration, logging server, etc. and toggled that knob on for TL-using scenarios
• Used this infra to accurately get the Source Roots for the projects, so that when builds happen outside of the CWD and inside the Source Root we can render a relative path between the CWD and the Source Root as the 'anchor' for the project outputs
• From this base we can do many of the data-based enhancements (NuGet package detection?!) that we've wanted to do.

Testing

Manual testing - the test case is building a solution from a project that is a sibling of that solution (so same parent, but different directory), building with /m:8.

Notes

TODO:

• Tests
• Perf comparison? This will likely be a mixed bag because we should be emitting fewer events from the child loggers, but some of the events may contain more data.
• Reviews/cleanup

[Copilot]

Pull Request Overview

This pull request extends the TerminalLogger functionality to act as a ForwardingLogger and introduces support for SourceRoot-based relative paths for out‐of‐tree projects while propagating a new enableTargetOutputLogging flag throughout the build system.

• Added an optional enableTargetOutputLogging parameter to several project building methods and integrated it into project collection and logging service instantiation.
• Introduced a new SourceRoot property in TerminalProjectInfo and updated TerminalLogger to choose a relative display path based on the working directory or source root.
• Updated tests and configuration in various components, including the Distributed Logger registration, node configuration, and command-line switch handling.

Reviewed Changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/UnitTests.Shared/ObjectModelHelpers.cs	Added a parameter for target output logging to build helper methods.
src/MSBuild/XMake.cs	Propagated the new logging flag and introduced a specialized forwarding logger record.
src/Build/Logging/TerminalLogger/TerminalLogger.cs	Updated path rendering logic to support relative paths via SourceRoot.
src/Build/Definition/ProjectCollection.cs	Added enableTargetOutputLogging support and event triggering in the properties.
src/Build/BackEnd/Node/*.cs and Logging components	Updated logging service, node configuration and related tests to support the new flag.
src/Build.UnitTests/*	Converted tests to [Theory]/[InlineData] and updated target outputs verification.
src/Build.UnitTests/BackEnd/MockLoggingService.cs	Updated mock implementation to include the logging flag (returns fixed false).

Comments suppressed due to low confidence (1)

src/UnitTests.Shared/ObjectModelHelpers.cs:1343

• [nitpick] Consider updating the XML documentation of BuildProjectWithNewOMExpectSuccess to describe the new enableTargetOutputLogging parameter and its effect on the build behavior.

        public static MockLogger BuildProjectWithNewOMExpectSuccess(string content, Dictionary<string, string> globalProperties = null, MockLogger logger = null, bool enableTargetOutputLogging = false)

[SimaTian]

I've run into an issue. Long story short, I'm having trouble with validating if the behavior of the environment variable stays consistent with what was happening before:

In logging TargetLoggingContext.cs
Before, the s_enableTargetOutputLogging was directly affected by the environment variable.

Now, the same behavior depends on
LoggingService.EnableTargetOutputLogging
which as you state here: https://github.com/dotnet/msbuild/pull/12082/files#r2191360984
can be set up from many places.

So to validate the consistency, it is now necessary to check if all the previous endpoints are referencing both the property(which is new, so it won't break anyone) and the old env var.

I understand and support the addition of command line parameter.
Moving from environment variable to Trait that is populated by the environment variable is good.

With that in mind, can we keep both the old tests(with env var) and the new tests going forward?
I'm not sure what the probability of someone depending on the env var is but it could be a pain if we break someone because he depended on it (and we failed to fully transfer the behavior)

Alternatively we could do
if (!LoggingService.OnlyLogCriticalEvents && (LoggingService.EnableTargetOutputLogging || Traits.Instance.EnableTargetOutputLogging) && targetOutputs != null)
and call it good enough. Although I admit it's kind of ugly.

Initially I though "this is due to the PR being too big, we need to split it". However after some consideration, I've come to conclusion that the core issue would stay the same even in a split scenario, so it won't actually help.

[baronfel]

Good call @SimaTian - I'll see if I can flow the Trait check into a central location to preserve the existing behavior as a fallback.

[baronfel]

Alternatively we could do if (!LoggingService.OnlyLogCriticalEvents && (LoggingService.EnableTargetOutputLogging || Traits.Instance.EnableTargetOutputLogging) && targetOutputs != null) and call it good enough. Although I admit it's kind of ugly.

It may be ugly, but it's also the most central location I could find! I've added new permutations of the tests that now also stress the environment variable pathway so that we have parity. Note now that if a user set the env var explicitly to false while the TL was configured, we would still emit the events due to the TL requiring them.

[SimaTian]

I think the new tests (because of the env var)

This plagues my sidecar tests that are based on env var as well. I still don't have a reasonable solution so I've had to disable the tests in the meantime, which is annoying.
The behavior is mostly the same - locally all green, in pipelines quite flaky, no matter how I attempt to force any sort of decent behavior. If you figure anything out, please let me know. I will also look into it since it is vexing.

[baronfel]

The failing tests are green on my machine - I don't have a theory for the failures yet.

[baronfel]

I dug in a bit, and the pipelines are using the cibuild_bootstrapped_msbuild scripts to run the tests, so I did the same locally. Those tests also worked. I can only imaging that there's some kind of pinned/broken state here? Maybe I can try resetting the trait explicitly in the tests?

[SimaTian]

The issue I've raised has been addressed and rest was(and continues to be) fine.
Thank you @YuliiaKovalova for resolving the test issues.