From 748844bd094dbcb8432ddf3876d68dd0259095f9 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Fri, 22 Mar 2024 14:57:12 +0100 Subject: [PATCH 1/2] Log TaskParameterEvent for scalar parameters --- .../BackEnd/TaskExecutionHost_Tests.cs | 31 +++++++++++ .../TaskThatReturnsDictionaryTaskItem.cs | 2 + .../TaskExecutionHost/TaskExecutionHost.cs | 53 ++++++++----------- src/UnitTests.Shared/MockLogger.cs | 10 ++++ 4 files changed, 66 insertions(+), 30 deletions(-) diff --git a/src/Build.UnitTests/BackEnd/TaskExecutionHost_Tests.cs b/src/Build.UnitTests/BackEnd/TaskExecutionHost_Tests.cs index f4644d4e358..633a52f037a 100644 --- a/src/Build.UnitTests/BackEnd/TaskExecutionHost_Tests.cs +++ b/src/Build.UnitTests/BackEnd/TaskExecutionHost_Tests.cs @@ -18,6 +18,7 @@ using Microsoft.Build.Execution; using Microsoft.Build.Framework; using Microsoft.Build.Shared; +using Shouldly; using Xunit; using InvalidProjectFileException = Microsoft.Build.Exceptions.InvalidProjectFileException; using TaskItem = Microsoft.Build.Execution.ProjectItemInstance.TaskItem; @@ -1048,6 +1049,36 @@ public void TestTaskDictionaryOutputItems() """); ml.AssertLogContains("a=b"); } + + [Fact] + public void TestTaskParameterLogging() + { + string customTaskPath = Assembly.GetExecutingAssembly().Location; + MockLogger ml = ObjectModelHelpers.BuildProjectExpectSuccess($""" + + + + + + + + + + + """); + + // Each parameter should be logged as TaskParameterEvent. + ml.TaskParameterEvents.Count.ShouldBe(3); + IList messages = ml.TaskParameterEvents.Select(e => e.Message).ToList(); + messages.ShouldContain($"{ItemGroupLoggingHelper.TaskParameterPrefix}Key=a"); + messages.ShouldContain($"{ItemGroupLoggingHelper.TaskParameterPrefix}Value=b"); + messages.ShouldContain($"{ItemGroupLoggingHelper.TaskParameterPrefix}\n AdditionalParameters=\n item1\n item2"); + + // Parameters should not be logged as messages. + messages = ml.BuildMessageEvents.Select(e => e.Message).ToList(); + messages.ShouldNotContain(m => m.StartsWith(ItemGroupLoggingHelper.TaskParameterPrefix)); + } + #endregion #region ITestTaskHost Members diff --git a/src/Build.UnitTests/BackEnd/TaskThatReturnsDictionaryTaskItem.cs b/src/Build.UnitTests/BackEnd/TaskThatReturnsDictionaryTaskItem.cs index c258beb89a4..b662a2ca441 100644 --- a/src/Build.UnitTests/BackEnd/TaskThatReturnsDictionaryTaskItem.cs +++ b/src/Build.UnitTests/BackEnd/TaskThatReturnsDictionaryTaskItem.cs @@ -17,6 +17,8 @@ public sealed class TaskThatReturnsDictionaryTaskItem : Utilities.Task public string Key { get; set; } public string Value { get; set; } + public ITaskItem[] AdditionalParameters { get; set; } + public override bool Execute() { var metaValue = new MinimalDictionary diff --git a/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs b/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs index 4418a48a63d..48308779720 100644 --- a/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs +++ b/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs @@ -1266,30 +1266,6 @@ private bool InitializeTaskVectorParameter( return success; } - /// - /// Variation to handle arrays, to help with logging the parameters. - /// - /// - /// Logging currently enabled only by an env var. - /// - private bool InternalSetTaskParameter(TaskPropertyInfo parameter, IList parameterValue) - { - if (LogTaskInputs && - !_taskLoggingContext.LoggingService.OnlyLogCriticalEvents && - parameterValue.Count > 0 && - parameter.Log) - { - ItemGroupLoggingHelper.LogTaskParameter( - _taskLoggingContext, - TaskParameterMessageKind.TaskInput, - parameter.Name, - parameterValue, - parameter.LogItemMetadata); - } - - return InternalSetTaskParameter(parameter, (object)parameterValue); - } - private static readonly string TaskParameterFormatString = ItemGroupLoggingHelper.TaskParameterPrefix + "{0}={1}"; /// @@ -1303,14 +1279,31 @@ private bool InternalSetTaskParameter( if (LogTaskInputs && !_taskLoggingContext.LoggingService.OnlyLogCriticalEvents) { - // If the type is a list, we already logged the parameters - if (!(parameterValue is IList)) + IList parameterValueAsList = parameterValue as IList; + bool legacyBehavior = !ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10); + + // Legacy textual logging for parameters that are not lists. + if (legacyBehavior && parameterValueAsList == null) { _taskLoggingContext.LogCommentFromText( - MessageImportance.Low, - TaskParameterFormatString, - parameter.Name, - ItemGroupLoggingHelper.GetStringFromParameterValue(parameterValue)); + MessageImportance.Low, + TaskParameterFormatString, + parameter.Name, + ItemGroupLoggingHelper.GetStringFromParameterValue(parameterValue)); + } + + if (parameter.Log) + { + // Structured logging for all parameters that have logging enabled and are not empty lists. + if (parameterValueAsList?.Count > 0 || (parameterValueAsList == null && !legacyBehavior)) + { + ItemGroupLoggingHelper.LogTaskParameter( + _taskLoggingContext, + TaskParameterMessageKind.TaskInput, + parameter.Name, + parameterValueAsList ?? new object[] { parameterValue }, + parameter.LogItemMetadata); + } } } diff --git a/src/UnitTests.Shared/MockLogger.cs b/src/UnitTests.Shared/MockLogger.cs index dab6b5e32d0..782cef74d41 100644 --- a/src/UnitTests.Shared/MockLogger.cs +++ b/src/UnitTests.Shared/MockLogger.cs @@ -123,6 +123,11 @@ public sealed class MockLogger : ILogger /// public List TaskFinishedEvents { get; } = new List(); + /// + /// List of TaskParameter events + /// + public List TaskParameterEvents { get; } = new List(); + /// /// List of BuildMessage events /// @@ -362,6 +367,11 @@ public void LoggerEventHandler(object sender, BuildEventArgs eventArgs) TaskFinishedEvents.Add(taskFinishedEventArgs); break; } + case TaskParameterEventArgs taskParameterEventArgs: + { + TaskParameterEvents.Add(taskParameterEventArgs); + break; + } case BuildMessageEventArgs buildMessageEventArgs: { BuildMessageEvents.Add(buildMessageEventArgs); From f9f8b0fed353b7d4fdb19af45b99622c67acb409 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Mon, 25 Mar 2024 11:14:22 +0100 Subject: [PATCH 2/2] Introduce change wave 17.12 --- documentation/wiki/ChangeWaves.md | 5 +++-- src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs | 2 +- src/Framework/ChangeWaves.cs | 3 ++- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md index f2ad4211e16..a6e36ac2f04 100644 --- a/documentation/wiki/ChangeWaves.md +++ b/documentation/wiki/ChangeWaves.md @@ -23,6 +23,9 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t ## Current Rotation of Change Waves +### 17.12 +- [Log TaskParameterEvent for scalar parameters](https://github.com/dotnet/msbuild/pull/9908) + ### 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` - [Warning on serialization custom events by default in .NET framework](https://github.com/dotnet/msbuild/pull/9318) @@ -36,14 +39,12 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t - [Introduce [MSBuild]::StableStringHash overloads](https://github.com/dotnet/msbuild/issues/9519) - [Keep the encoding of standard output & error consistent with the console code page for ToolTask](https://github.com/dotnet/msbuild/pull/9539) - ### 17.8 - [[RAR] Don't do I/O on SDK-provided references](https://github.com/dotnet/msbuild/pull/8688) - [Delete destination file before copy](https://github.com/dotnet/msbuild/pull/8685) - [Moving from SHA1 to SHA256 for Hash task](https://github.com/dotnet/msbuild/pull/8812) - [Deprecating custom derived BuildEventArgs](https://github.com/dotnet/msbuild/pull/8917) - 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` - ### 17.6 - [Parse invalid property under target](https://github.com/dotnet/msbuild/pull/8190) - [Eliminate project string cache](https://github.com/dotnet/msbuild/pull/7965) diff --git a/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs b/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs index 48308779720..8296519a5b0 100644 --- a/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs +++ b/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs @@ -1280,7 +1280,7 @@ private bool InternalSetTaskParameter( if (LogTaskInputs && !_taskLoggingContext.LoggingService.OnlyLogCriticalEvents) { IList parameterValueAsList = parameterValue as IList; - bool legacyBehavior = !ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10); + bool legacyBehavior = !ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_12); // Legacy textual logging for parameters that are not lists. if (legacyBehavior && parameterValueAsList == null) diff --git a/src/Framework/ChangeWaves.cs b/src/Framework/ChangeWaves.cs index b6479c3698e..51717c375ae 100644 --- a/src/Framework/ChangeWaves.cs +++ b/src/Framework/ChangeWaves.cs @@ -29,7 +29,8 @@ internal static class ChangeWaves internal static readonly Version Wave17_6 = new Version(17, 6); internal static readonly Version Wave17_8 = new Version(17, 8); internal static readonly Version Wave17_10 = new Version(17, 10); - internal static readonly Version[] AllWaves = { Wave17_4, Wave17_6, Wave17_8, Wave17_10 }; + internal static readonly Version Wave17_12 = new Version(17, 12); + internal static readonly Version[] AllWaves = { Wave17_4, Wave17_6, Wave17_8, Wave17_10, Wave17_12 }; /// /// Special value indicating that all features behind all Change Waves should be enabled.