From 0d05c2e42ce8ce92fe988eb1d73568fdc9466100 Mon Sep 17 00:00:00 2001 From: Jenny Bai Date: Mon, 10 Jul 2023 15:50:56 +0800 Subject: [PATCH 1/3] Improving the log of CombineTargetFrameworkInfoProperties fails with not valid RootElementName --- ...bineTargetFrameworkInfoProperties_Tests.cs | 36 +++++++++++++++++++ .../CombineTargetFrameworkInfoProperties.cs | 35 ++++++++++++++++-- 2 files changed, 68 insertions(+), 3 deletions(-) create mode 100644 src/Tasks.UnitTests/CombineTargetFrameworkInfoProperties_Tests.cs diff --git a/src/Tasks.UnitTests/CombineTargetFrameworkInfoProperties_Tests.cs b/src/Tasks.UnitTests/CombineTargetFrameworkInfoProperties_Tests.cs new file mode 100644 index 00000000000..553c62862d9 --- /dev/null +++ b/src/Tasks.UnitTests/CombineTargetFrameworkInfoProperties_Tests.cs @@ -0,0 +1,36 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using Microsoft.Build.Framework; +using Microsoft.Build.Tasks; +using Shouldly; +using Xunit; + +namespace Microsoft.Build.UnitTests +{ + public sealed class CombineTargetFrameworkInfoProperties_Tests + { + /// + /// https://github.com/dotnet/msbuild/issues/8320 + /// + [Fact] + public void RootElementNameNotValid() + { + var task = new CombineTargetFrameworkInfoProperties(); + var items = new ITaskItem[] + { + new TaskItemData("ItemSpec1", null) + }; + task.PropertiesAndValues = items; + task.UseAttributeForTargetFrameworkInfoPropertyNames = true; + var exp = Assert.Throws(() => task.Execute()); + exp.Message.ShouldContain("RootElementName"); + + task.RootElementName = string.Empty; + task.UseAttributeForTargetFrameworkInfoPropertyNames = false; + var exp1 = Assert.Throws(() => task.Execute()); + exp1.Message.ShouldContain("RootElementName"); + } + } +} diff --git a/src/Tasks/CombineTargetFrameworkInfoProperties.cs b/src/Tasks/CombineTargetFrameworkInfoProperties.cs index 6830ba9cb99..8707e14c169 100644 --- a/src/Tasks/CombineTargetFrameworkInfoProperties.cs +++ b/src/Tasks/CombineTargetFrameworkInfoProperties.cs @@ -17,7 +17,28 @@ public class CombineTargetFrameworkInfoProperties : TaskExtension /// /// The root element name to use for the generated XML string /// - public string RootElementName { get; set; } + private string _rootElementName; + + /// + /// Gets or sets the root element name to use for the generated XML string + /// + public string RootElementName + { + get + { + if (!UseAttributeForTargetFrameworkInfoPropertyNames) + { + ErrorUtilities.VerifyThrowArgumentLength(_rootElementName, nameof(RootElementName)); + } + else + { + ErrorUtilities.VerifyThrowArgumentNull(_rootElementName, nameof(RootElementName)); + } + return _rootElementName; + } + + set => _rootElementName = value; + } /// /// Items to include in the XML. The ItemSpec should be the property name, and it should have Value metadata for its value. @@ -39,9 +60,17 @@ public override bool Execute() { if (PropertiesAndValues != null) { + if (!UseAttributeForTargetFrameworkInfoPropertyNames) + { + ErrorUtilities.VerifyThrowArgumentLength(_rootElementName, nameof(RootElementName)); + } + else + { + ErrorUtilities.VerifyThrowArgumentNull(_rootElementName, nameof(RootElementName)); + } XElement root = UseAttributeForTargetFrameworkInfoPropertyNames ? - new("TargetFramework", new XAttribute("Name", EscapingUtilities.Escape(RootElementName))) : - new(RootElementName); + new("TargetFramework", new XAttribute("Name", EscapingUtilities.Escape(_rootElementName))) : + new(_rootElementName); foreach (ITaskItem item in PropertiesAndValues) { From 97757ea262fc1a799eab20d7b2472ab0cffe128d Mon Sep 17 00:00:00 2001 From: Jenny Bai Date: Tue, 11 Jul 2023 17:35:11 +0800 Subject: [PATCH 2/3] Set a unique error code for the specific failure --- ...bineTargetFrameworkInfoProperties_Tests.cs | 10 ++-- .../CombineTargetFrameworkInfoProperties.cs | 48 +++++-------------- src/Tasks/Resources/Strings.resx | 13 +++++ src/Tasks/Resources/xlf/Strings.cs.xlf | 10 ++++ src/Tasks/Resources/xlf/Strings.de.xlf | 10 ++++ src/Tasks/Resources/xlf/Strings.es.xlf | 10 ++++ src/Tasks/Resources/xlf/Strings.fr.xlf | 10 ++++ src/Tasks/Resources/xlf/Strings.it.xlf | 10 ++++ src/Tasks/Resources/xlf/Strings.ja.xlf | 10 ++++ src/Tasks/Resources/xlf/Strings.ko.xlf | 10 ++++ src/Tasks/Resources/xlf/Strings.pl.xlf | 10 ++++ src/Tasks/Resources/xlf/Strings.pt-BR.xlf | 10 ++++ src/Tasks/Resources/xlf/Strings.ru.xlf | 10 ++++ src/Tasks/Resources/xlf/Strings.tr.xlf | 10 ++++ src/Tasks/Resources/xlf/Strings.zh-Hans.xlf | 10 ++++ src/Tasks/Resources/xlf/Strings.zh-Hant.xlf | 10 ++++ 16 files changed, 162 insertions(+), 39 deletions(-) diff --git a/src/Tasks.UnitTests/CombineTargetFrameworkInfoProperties_Tests.cs b/src/Tasks.UnitTests/CombineTargetFrameworkInfoProperties_Tests.cs index 553c62862d9..7cb2bd0add6 100644 --- a/src/Tasks.UnitTests/CombineTargetFrameworkInfoProperties_Tests.cs +++ b/src/Tasks.UnitTests/CombineTargetFrameworkInfoProperties_Tests.cs @@ -17,20 +17,22 @@ public sealed class CombineTargetFrameworkInfoProperties_Tests [Fact] public void RootElementNameNotValid() { + MockEngine e = new MockEngine(); var task = new CombineTargetFrameworkInfoProperties(); + task.BuildEngine = e; var items = new ITaskItem[] { new TaskItemData("ItemSpec1", null) }; task.PropertiesAndValues = items; task.UseAttributeForTargetFrameworkInfoPropertyNames = true; - var exp = Assert.Throws(() => task.Execute()); - exp.Message.ShouldContain("RootElementName"); + task.Execute().ShouldBe(false); + e.AssertLogContains("MSB3992"); task.RootElementName = string.Empty; task.UseAttributeForTargetFrameworkInfoPropertyNames = false; - var exp1 = Assert.Throws(() => task.Execute()); - exp1.Message.ShouldContain("RootElementName"); + task.Execute().ShouldBe(false); + e.AssertLogContains("MSB3991"); } } } diff --git a/src/Tasks/CombineTargetFrameworkInfoProperties.cs b/src/Tasks/CombineTargetFrameworkInfoProperties.cs index 8707e14c169..a5f75f3bbdd 100644 --- a/src/Tasks/CombineTargetFrameworkInfoProperties.cs +++ b/src/Tasks/CombineTargetFrameworkInfoProperties.cs @@ -17,29 +17,7 @@ public class CombineTargetFrameworkInfoProperties : TaskExtension /// /// The root element name to use for the generated XML string /// - private string _rootElementName; - - /// - /// Gets or sets the root element name to use for the generated XML string - /// - public string RootElementName - { - get - { - if (!UseAttributeForTargetFrameworkInfoPropertyNames) - { - ErrorUtilities.VerifyThrowArgumentLength(_rootElementName, nameof(RootElementName)); - } - else - { - ErrorUtilities.VerifyThrowArgumentNull(_rootElementName, nameof(RootElementName)); - } - return _rootElementName; - } - - set => _rootElementName = value; - } - + public string RootElementName { get; set; } /// /// Items to include in the XML. The ItemSpec should be the property name, and it should have Value metadata for its value. /// @@ -60,24 +38,24 @@ public override bool Execute() { if (PropertiesAndValues != null) { - if (!UseAttributeForTargetFrameworkInfoPropertyNames) + if ((!UseAttributeForTargetFrameworkInfoPropertyNames && string.IsNullOrEmpty(RootElementName)) || (UseAttributeForTargetFrameworkInfoPropertyNames && RootElementName == null)) { - ErrorUtilities.VerifyThrowArgumentLength(_rootElementName, nameof(RootElementName)); + string resource = UseAttributeForTargetFrameworkInfoPropertyNames ? "CombineTargetFrameworkInfoProperties.NotNullRootElementName" : "CombineTargetFrameworkInfoProperties.NotNullAndEmptyRootElementName"; + Log.LogErrorWithCodeFromResources(resource, "RootElementName"); } else { - ErrorUtilities.VerifyThrowArgumentNull(_rootElementName, nameof(RootElementName)); - } - XElement root = UseAttributeForTargetFrameworkInfoPropertyNames ? - new("TargetFramework", new XAttribute("Name", EscapingUtilities.Escape(_rootElementName))) : - new(_rootElementName); + XElement root = UseAttributeForTargetFrameworkInfoPropertyNames ? + new("TargetFramework", new XAttribute("Name", EscapingUtilities.Escape(RootElementName))) : + new(RootElementName); - foreach (ITaskItem item in PropertiesAndValues) - { - root.Add(new XElement(item.ItemSpec, item.GetMetadata("Value"))); - } + foreach (ITaskItem item in PropertiesAndValues) + { + root.Add(new XElement(item.ItemSpec, item.GetMetadata("Value"))); + } - Result = root.ToString(); + Result = root.ToString(); + } } return !Log.HasLoggedErrors; } diff --git a/src/Tasks/Resources/Strings.resx b/src/Tasks/Resources/Strings.resx index fdf44f5268a..5490ca34553 100644 --- a/src/Tasks/Resources/Strings.resx +++ b/src/Tasks/Resources/Strings.resx @@ -2985,6 +2985,18 @@ PFX signing not supported on .NET Core. + + + + MSB3991: '{0}' is not set or empty. When UseAttributeForTargetFrameworkInfoPropertyNames is false, make sure to set a non-empty value for '{0}'. + {StrBegin="MSB3991: "} + + + MSB3992: '{0}' is not set. When UseAttributeForTargetFrameworkInfoPropertyNames is true, make sure to set a value for '{0}'. + {StrBegin="MSB3992: "} + - MSB3991: '{0}' is not set or empty. When UseAttributeForTargetFrameworkInfoPropertyNames is false, make sure to set a non-empty value for '{0}'. + MSB3991: '{0}' is not set or empty. When {1} is false, make sure to set a non-empty value for '{0}'. {StrBegin="MSB3991: "} - MSB3992: '{0}' is not set. When UseAttributeForTargetFrameworkInfoPropertyNames is true, make sure to set a value for '{0}'. + MSB3992: '{0}' is not set. When {1} is true, make sure to set a value for '{0}'. {StrBegin="MSB3992: "}