Revert Emit eval props if requested by any sink (#10243)

documentation/wiki/ChangeWaves.md

@@ -28,7 +28,6 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t
 - [Convert.ToString during a property evaluation uses the InvariantCulture for all types](https://github.com/dotnet/msbuild/pull/9874)
 - [Fix oversharing of build results in ResultsCache](https://github.com/dotnet/msbuild/pull/9987)
 - [Add ParameterName and PropertyName to TaskParameterEventArgs](https://github.com/dotnet/msbuild/pull/10130)
-- [Emit eval props if requested by any sink](https://github.com/dotnet/msbuild/pull/10243)
 
 ### 17.10
 - [AppDomain configuration is serialized without using BinFmt](https://github.com/dotnet/msbuild/pull/9320) - feature can be opted out only if [BinaryFormatter](https://learn.microsoft.com/en-us/dotnet/api/system.runtime.serialization.formatters.binary.binaryformatter) is allowed at runtime by editing `MSBuild.runtimeconfig.json`

src/Build.UnitTests/BackEnd/BuildManager_Tests.cs

@@ -82,7 +82,7 @@ public BuildManager_Tests(ITestOutputHelper output)
                 EnableNodeReuse = false
             };
             _buildManager = new BuildManager();
-            _projectCollection = new ProjectCollection(globalProperties: null, _parameters.Loggers, ToolsetDefinitionLocations.Default);
+            _projectCollection = new ProjectCollection();
 
             _env = TestEnvironment.Create(output);
             _inProcEnvCheckTransientEnvironmentVariable = _env.SetEnvironmentVariable("MSBUILDINPROCENVCHECK", "1");
@@ -137,8 +137,8 @@ public void SimpleBuild()
             _logger.AssertLogContains("[success]");
             Assert.Single(_logger.ProjectStartedEvents);
 
-            ProjectEvaluationFinishedEventArgs evalFinishedEvent = _logger.EvaluationFinishedEvents[0];
-            Dictionary<string, string> properties = ExtractProjectStartedPropertyList(evalFinishedEvent.Properties);
+            ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
+            Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
 
             Assert.True(properties.TryGetValue("InitialProperty1", out string propertyValue));
             Assert.Equal("InitialProperty1", propertyValue);
@@ -254,8 +254,8 @@ public void SimpleGraphBuild()
             _logger.AssertLogContains("[success]");
             _logger.ProjectStartedEvents.Count.ShouldBe(1);
 
-            ProjectEvaluationFinishedEventArgs evalFinishedEvent = _logger.EvaluationFinishedEvents[0];
-            Dictionary<string, string> properties = ExtractProjectStartedPropertyList(evalFinishedEvent.Properties);
+            ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
+            Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
 
             properties.TryGetValue("InitialProperty1", out string propertyValue).ShouldBeTrue();
             propertyValue.ShouldBe("InitialProperty1", StringCompareShould.IgnoreCase);
@@ -571,8 +571,8 @@ public void InProcForwardPropertiesFromChild()
             _logger.AssertLogContains("[success]");
             Assert.Single(_logger.ProjectStartedEvents);
 
-            ProjectEvaluationFinishedEventArgs evalFinishedEvent = _logger.EvaluationFinishedEvents[0];
-            Dictionary<string, string> properties = ExtractProjectStartedPropertyList(evalFinishedEvent.Properties);
+            ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
+            Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
 
             Assert.True(properties.TryGetValue("InitialProperty1", out string propertyValue));
             Assert.Equal("InitialProperty1", propertyValue);
@@ -611,8 +611,8 @@ public void InProcMsBuildForwardAllPropertiesFromChild()
             _logger.AssertLogContains("[success]");
             Assert.Single(_logger.ProjectStartedEvents);
 
-            ProjectEvaluationFinishedEventArgs evalFinishedEvent = _logger.EvaluationFinishedEvents[0];
-            Dictionary<string, string> properties = ExtractProjectStartedPropertyList(evalFinishedEvent.Properties);
+            ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
+            Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
 
             Assert.True(properties.TryGetValue("InitialProperty1", out string propertyValue));
             Assert.Equal("InitialProperty1", propertyValue);
@@ -655,8 +655,8 @@ public void MsBuildForwardAllPropertiesFromChildLaunchChildNode()
             _logger.AssertLogContains("[success]");
             Assert.Single(_logger.ProjectStartedEvents);
 
-            ProjectEvaluationFinishedEventArgs evalFinishedEvent = _logger.EvaluationFinishedEvents[0];
-            Dictionary<string, string> properties = ExtractProjectStartedPropertyList(evalFinishedEvent.Properties);
+            ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
+            Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
 
             Assert.True(properties.TryGetValue("InitialProperty1", out string propertyValue));
             Assert.Equal("InitialProperty1", propertyValue);
@@ -704,15 +704,7 @@ public void OutOfProcNodeForwardCertainproperties()
             var data = new BuildRequestData(project.FullPath, new Dictionary<string, string>(),
                 MSBuildDefaultToolsVersion, Array.Empty<string>(), null);
 
-            // We need to recreate build parameters to ensure proper capturing of newly set environment variables
-            BuildParameters parameters = new BuildParameters
-            {
-                ShutdownInProcNodeOnBuildFinish = true,
-                Loggers = new ILogger[] { _logger },
-                EnableNodeReuse = false
-            };
-
-            BuildResult result = _buildManager.Build(parameters, data);
+            BuildResult result = _buildManager.Build(_parameters, data);
             Assert.Equal(BuildResultCode.Success, result.OverallResult);
             _logger.AssertLogContains("[success]");
             Assert.Single(_logger.ProjectStartedEvents);
@@ -768,21 +760,11 @@ public void OutOfProcNodeForwardCertainpropertiesAlsoGetResultsFromCache()
             _env.SetEnvironmentVariable("MsBuildForwardPropertiesFromChild", "InitialProperty3;IAMNOTREAL");
             _env.SetEnvironmentVariable("MSBUILDNOINPROCNODE", "1");
 
-            _env.SetEnvironmentVariable("MSBUILDLOGPROPERTIESANDITEMSAFTEREVALUATION", "0");
-
             var project = CreateProject(contents, null, _projectCollection, false);
             var data = new BuildRequestData(project.FullPath, new Dictionary<string, string>(),
                 MSBuildDefaultToolsVersion, Array.Empty<string>(), null);
 
-            // We need to recreate build parameters to ensure proper capturing of newly set environment variables
-            BuildParameters parameters = new BuildParameters
-            {
-                ShutdownInProcNodeOnBuildFinish = true,
-                Loggers = new ILogger[] { _logger },
-                EnableNodeReuse = false
-            };
-
-            BuildResult result = _buildManager.Build(parameters, data);
+            BuildResult result = _buildManager.Build(_parameters, data);
             Assert.Equal(BuildResultCode.Success, result.OverallResult);
             _logger.AssertLogContains("[success]");
             Assert.Equal(3, _logger.ProjectStartedEvents.Count);
@@ -803,8 +785,7 @@ public void OutOfProcNodeForwardCertainpropertiesAlsoGetResultsFromCache()
             Assert.Equal("InitialProperty3", propertyValue);
 
             projectStartedEvent = _logger.ProjectStartedEvents[2];
-            properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
-            (properties == null || properties.Count == 0).ShouldBeTrue();
+            Assert.Null(projectStartedEvent.Properties);
         }
 
         /// <summary>
@@ -841,7 +822,7 @@ public void ForwardNoPropertiesLaunchChildNode()
 
             ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
             Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
-            (properties == null || properties.Count == 0).ShouldBeTrue();
+            Assert.Null(properties);
         }
 
         /// <summary>
@@ -938,7 +919,7 @@ public void ForwardNoPropertiesLaunchChildNodeDefault()
 
             ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
             Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
-            (properties == null || properties.Count == 0).ShouldBeTrue();
+            Assert.Null(properties);
         }
 
         /// <summary>
@@ -3494,11 +3475,9 @@ private static string BuildAndCheckCache(BuildManager localBuildManager, IEnumer
         /// </summary>
         private static Dictionary<string, string> ExtractProjectStartedPropertyList(IEnumerable properties)
         {
-            Dictionary<string, string> propertiesLookup = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
-            Internal.Utilities.EnumerateProperties(properties, propertiesLookup,
-                static (dict, kvp) => dict.Add(kvp.Key, kvp.Value));
-
-            return propertiesLookup;
+            // Gather a sorted list of all the properties.
+            return properties?.Cast<DictionaryEntry>()
+                .ToDictionary(prop => (string)prop.Key, prop => (string)prop.Value, StringComparer.OrdinalIgnoreCase);
         }
 
         /// <summary>

src/Build.UnitTests/BackEnd/MockLoggingService.cs

@@ -222,21 +222,14 @@ public bool IncludeEvaluationProfile
             set { }
         }
 
-        /// <inheritdoc cref="ILoggingService.SetIncludeEvaluationPropertiesAndItemsInEvents"/>
-        public void SetIncludeEvaluationPropertiesAndItemsInEvents(bool inProjectStartedEvent,
-            bool inEvaluationFinishedEvent)
-        { }
-
-        /// <inheritdoc cref="ILoggingService.IncludeEvaluationPropertiesAndItemsInProjectStartedEvent"/>
-        public bool IncludeEvaluationPropertiesAndItemsInProjectStartedEvent
-        {
-            get => false;
-        }
-
-        /// <inheritdoc cref="ILoggingService.IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent"/>
-        public bool IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent
+        /// <summary>
+        /// Log properties and items on ProjectEvaluationFinishedEventArgs
+        /// instead of ProjectStartedEventArgs.
+        /// </summary>
+        public bool IncludeEvaluationPropertiesAndItems
         {
             get => false;
+            set { }
         }
 
         /// <summary>

src/Build.UnitTests/ConsoleLogger_Tests.cs

@@ -277,7 +277,7 @@ public void ErrorMessageWithMultiplePropertiesInMessage(bool includeEvaluationPr
 
             if (includeEvaluationPropertiesAndItems)
             {
-                pc.Collection.LoggingService.SetIncludeEvaluationPropertiesAndItemsInEvents(inProjectStartedEvent: false, inEvaluationFinishedEvent: true);
+                pc.Collection.LoggingService.IncludeEvaluationPropertiesAndItems = true;
             }
 
             var project = env.CreateTestProjectWithFiles(@"

src/Build/BackEnd/BuildManager/BuildManager.cs

@@ -2757,8 +2757,7 @@ private NodeConfiguration GetNodeConfiguration()
                 , new LoggingNodeConfiguration(
                     loggingService.IncludeEvaluationMetaprojects,
                     loggingService.IncludeEvaluationProfile,
-                    loggingService.IncludeEvaluationPropertiesAndItemsInProjectStartedEvent,
-                    loggingService.IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent,
+                    loggingService.IncludeEvaluationPropertiesAndItems,
                     loggingService.IncludeTaskInputs));
             }
 

src/Build/BackEnd/Components/Logging/ILoggingService.cs

@@ -206,24 +206,12 @@ bool IncludeEvaluationProfile
 
         /// <summary>
         /// Should properties and items be logged on <see cref="ProjectEvaluationFinishedEventArgs"/>
-        /// or/and <see cref="ProjectStartedEventArgs"/>?
+        /// instead of <see cref="ProjectStartedEventArgs"/>?
         /// </summary>
-        void SetIncludeEvaluationPropertiesAndItemsInEvents(bool inProjectStartedEvent, bool inEvaluationFinishedEvent);
-
-        /// <summary>
-        /// Indicates whether properties and items should be logged on <see cref="ProjectStartedEventArgs"/>.
-        /// </summary>
-        bool IncludeEvaluationPropertiesAndItemsInProjectStartedEvent
-        {
-            get;
-        }
-
-        /// <summary>
-        /// Indicates whether properties and items should be logged on <see cref="ProjectEvaluationFinishedEventArgs"/>.
-        /// </summary>
-        bool IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent
+        bool IncludeEvaluationPropertiesAndItems
         {
             get;
+            set;
         }
 
         /// <summary>

src/Build/BackEnd/Components/Logging/LoggingService.cs

@@ -201,6 +201,12 @@ internal partial class LoggingService : ILoggingService, INodePacketHandler
         /// </summary>
         private bool? _includeEvaluationProfile;
 
+        /// <summary>
+        /// Whether properties and items should be logged on <see cref="ProjectEvaluationFinishedEventArgs"/>
+        /// instead of <see cref="ProjectStartedEventArgs"/>.
+        /// </summary>
+        private bool? _includeEvaluationPropertiesAndItems;
+
         /// <summary>
         /// Whether to include task inputs in task events.
         /// </summary>
@@ -540,77 +546,33 @@ public bool IncludeTaskInputs
             set => _includeTaskInputs = value;
         }
 
-        /// <inheritdoc cref="ILoggingService.SetIncludeEvaluationPropertiesAndItemsInEvents"/>
-        public void SetIncludeEvaluationPropertiesAndItemsInEvents(bool inProjectStartedEvent, bool inEvaluationFinishedEvent)
-        {
-            _evalDataBehaviorSet = true;
-            IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent = inEvaluationFinishedEvent;
-            IncludeEvaluationPropertiesAndItemsInProjectStartedEvent = inProjectStartedEvent;
-        }
-
-        private bool _evalDataBehaviorSet;
-        private bool _includeEvaluationPropertiesAndItemsInProjectStartedEvent;
-        private bool _includeEvaluationPropertiesAndItemsInEvaluationFinishedEvent;
-        private void InferEvalDataBehavior()
+        /// <summary>
+        /// Should properties and items be logged on <see cref="ProjectEvaluationFinishedEventArgs"/>
+        /// instead of <see cref="ProjectStartedEventArgs"/>?
+        /// </summary>
+        public bool IncludeEvaluationPropertiesAndItems
         {
-            if (_evalDataBehaviorSet)
-            {
-                return;
-            }
-            // Set this right away - to prevent SO exception in case of any future refactoring
-            //  that would refer to the IncludeEvaluation... properties here
-            _evalDataBehaviorSet = true;
-
-            bool? escapeHatch = Traits.Instance.EscapeHatches.LogPropertiesAndItemsAfterEvaluation;
-            if (escapeHatch.HasValue)
-            {
-                IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent = escapeHatch.Value;
-                IncludeEvaluationPropertiesAndItemsInProjectStartedEvent = !escapeHatch.Value;
-            }
-            else
+            get
             {
-                var sinks = _eventSinkDictionary.Values.OfType<EventSourceSink>().ToList();
-
-                if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_12))
-                {
-                    // If any logger requested the data - we need to emit them
-                    IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent =
-                        sinks.Any(sink => sink.IncludeEvaluationPropertiesAndItems);
-                    // If any logger didn't request the data - hence it's likely legacy logger
-                    //  - we need to populate the data in legacy way
-                    IncludeEvaluationPropertiesAndItemsInProjectStartedEvent =
-                        sinks.Any(sink => !sink.IncludeEvaluationPropertiesAndItems);
-                }
-                else
+                if (_includeEvaluationPropertiesAndItems == null)
                 {
-                    bool allSinksIncludeEvalData = sinks.Any() && sinks.All(sink => sink.IncludeEvaluationPropertiesAndItems);
-
-                    IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent = allSinksIncludeEvalData;
-                    IncludeEvaluationPropertiesAndItemsInProjectStartedEvent = !allSinksIncludeEvalData;
+                    var escapeHatch = Traits.Instance.EscapeHatches.LogPropertiesAndItemsAfterEvaluation;
+                    if (escapeHatch.HasValue)
+                    {
+                        _includeEvaluationPropertiesAndItems = escapeHatch.Value;
+                    }
+                    else
+                    {
+                        var sinks = _eventSinkDictionary.Values.OfType<EventSourceSink>();
+                        // .All() on an empty list defaults to true, we want to default to false
+                        _includeEvaluationPropertiesAndItems = sinks.Any() && sinks.All(sink => sink.IncludeEvaluationPropertiesAndItems);
+                    }
                 }
-            }
-        }
 
-        /// <inheritdoc cref="ILoggingService.IncludeEvaluationPropertiesAndItemsInProjectStartedEvent"/>
-        public bool IncludeEvaluationPropertiesAndItemsInProjectStartedEvent
-        {
-            get
-            {
-                InferEvalDataBehavior();
-                return _includeEvaluationPropertiesAndItemsInProjectStartedEvent;
+                return _includeEvaluationPropertiesAndItems ?? false;
             }
-            private set => _includeEvaluationPropertiesAndItemsInProjectStartedEvent = value;
-        }
 
-        /// <inheritdoc cref="ILoggingService.IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent"/>
-        public bool IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent
-        {
-            get
-            {
-                InferEvalDataBehavior();
-                return _includeEvaluationPropertiesAndItemsInEvaluationFinishedEvent;
-            }
-            private set => _includeEvaluationPropertiesAndItemsInEvaluationFinishedEvent = value;
+            set => _includeEvaluationPropertiesAndItems = value;
         }
 
         /// <summary>
@@ -652,7 +614,6 @@ public ICollection<string> GetWarningsNotAsErrors(BuildEventContext context)
             return GetWarningsForProject(context, _warningsNotAsErrorsByProject, WarningsNotAsErrors);
         }
 
-
         /// <summary>
         /// Returns a collection of warnings to be demoted to messages for the specified build context.
         /// </summary>

src/Build/BackEnd/Components/Logging/ProjectLoggingContext.cs

@@ -132,7 +132,7 @@ private static BuildEventContext CreateInitialContext(
 
             // If we are only logging critical events lets not pass back the items or properties
             if (!loggingService.OnlyLogCriticalEvents &&
-                loggingService.IncludeEvaluationPropertiesAndItemsInProjectStartedEvent &&
+                !loggingService.IncludeEvaluationPropertiesAndItems &&
                 (!loggingService.RunningOnRemoteNode || loggingService.SerializeAllProperties))
             {
                 if (projectProperties is null)
@@ -152,7 +152,7 @@ private static BuildEventContext CreateInitialContext(
             }
 
             if (projectProperties != null &&
-                loggingService.IncludeEvaluationPropertiesAndItemsInProjectStartedEvent &&
+                !loggingService.IncludeEvaluationPropertiesAndItems &&
                 propertiesToSerialize?.Length > 0 &&
                 !loggingService.SerializeAllProperties)
             {