Skip to content

Commit bd46115

Browse files
JanKrivanekJanKrivanek
authored
Revert Emit eval props if requested by any sink (#10243) (#10447)
1 parent b381fcd commit bd46115

15 files changed

Lines changed: 93 additions & 176 deletions

File tree

‎documentation/wiki/ChangeWaves.md‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t
2828
- [Convert.ToString during a property evaluation uses the InvariantCulture for all types](https://github.com/dotnet/msbuild/pull/9874)
2929
- [Fix oversharing of build results in ResultsCache](https://github.com/dotnet/msbuild/pull/9987)
3030
- [Add ParameterName and PropertyName to TaskParameterEventArgs](https://github.com/dotnet/msbuild/pull/10130)
31-
- [Emit eval props if requested by any sink](https://github.com/dotnet/msbuild/pull/10243)
3231

3332
### 17.10
3433
- [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‎

Lines changed: 19 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ public BuildManager_Tests(ITestOutputHelper output)
8282
EnableNodeReuse = false
8383
};
8484
_buildManager = new BuildManager();
85-
_projectCollection = new ProjectCollection(globalProperties: null, _parameters.Loggers, ToolsetDefinitionLocations.Default);
85+
_projectCollection = new ProjectCollection();
8686

8787
_env = TestEnvironment.Create(output);
8888
_inProcEnvCheckTransientEnvironmentVariable = _env.SetEnvironmentVariable("MSBUILDINPROCENVCHECK", "1");
@@ -137,8 +137,8 @@ public void SimpleBuild()
137137
_logger.AssertLogContains("[success]");
138138
Assert.Single(_logger.ProjectStartedEvents);
139139

140-
ProjectEvaluationFinishedEventArgs evalFinishedEvent = _logger.EvaluationFinishedEvents[0];
141-
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(evalFinishedEvent.Properties);
140+
ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
141+
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
142142

143143
Assert.True(properties.TryGetValue("InitialProperty1", out string propertyValue));
144144
Assert.Equal("InitialProperty1", propertyValue);
@@ -254,8 +254,8 @@ public void SimpleGraphBuild()
254254
_logger.AssertLogContains("[success]");
255255
_logger.ProjectStartedEvents.Count.ShouldBe(1);
256256

257-
ProjectEvaluationFinishedEventArgs evalFinishedEvent = _logger.EvaluationFinishedEvents[0];
258-
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(evalFinishedEvent.Properties);
257+
ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
258+
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
259259

260260
properties.TryGetValue("InitialProperty1", out string propertyValue).ShouldBeTrue();
261261
propertyValue.ShouldBe("InitialProperty1", StringCompareShould.IgnoreCase);
@@ -571,8 +571,8 @@ public void InProcForwardPropertiesFromChild()
571571
_logger.AssertLogContains("[success]");
572572
Assert.Single(_logger.ProjectStartedEvents);
573573

574-
ProjectEvaluationFinishedEventArgs evalFinishedEvent = _logger.EvaluationFinishedEvents[0];
575-
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(evalFinishedEvent.Properties);
574+
ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
575+
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
576576

577577
Assert.True(properties.TryGetValue("InitialProperty1", out string propertyValue));
578578
Assert.Equal("InitialProperty1", propertyValue);
@@ -611,8 +611,8 @@ public void InProcMsBuildForwardAllPropertiesFromChild()
611611
_logger.AssertLogContains("[success]");
612612
Assert.Single(_logger.ProjectStartedEvents);
613613

614-
ProjectEvaluationFinishedEventArgs evalFinishedEvent = _logger.EvaluationFinishedEvents[0];
615-
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(evalFinishedEvent.Properties);
614+
ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
615+
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
616616

617617
Assert.True(properties.TryGetValue("InitialProperty1", out string propertyValue));
618618
Assert.Equal("InitialProperty1", propertyValue);
@@ -655,8 +655,8 @@ public void MsBuildForwardAllPropertiesFromChildLaunchChildNode()
655655
_logger.AssertLogContains("[success]");
656656
Assert.Single(_logger.ProjectStartedEvents);
657657

658-
ProjectEvaluationFinishedEventArgs evalFinishedEvent = _logger.EvaluationFinishedEvents[0];
659-
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(evalFinishedEvent.Properties);
658+
ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
659+
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
660660

661661
Assert.True(properties.TryGetValue("InitialProperty1", out string propertyValue));
662662
Assert.Equal("InitialProperty1", propertyValue);
@@ -704,15 +704,7 @@ public void OutOfProcNodeForwardCertainproperties()
704704
var data = new BuildRequestData(project.FullPath, new Dictionary<string, string>(),
705705
MSBuildDefaultToolsVersion, Array.Empty<string>(), null);
706706

707-
// We need to recreate build parameters to ensure proper capturing of newly set environment variables
708-
BuildParameters parameters = new BuildParameters
709-
{
710-
ShutdownInProcNodeOnBuildFinish = true,
711-
Loggers = new ILogger[] { _logger },
712-
EnableNodeReuse = false
713-
};
714-
715-
BuildResult result = _buildManager.Build(parameters, data);
707+
BuildResult result = _buildManager.Build(_parameters, data);
716708
Assert.Equal(BuildResultCode.Success, result.OverallResult);
717709
_logger.AssertLogContains("[success]");
718710
Assert.Single(_logger.ProjectStartedEvents);
@@ -768,21 +760,11 @@ public void OutOfProcNodeForwardCertainpropertiesAlsoGetResultsFromCache()
768760
_env.SetEnvironmentVariable("MsBuildForwardPropertiesFromChild", "InitialProperty3;IAMNOTREAL");
769761
_env.SetEnvironmentVariable("MSBUILDNOINPROCNODE", "1");
770762

771-
_env.SetEnvironmentVariable("MSBUILDLOGPROPERTIESANDITEMSAFTEREVALUATION", "0");
772-
773763
var project = CreateProject(contents, null, _projectCollection, false);
774764
var data = new BuildRequestData(project.FullPath, new Dictionary<string, string>(),
775765
MSBuildDefaultToolsVersion, Array.Empty<string>(), null);
776766

777-
// We need to recreate build parameters to ensure proper capturing of newly set environment variables
778-
BuildParameters parameters = new BuildParameters
779-
{
780-
ShutdownInProcNodeOnBuildFinish = true,
781-
Loggers = new ILogger[] { _logger },
782-
EnableNodeReuse = false
783-
};
784-
785-
BuildResult result = _buildManager.Build(parameters, data);
767+
BuildResult result = _buildManager.Build(_parameters, data);
786768
Assert.Equal(BuildResultCode.Success, result.OverallResult);
787769
_logger.AssertLogContains("[success]");
788770
Assert.Equal(3, _logger.ProjectStartedEvents.Count);
@@ -803,8 +785,7 @@ public void OutOfProcNodeForwardCertainpropertiesAlsoGetResultsFromCache()
803785
Assert.Equal("InitialProperty3", propertyValue);
804786

805787
projectStartedEvent = _logger.ProjectStartedEvents[2];
806-
properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
807-
(properties == null || properties.Count == 0).ShouldBeTrue();
788+
Assert.Null(projectStartedEvent.Properties);
808789
}
809790

810791
/// <summary>
@@ -841,7 +822,7 @@ public void ForwardNoPropertiesLaunchChildNode()
841822

842823
ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
843824
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
844-
(properties == null || properties.Count == 0).ShouldBeTrue();
825+
Assert.Null(properties);
845826
}
846827

847828
/// <summary>
@@ -938,7 +919,7 @@ public void ForwardNoPropertiesLaunchChildNodeDefault()
938919

939920
ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
940921
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
941-
(properties == null || properties.Count == 0).ShouldBeTrue();
922+
Assert.Null(properties);
942923
}
943924

944925
/// <summary>
@@ -3494,11 +3475,9 @@ private static string BuildAndCheckCache(BuildManager localBuildManager, IEnumer
34943475
/// </summary>
34953476
private static Dictionary<string, string> ExtractProjectStartedPropertyList(IEnumerable properties)
34963477
{
3497-
Dictionary<string, string> propertiesLookup = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
3498-
Internal.Utilities.EnumerateProperties(properties, propertiesLookup,
3499-
static (dict, kvp) => dict.Add(kvp.Key, kvp.Value));
3500-
3501-
return propertiesLookup;
3478+
// Gather a sorted list of all the properties.
3479+
return properties?.Cast<DictionaryEntry>()
3480+
.ToDictionary(prop => (string)prop.Key, prop => (string)prop.Value, StringComparer.OrdinalIgnoreCase);
35023481
}
35033482

35043483
/// <summary>

‎src/Build.UnitTests/BackEnd/MockLoggingService.cs‎

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -222,21 +222,14 @@ public bool IncludeEvaluationProfile
222222
set { }
223223
}
224224

225-
/// <inheritdoc cref="ILoggingService.SetIncludeEvaluationPropertiesAndItemsInEvents"/>
226-
public void SetIncludeEvaluationPropertiesAndItemsInEvents(bool inProjectStartedEvent,
227-
bool inEvaluationFinishedEvent)
228-
{ }
229-
230-
/// <inheritdoc cref="ILoggingService.IncludeEvaluationPropertiesAndItemsInProjectStartedEvent"/>
231-
public bool IncludeEvaluationPropertiesAndItemsInProjectStartedEvent
232-
{
233-
get => false;
234-
}
235-
236-
/// <inheritdoc cref="ILoggingService.IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent"/>
237-
public bool IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent
225+
/// <summary>
226+
/// Log properties and items on ProjectEvaluationFinishedEventArgs
227+
/// instead of ProjectStartedEventArgs.
228+
/// </summary>
229+
public bool IncludeEvaluationPropertiesAndItems
238230
{
239231
get => false;
232+
set { }
240233
}
241234

242235
/// <summary>

‎src/Build.UnitTests/ConsoleLogger_Tests.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ public void ErrorMessageWithMultiplePropertiesInMessage(bool includeEvaluationPr
277277

278278
if (includeEvaluationPropertiesAndItems)
279279
{
280-
pc.Collection.LoggingService.SetIncludeEvaluationPropertiesAndItemsInEvents(inProjectStartedEvent: false, inEvaluationFinishedEvent: true);
280+
pc.Collection.LoggingService.IncludeEvaluationPropertiesAndItems = true;
281281
}
282282

283283
var project = env.CreateTestProjectWithFiles(@"

‎src/Build/BackEnd/BuildManager/BuildManager.cs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2757,8 +2757,7 @@ private NodeConfiguration GetNodeConfiguration()
27572757
, new LoggingNodeConfiguration(
27582758
loggingService.IncludeEvaluationMetaprojects,
27592759
loggingService.IncludeEvaluationProfile,
2760-
loggingService.IncludeEvaluationPropertiesAndItemsInProjectStartedEvent,
2761-
loggingService.IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent,
2760+
loggingService.IncludeEvaluationPropertiesAndItems,
27622761
loggingService.IncludeTaskInputs));
27632762
}
27642763

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

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -206,24 +206,12 @@ bool IncludeEvaluationProfile
206206

207207
/// <summary>
208208
/// Should properties and items be logged on <see cref="ProjectEvaluationFinishedEventArgs"/>
209-
/// or/and <see cref="ProjectStartedEventArgs"/>?
209+
/// instead of <see cref="ProjectStartedEventArgs"/>?
210210
/// </summary>
211-
void SetIncludeEvaluationPropertiesAndItemsInEvents(bool inProjectStartedEvent, bool inEvaluationFinishedEvent);
212-
213-
/// <summary>
214-
/// Indicates whether properties and items should be logged on <see cref="ProjectStartedEventArgs"/>.
215-
/// </summary>
216-
bool IncludeEvaluationPropertiesAndItemsInProjectStartedEvent
217-
{
218-
get;
219-
}
220-
221-
/// <summary>
222-
/// Indicates whether properties and items should be logged on <see cref="ProjectEvaluationFinishedEventArgs"/>.
223-
/// </summary>
224-
bool IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent
211+
bool IncludeEvaluationPropertiesAndItems
225212
{
226213
get;
214+
set;
227215
}
228216

229217
/// <summary>

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

Lines changed: 26 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,12 @@ internal partial class LoggingService : ILoggingService, INodePacketHandler
201201
/// </summary>
202202
private bool? _includeEvaluationProfile;
203203

204+
/// <summary>
205+
/// Whether properties and items should be logged on <see cref="ProjectEvaluationFinishedEventArgs"/>
206+
/// instead of <see cref="ProjectStartedEventArgs"/>.
207+
/// </summary>
208+
private bool? _includeEvaluationPropertiesAndItems;
209+
204210
/// <summary>
205211
/// Whether to include task inputs in task events.
206212
/// </summary>
@@ -540,77 +546,33 @@ public bool IncludeTaskInputs
540546
set => _includeTaskInputs = value;
541547
}
542548

543-
/// <inheritdoc cref="ILoggingService.SetIncludeEvaluationPropertiesAndItemsInEvents"/>
544-
public void SetIncludeEvaluationPropertiesAndItemsInEvents(bool inProjectStartedEvent, bool inEvaluationFinishedEvent)
545-
{
546-
_evalDataBehaviorSet = true;
547-
IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent = inEvaluationFinishedEvent;
548-
IncludeEvaluationPropertiesAndItemsInProjectStartedEvent = inProjectStartedEvent;
549-
}
550-
551-
private bool _evalDataBehaviorSet;
552-
private bool _includeEvaluationPropertiesAndItemsInProjectStartedEvent;
553-
private bool _includeEvaluationPropertiesAndItemsInEvaluationFinishedEvent;
554-
private void InferEvalDataBehavior()
549+
/// <summary>
550+
/// Should properties and items be logged on <see cref="ProjectEvaluationFinishedEventArgs"/>
551+
/// instead of <see cref="ProjectStartedEventArgs"/>?
552+
/// </summary>
553+
public bool IncludeEvaluationPropertiesAndItems
555554
{
556-
if (_evalDataBehaviorSet)
557-
{
558-
return;
559-
}
560-
// Set this right away - to prevent SO exception in case of any future refactoring
561-
// that would refer to the IncludeEvaluation... properties here
562-
_evalDataBehaviorSet = true;
563-
564-
bool? escapeHatch = Traits.Instance.EscapeHatches.LogPropertiesAndItemsAfterEvaluation;
565-
if (escapeHatch.HasValue)
566-
{
567-
IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent = escapeHatch.Value;
568-
IncludeEvaluationPropertiesAndItemsInProjectStartedEvent = !escapeHatch.Value;
569-
}
570-
else
555+
get
571556
{
572-
var sinks = _eventSinkDictionary.Values.OfType<EventSourceSink>().ToList();
573-
574-
if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_12))
575-
{
576-
// If any logger requested the data - we need to emit them
577-
IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent =
578-
sinks.Any(sink => sink.IncludeEvaluationPropertiesAndItems);
579-
// If any logger didn't request the data - hence it's likely legacy logger
580-
// - we need to populate the data in legacy way
581-
IncludeEvaluationPropertiesAndItemsInProjectStartedEvent =
582-
sinks.Any(sink => !sink.IncludeEvaluationPropertiesAndItems);
583-
}
584-
else
557+
if (_includeEvaluationPropertiesAndItems == null)
585558
{
586-
bool allSinksIncludeEvalData = sinks.Any() && sinks.All(sink => sink.IncludeEvaluationPropertiesAndItems);
587-
588-
IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent = allSinksIncludeEvalData;
589-
IncludeEvaluationPropertiesAndItemsInProjectStartedEvent = !allSinksIncludeEvalData;
559+
var escapeHatch = Traits.Instance.EscapeHatches.LogPropertiesAndItemsAfterEvaluation;
560+
if (escapeHatch.HasValue)
561+
{
562+
_includeEvaluationPropertiesAndItems = escapeHatch.Value;
563+
}
564+
else
565+
{
566+
var sinks = _eventSinkDictionary.Values.OfType<EventSourceSink>();
567+
// .All() on an empty list defaults to true, we want to default to false
568+
_includeEvaluationPropertiesAndItems = sinks.Any() && sinks.All(sink => sink.IncludeEvaluationPropertiesAndItems);
569+
}
590570
}
591-
}
592-
}
593571

594-
/// <inheritdoc cref="ILoggingService.IncludeEvaluationPropertiesAndItemsInProjectStartedEvent"/>
595-
public bool IncludeEvaluationPropertiesAndItemsInProjectStartedEvent
596-
{
597-
get
598-
{
599-
InferEvalDataBehavior();
600-
return _includeEvaluationPropertiesAndItemsInProjectStartedEvent;
572+
return _includeEvaluationPropertiesAndItems ?? false;
601573
}
602-
private set => _includeEvaluationPropertiesAndItemsInProjectStartedEvent = value;
603-
}
604574

605-
/// <inheritdoc cref="ILoggingService.IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent"/>
606-
public bool IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent
607-
{
608-
get
609-
{
610-
InferEvalDataBehavior();
611-
return _includeEvaluationPropertiesAndItemsInEvaluationFinishedEvent;
612-
}
613-
private set => _includeEvaluationPropertiesAndItemsInEvaluationFinishedEvent = value;
575+
set => _includeEvaluationPropertiesAndItems = value;
614576
}
615577

616578
/// <summary>
@@ -652,7 +614,6 @@ public ICollection<string> GetWarningsNotAsErrors(BuildEventContext context)
652614
return GetWarningsForProject(context, _warningsNotAsErrorsByProject, WarningsNotAsErrors);
653615
}
654616

655-
656617
/// <summary>
657618
/// Returns a collection of warnings to be demoted to messages for the specified build context.
658619
/// </summary>

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ private static BuildEventContext CreateInitialContext(
132132

133133
// If we are only logging critical events lets not pass back the items or properties
134134
if (!loggingService.OnlyLogCriticalEvents &&
135-
loggingService.IncludeEvaluationPropertiesAndItemsInProjectStartedEvent &&
135+
!loggingService.IncludeEvaluationPropertiesAndItems &&
136136
(!loggingService.RunningOnRemoteNode || loggingService.SerializeAllProperties))
137137
{
138138
if (projectProperties is null)
@@ -152,7 +152,7 @@ private static BuildEventContext CreateInitialContext(
152152
}
153153

154154
if (projectProperties != null &&
155-
loggingService.IncludeEvaluationPropertiesAndItemsInProjectStartedEvent &&
155+
!loggingService.IncludeEvaluationPropertiesAndItems &&
156156
propertiesToSerialize?.Length > 0 &&
157157
!loggingService.SerializeAllProperties)
158158
{

0 commit comments

Comments
 (0)