Skip to content

Commit 7aeeca8

Browse files
JanKrivanekJanKrivanek
authored
Revert "Revert Emit eval props if requested by any sink (#10243) (#10447)"
This reverts commit bd46115.
1 parent b7e76d1 commit 7aeeca8

15 files changed

Lines changed: 176 additions & 93 deletions

File tree

‎documentation/wiki/ChangeWaves.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ 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)
3132

3233
### 17.10
3334
- [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: 40 additions & 19 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();
85+
_projectCollection = new ProjectCollection(globalProperties: null, _parameters.Loggers, ToolsetDefinitionLocations.Default);
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-
ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
141-
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
140+
ProjectEvaluationFinishedEventArgs evalFinishedEvent = _logger.EvaluationFinishedEvents[0];
141+
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(evalFinishedEvent.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-
ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
258-
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
257+
ProjectEvaluationFinishedEventArgs evalFinishedEvent = _logger.EvaluationFinishedEvents[0];
258+
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(evalFinishedEvent.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-
ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
575-
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
574+
ProjectEvaluationFinishedEventArgs evalFinishedEvent = _logger.EvaluationFinishedEvents[0];
575+
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(evalFinishedEvent.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-
ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
615-
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
614+
ProjectEvaluationFinishedEventArgs evalFinishedEvent = _logger.EvaluationFinishedEvents[0];
615+
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(evalFinishedEvent.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-
ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
659-
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
658+
ProjectEvaluationFinishedEventArgs evalFinishedEvent = _logger.EvaluationFinishedEvents[0];
659+
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(evalFinishedEvent.Properties);
660660

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

707-
BuildResult result = _buildManager.Build(_parameters, data);
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);
708716
Assert.Equal(BuildResultCode.Success, result.OverallResult);
709717
_logger.AssertLogContains("[success]");
710718
Assert.Single(_logger.ProjectStartedEvents);
@@ -760,11 +768,21 @@ public void OutOfProcNodeForwardCertainpropertiesAlsoGetResultsFromCache()
760768
_env.SetEnvironmentVariable("MsBuildForwardPropertiesFromChild", "InitialProperty3;IAMNOTREAL");
761769
_env.SetEnvironmentVariable("MSBUILDNOINPROCNODE", "1");
762770

771+
_env.SetEnvironmentVariable("MSBUILDLOGPROPERTIESANDITEMSAFTEREVALUATION", "0");
772+
763773
var project = CreateProject(contents, null, _projectCollection, false);
764774
var data = new BuildRequestData(project.FullPath, new Dictionary<string, string>(),
765775
MSBuildDefaultToolsVersion, Array.Empty<string>(), null);
766776

767-
BuildResult result = _buildManager.Build(_parameters, data);
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);
768786
Assert.Equal(BuildResultCode.Success, result.OverallResult);
769787
_logger.AssertLogContains("[success]");
770788
Assert.Equal(3, _logger.ProjectStartedEvents.Count);
@@ -785,7 +803,8 @@ public void OutOfProcNodeForwardCertainpropertiesAlsoGetResultsFromCache()
785803
Assert.Equal("InitialProperty3", propertyValue);
786804

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

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

823842
ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
824843
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
825-
Assert.Null(properties);
844+
(properties == null || properties.Count == 0).ShouldBeTrue();
826845
}
827846

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

920939
ProjectStartedEventArgs projectStartedEvent = _logger.ProjectStartedEvents[0];
921940
Dictionary<string, string> properties = ExtractProjectStartedPropertyList(projectStartedEvent.Properties);
922-
Assert.Null(properties);
941+
(properties == null || properties.Count == 0).ShouldBeTrue();
923942
}
924943

925944
/// <summary>
@@ -3475,9 +3494,11 @@ private static string BuildAndCheckCache(BuildManager localBuildManager, IEnumer
34753494
/// </summary>
34763495
private static Dictionary<string, string> ExtractProjectStartedPropertyList(IEnumerable properties)
34773496
{
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);
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;
34813502
}
34823503

34833504
/// <summary>

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

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

225-
/// <summary>
226-
/// Log properties and items on ProjectEvaluationFinishedEventArgs
227-
/// instead of ProjectStartedEventArgs.
228-
/// </summary>
229-
public bool IncludeEvaluationPropertiesAndItems
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
230238
{
231239
get => false;
232-
set { }
233240
}
234241

235242
/// <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.IncludeEvaluationPropertiesAndItems = true;
280+
pc.Collection.LoggingService.SetIncludeEvaluationPropertiesAndItemsInEvents(inProjectStartedEvent: false, inEvaluationFinishedEvent: true);
281281
}
282282

283283
var project = env.CreateTestProjectWithFiles(@"

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2771,7 +2771,8 @@ private NodeConfiguration GetNodeConfiguration()
27712771
, new LoggingNodeConfiguration(
27722772
loggingService.IncludeEvaluationMetaprojects,
27732773
loggingService.IncludeEvaluationProfile,
2774-
loggingService.IncludeEvaluationPropertiesAndItems,
2774+
loggingService.IncludeEvaluationPropertiesAndItemsInProjectStartedEvent,
2775+
loggingService.IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent,
27752776
loggingService.IncludeTaskInputs));
27762777
}
27772778

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

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

207207
/// <summary>
208208
/// Should properties and items be logged on <see cref="ProjectEvaluationFinishedEventArgs"/>
209-
/// instead of <see cref="ProjectStartedEventArgs"/>?
209+
/// or/and <see cref="ProjectStartedEventArgs"/>?
210210
/// </summary>
211-
bool IncludeEvaluationPropertiesAndItems
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
212225
{
213226
get;
214-
set;
215227
}
216228

217229
/// <summary>

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

Lines changed: 65 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -201,12 +201,6 @@ 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-
210204
/// <summary>
211205
/// Whether to include task inputs in task events.
212206
/// </summary>
@@ -546,33 +540,77 @@ public bool IncludeTaskInputs
546540
set => _includeTaskInputs = value;
547541
}
548542

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
543+
/// <inheritdoc cref="ILoggingService.SetIncludeEvaluationPropertiesAndItemsInEvents"/>
544+
public void SetIncludeEvaluationPropertiesAndItemsInEvents(bool inProjectStartedEvent, bool inEvaluationFinishedEvent)
554545
{
555-
get
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()
555+
{
556+
if (_evalDataBehaviorSet)
556557
{
557-
if (_includeEvaluationPropertiesAndItems == null)
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
571+
{
572+
var sinks = _eventSinkDictionary.Values.OfType<EventSourceSink>().ToList();
573+
574+
if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_12))
558575
{
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-
}
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);
570583
}
584+
else
585+
{
586+
bool allSinksIncludeEvalData = sinks.Any() && sinks.All(sink => sink.IncludeEvaluationPropertiesAndItems);
587+
588+
IncludeEvaluationPropertiesAndItemsInEvaluationFinishedEvent = allSinksIncludeEvalData;
589+
IncludeEvaluationPropertiesAndItemsInProjectStartedEvent = !allSinksIncludeEvalData;
590+
}
591+
}
592+
}
571593

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

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

578616
/// <summary>
@@ -614,6 +652,7 @@ public ICollection<string> GetWarningsNotAsErrors(BuildEventContext context)
614652
return GetWarningsForProject(context, _warningsNotAsErrorsByProject, WarningsNotAsErrors);
615653
}
616654

655+
617656
/// <summary>
618657
/// Returns a collection of warnings to be demoted to messages for the specified build context.
619658
/// </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.IncludeEvaluationPropertiesAndItems &&
135+
loggingService.IncludeEvaluationPropertiesAndItemsInProjectStartedEvent &&
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.IncludeEvaluationPropertiesAndItems &&
155+
loggingService.IncludeEvaluationPropertiesAndItemsInProjectStartedEvent &&
156156
propertiesToSerialize?.Length > 0 &&
157157
!loggingService.SerializeAllProperties)
158158
{

0 commit comments

Comments
 (0)