From a32bf48ed73ac1fbd7cbdfd4766c4017ecaf6471 Mon Sep 17 00:00:00 2001 From: SimaTian Date: Thu, 7 Nov 2024 14:09:23 +0100 Subject: [PATCH 1/6] add WarningsAsMessages, WarningsAsErrors, WarningsNotAsErrors and TreatWarningsAsErrors to the engine (e.g. variant without prefix). test those so that nothing breaks --- .../WarningsAsMessagesAndErrors_Tests.cs | 201 ++++++++++++++++-- .../RequestBuilder/RequestBuilder.cs | 37 +++- src/Shared/Constants.cs | 21 ++ 3 files changed, 235 insertions(+), 24 deletions(-) diff --git a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs index db2d9eab3ad..c16a210b227 100644 --- a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs +++ b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs @@ -35,6 +35,19 @@ public void TreatAllWarningsAsErrors() ObjectModelHelpers.BuildProjectExpectSuccess(GetTestProject(treatAllWarningsAsErrors: false)); } + [Fact] + public void TreatAllWarningsAsErrorsNoPrefix() + { + MockLogger logger = ObjectModelHelpers.BuildProjectExpectFailure(GetTestProject(customProperties: new Dictionary + { + {"TreatWarningsAsErrors", "true"}, + })); + + VerifyBuildErrorEvent(logger); + + ObjectModelHelpers.BuildProjectExpectSuccess(GetTestProject(treatAllWarningsAsErrors: false)); + } + /// /// https://github.com/dotnet/msbuild/issues/2667 /// @@ -91,22 +104,6 @@ public void TreatWarningsAsErrorsWhenSpecifiedIndirectly() VerifyBuildErrorEvent(logger); } - [Fact] - public void TreatWarningsAsErrorsWhenSpecifiedThroughAdditiveProperty() - { - MockLogger logger = ObjectModelHelpers.BuildProjectExpectFailure( - GetTestProject( - customProperties: new List> - { - new KeyValuePair("MSBuildWarningsAsErrors", "123"), - new KeyValuePair("MSBuildWarningsAsErrors", $@"$(MSBuildWarningsAsErrors); - {ExpectedEventCode.ToLowerInvariant()}"), - new KeyValuePair("MSBuildWarningsAsErrors", "$(MSBuildWarningsAsErrors);ABC") - })); - - VerifyBuildErrorEvent(logger); - } - [Fact] public void NotTreatWarningsAsErrorsWhenCodeNotSpecified() { @@ -193,6 +190,125 @@ public void TreatWarningsAsMessagesWhenSpecifiedThroughAdditiveProperty() VerifyBuildMessageEvent(logger); } + + [Fact] + public void TreatWarningsAsMessagesWhenSpecifiedThroughAdditivePropertyNoPrefix() + { + MockLogger logger = ObjectModelHelpers.BuildProjectExpectSuccess( + GetTestProject( + customProperties: new List> + { + new KeyValuePair("WarningsAsMessages", "123"), + new KeyValuePair("WarningsAsMessages", $@"$(WarningsAsMessages); + {ExpectedEventCode.ToLowerInvariant()}"), + new KeyValuePair("WarningsAsMessages", "$(WarningsAsMessages);ABC") + })); + + VerifyBuildMessageEvent(logger); + } + + [Fact] + public void TreatWarningsAsMessagesWhenSpecifiedThroughAdditivePropertyCombination() + { + MockLogger logger = ObjectModelHelpers.BuildProjectExpectSuccess( + GetTestProject( + customProperties: new List> + { + new KeyValuePair("MSBuildWarningsAsMessages", "123"), + new KeyValuePair("WarningsAsMessages", $@"$(BuildWarningsAsMessages); + {ExpectedEventCode.ToLowerInvariant()}"), + new KeyValuePair("MSBuildWarningsAsMessages", "$(BuildWarningsAsMessages);ABC") + })); + + VerifyBuildMessageEvent(logger); + } + + [Fact] + public void TreatWarningsNotAsErrorsWhenSpecifiedThroughAdditivePropertyx() + { + MockLogger logger = ObjectModelHelpers.BuildProjectExpectSuccess( + GetTestProject( + treatAllWarningsAsErrors: true, + customProperties: new List> + { + new KeyValuePair("MSBuildWarningsNotAsErrors", "123"), + new KeyValuePair("MSBuildWarningsNotAsErrors", $@"$(MSBuildWarningsNotAsErrors); + {ExpectedEventCode.ToLowerInvariant()}"), + new KeyValuePair("MSBuildWarningsNotAsErrors", "$(MSBuildWarningsNotAsErrors);ABC") + })); + + VerifyBuildWarningEvent(logger); + } + + [Fact] + public void TreatWarningsNotAsErrorsWhenSpecifiedThroughAdditivePropertyNoPrefix() + { + MockLogger logger = ObjectModelHelpers.BuildProjectExpectSuccess( + GetTestProject( + treatAllWarningsAsErrors: true, + customProperties: new List> + { + new KeyValuePair("WarningsNotAsErrors", "123"), + new KeyValuePair("WarningsNotAsErrors", $@"$(WarningsNotAsErrors); + {ExpectedEventCode.ToLowerInvariant()}"), + new KeyValuePair("WarningsNotAsErrors", "$(WarningsNotAsErrors);ABC") + })); + + VerifyBuildWarningEvent(logger); + } + + [Fact] + public void TreatWarningsNotAsErrorsWhenSpecifiedThroughAdditivePropertyCombination() + { + MockLogger logger = ObjectModelHelpers.BuildProjectExpectSuccess( + GetTestProject( + customProperties: new List> + { + new KeyValuePair("MSBuildWarningsNotAsErrors", "123"), + new KeyValuePair("WarningsNotAsErrors", $@"$(MSBuildWarningsNotAsErrors); + {ExpectedEventCode.ToLowerInvariant()}"), + new KeyValuePair("MSBuildWarningsNotAsErrors", "$(WarningsNotAsErrors);ABC") + })); + + VerifyBuildWarningEvent(logger); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void TreatWarningsAsErrorsWhenSpecifiedThroughAdditiveProperty(bool MSBuildPrefix) + { + string prefix = MSBuildPrefix ? "MSBuild" : ""; + MockLogger logger = ObjectModelHelpers.BuildProjectExpectFailure( + GetTestProject( + customProperties: new List> + { + new KeyValuePair($@"{prefix}WarningsAsErrors", "123"), + new KeyValuePair($@"{prefix}WarningsAsErrors", $@"$({prefix}WarningsAsErrors); + {ExpectedEventCode.ToLowerInvariant()}"), + new KeyValuePair($@"{prefix}WarningsAsErrors", $@"$({prefix}WarningsAsErrors);ABC") + })); + + VerifyBuildErrorEvent(logger); + } + + + [Fact] + public void TreatWarningsAsErrorsWhenSpecifiedThroughAdditivePropertyCombination() + { + MockLogger logger = ObjectModelHelpers.BuildProjectExpectFailure( + GetTestProject( + customProperties: new List> + { + new KeyValuePair("WarningsAsErrors", "123"), + new KeyValuePair("MSBuildWarningsAsErrors", $@"$(WarningsAsErrors); + {ExpectedEventCode.ToLowerInvariant()}"), + new KeyValuePair("WarningsAsErrors", "$(MSBuildWarningsAsErrors);ABC") + })); + + VerifyBuildErrorEvent(logger); + } + [Fact] public void NotTreatWarningsAsMessagesWhenCodeNotSpecified() { @@ -273,27 +389,33 @@ private string GetTestProject(bool? treatAllWarningsAsErrors = null, string warn "; } + [Theory] [InlineData("MSB1235", "MSB1234", "MSB1234", "MSB1234", false)] // Log MSB1234, treat as error via MSBuildWarningsAsErrors [InlineData("MSB1235", "", "MSB1234", "MSB1234", true)] // Log MSB1234, expect MSB1234 as error via MSBuildTreatWarningsAsErrors [InlineData("MSB1234", "MSB1234", "MSB1234", "MSB4181", true)]// Log MSB1234, MSBuildWarningsAsMessages takes priority + [InlineData("MSB1235", "MSB1234", "MSB1234", "MSB1234", false, false)] // Log MSB1234, treat as error via BuildWarningsAsErrors + [InlineData("MSB1235", "", "MSB1234", "MSB1234", true, false)] // Log MSB1234, expect MSB1234 as error via BuildTreatWarningsAsErrors + [InlineData("MSB1234", "MSB1234", "MSB1234", "MSB4181", true, false)]// Log MSB1234, BuildWarningsAsMessages takes priority public void WarningsAsErrorsAndMessages_Tests(string WarningsAsMessages, string WarningsAsErrors, string WarningToLog, string LogShouldContain, - bool allWarningsAreErrors = false) + bool allWarningsAreErrors = false, + bool useMSPrefix = true) { using (TestEnvironment env = TestEnvironment.Create(_output)) { + var prefix = useMSPrefix ? "MSBuild" : ""; TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@" - {allWarningsAreErrors} - {WarningsAsMessages} - {WarningsAsErrors} + <{prefix}TreatWarningsAsErrors>{allWarningsAreErrors} + <{prefix}WarningsAsMessages>{WarningsAsMessages} + <{prefix}WarningsAsErrors>{WarningsAsErrors} @@ -310,6 +432,36 @@ public void WarningsAsErrorsAndMessages_Tests(string WarningsAsMessages, } } + [Theory] + + [InlineData(true)]// Log MSB1234, BuildWarningsNotAsErrors takes priority + [InlineData(false)] + public void WarningsNotAsErrorsAndMessages_Tests(bool useMSPrefix) + { + string Warning = "MSB1235"; + using (TestEnvironment env = TestEnvironment.Create(_output)) + { + string prefix = useMSPrefix ? "MSBuild" : ""; + TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@" + + + <{prefix}TreatWarningsAsErrors>true + <{prefix}WarningsNotAsErrors>{Warning} + + + + + "); + + MockLogger logger = proj.BuildProjectExpectSuccess(); + + logger.WarningCount.ShouldBe(1); + logger.ErrorCount.ShouldBe(0); + + logger.AssertLogContains(Warning); + } + } + /// /// Item1 and Item2 log warnings and continue, item 3 logs a warn-> error and prevents item 4 from running in the batched build. /// @@ -371,8 +523,11 @@ public void TaskLogsWarningAsError_BatchedBuild() [Theory] [InlineData("MSB1234", false, 1, 1)] [InlineData("MSB0000", true, 0, 2)] - public void TaskReturnsTrue_Tests(string warningsAsErrors, bool treatAllWarningsAsErrors, int warningCountShouldBe, int errorCountShouldBe) + [InlineData("MSB1234", false, 1, 1, false)] + [InlineData("MSB0000", true, 0, 2, false)] + public void TaskReturnsTrue_Tests(string warningsAsErrors, bool treatAllWarningsAsErrors, int warningCountShouldBe, int errorCountShouldBe, bool useMSPrefix = true) { + string prefix = useMSPrefix ? "MSBuild" : ""; using (TestEnvironment env = TestEnvironment.Create(_output)) { TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@" @@ -380,8 +535,8 @@ public void TaskReturnsTrue_Tests(string warningsAsErrors, bool treatAllWarnings - {treatAllWarningsAsErrors} - {warningsAsErrors} + <{prefix}TreatWarningsAsErrors>{treatAllWarningsAsErrors} + <{prefix}WarningsAsErrors>{warningsAsErrors} diff --git a/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs b/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs index d93803fdda9..233c495a1d0 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs @@ -1390,7 +1390,8 @@ private void ConfigureWarningsAsErrorsAndMessages() // Ensure everything that is required is available at this time if (project != null && buildEventContext != null && loggingService != null && buildEventContext.ProjectInstanceId != BuildEventContext.InvalidProjectInstanceId) { - if (String.Equals(project.GetEngineRequiredPropertyValue(MSBuildConstants.TreatWarningsAsErrors)?.Trim(), "true", StringComparison.OrdinalIgnoreCase)) + if (String.Equals(project.GetEngineRequiredPropertyValue(MSBuildConstants.TreatWarningsAsErrors)?.Trim(), "true", StringComparison.OrdinalIgnoreCase) || + String.Equals(project.GetEngineRequiredPropertyValue(MSBuildConstants.TreatWarningsAsErrorsNoPrefix)?.Trim(), "true", StringComparison.OrdinalIgnoreCase)) { // If signals the IEventSourceSink to treat all warnings as errors loggingService.AddWarningsAsErrors(buildEventContext, new HashSet()); @@ -1398,6 +1399,20 @@ private void ConfigureWarningsAsErrorsAndMessages() else { ISet warningsAsErrors = ParseWarningCodes(project.GetEngineRequiredPropertyValue(MSBuildConstants.WarningsAsErrors)); + var warningsAsErrorsNoPrefix = ParseWarningCodes(project.GetEngineRequiredPropertyValue(MSBuildConstants.WarningsAsErrorsNoPrefix)); + if (warningsAsErrorsNoPrefix != null) + { + if (warningsAsErrors != null) + { + warningsAsErrors.UnionWith(warningsAsErrorsNoPrefix); + } + else + { + warningsAsErrors = warningsAsErrorsNoPrefix; + } + } + + if (warningsAsErrors?.Count > 0) { @@ -1406,6 +1421,20 @@ private void ConfigureWarningsAsErrorsAndMessages() } ISet warningsNotAsErrors = ParseWarningCodes(project.GetEngineRequiredPropertyValue(MSBuildConstants.WarningsNotAsErrors)); + var warningsNotAsErrorsNoPrefix = ParseWarningCodes(project.GetEngineRequiredPropertyValue(MSBuildConstants.WarningsNotAsErrorsNoPrefix)); + if (warningsNotAsErrorsNoPrefix != null) + { + if (warningsNotAsErrors != null) + { + warningsNotAsErrors.UnionWith(warningsNotAsErrorsNoPrefix); + } + else + { + warningsNotAsErrors = warningsNotAsErrorsNoPrefix; + } + } + + if (warningsNotAsErrors?.Count > 0) { @@ -1413,6 +1442,12 @@ private void ConfigureWarningsAsErrorsAndMessages() } ISet warningsAsMessages = ParseWarningCodes(project.GetEngineRequiredPropertyValue(MSBuildConstants.WarningsAsMessages)); + var warningsAsMessagesNoPrefix = ParseWarningCodes(project.GetEngineRequiredPropertyValue(MSBuildConstants.WarningsAsMessagesNoPrefix)); + if (warningsAsMessagesNoPrefix != null) + { + warningsAsMessages?.UnionWith(warningsAsMessagesNoPrefix); + warningsAsMessages ??= warningsAsMessagesNoPrefix; + } if (warningsAsMessages?.Count > 0) { diff --git a/src/Shared/Constants.cs b/src/Shared/Constants.cs index e0ac6bae417..246cff2c646 100644 --- a/src/Shared/Constants.cs +++ b/src/Shared/Constants.cs @@ -33,21 +33,42 @@ internal static class MSBuildConstants /// internal const string TreatWarningsAsErrors = "MSBuildTreatWarningsAsErrors"; + /// + /// Name of the property that indicates that all warnings should be treated as errors. + /// + internal const string TreatWarningsAsErrorsNoPrefix = "TreatWarningsAsErrors"; + /// /// Name of the property that indicates a list of warnings to treat as errors. /// internal const string WarningsAsErrors = "MSBuildWarningsAsErrors"; + /// + /// Name of the property that indicates a list of warnings to treat as errors. + /// + internal const string WarningsAsErrorsNoPrefix = "WarningsAsErrors"; + /// /// Name of the property that indicates a list of warnings to not treat as errors. /// internal const string WarningsNotAsErrors = "MSBuildWarningsNotAsErrors"; + /// + /// Name of the property that indicates a list of warnings to not treat as errors. + /// + internal const string WarningsNotAsErrorsNoPrefix = "WarningsNotAsErrors"; + /// /// Name of the property that indicates the list of warnings to treat as messages. /// internal const string WarningsAsMessages = "MSBuildWarningsAsMessages"; + + /// + /// Name of the property that indicates the list of warnings to treat as messages. + /// + internal const string WarningsAsMessagesNoPrefix = "WarningsAsMessages"; + /// /// The name of the environment variable that users can specify to override where NuGet assemblies are loaded from in the NuGetSdkResolver. /// From d3527be0a9021b47221e142e5e1c80f1fd38d11a Mon Sep 17 00:00:00 2001 From: Rainer Sigwald Date: Thu, 7 Nov 2024 16:06:22 -0600 Subject: [PATCH 2/6] Optional output in BuildProjectExpectFailure Optionally capture output in BuildProjectExpectFailure for better test diagnosability. --- src/UnitTests.Shared/ObjectModelHelpers.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/UnitTests.Shared/ObjectModelHelpers.cs b/src/UnitTests.Shared/ObjectModelHelpers.cs index 4e38ceba1d8..5dfd616d436 100644 --- a/src/UnitTests.Shared/ObjectModelHelpers.cs +++ b/src/UnitTests.Shared/ObjectModelHelpers.cs @@ -780,9 +780,9 @@ public static void BuildProjectExpectSuccess( /// /// The project file content in string format. /// The that was used during evaluation and build. - public static MockLogger BuildProjectExpectFailure(string projectContents) + public static MockLogger BuildProjectExpectFailure(string projectContents, ITestOutputHelper testOutputHelper = null) { - MockLogger logger = new MockLogger(); + MockLogger logger = new MockLogger(testOutputHelper); BuildProjectExpectFailure(projectContents, logger); return logger; } From 84afd71bb05a4927380a57acef144d30cf52d8aa Mon Sep 17 00:00:00 2001 From: Rainer Sigwald Date: Thu, 7 Nov 2024 16:06:41 -0600 Subject: [PATCH 3/6] Capture output logging in new tests --- .../WarningsAsMessagesAndErrors_Tests.cs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs index c16a210b227..1174a41dd95 100644 --- a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs +++ b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs @@ -252,7 +252,8 @@ public void TreatWarningsNotAsErrorsWhenSpecifiedThroughAdditivePropertyNoPrefix new KeyValuePair("WarningsNotAsErrors", $@"$(WarningsNotAsErrors); {ExpectedEventCode.ToLowerInvariant()}"), new KeyValuePair("WarningsNotAsErrors", "$(WarningsNotAsErrors);ABC") - })); + }), + _output); VerifyBuildWarningEvent(logger); } @@ -268,7 +269,8 @@ public void TreatWarningsNotAsErrorsWhenSpecifiedThroughAdditivePropertyCombinat new KeyValuePair("WarningsNotAsErrors", $@"$(MSBuildWarningsNotAsErrors); {ExpectedEventCode.ToLowerInvariant()}"), new KeyValuePair("MSBuildWarningsNotAsErrors", "$(WarningsNotAsErrors);ABC") - })); + }), + _output); VerifyBuildWarningEvent(logger); } @@ -287,7 +289,8 @@ public void TreatWarningsAsErrorsWhenSpecifiedThroughAdditiveProperty(bool MSBui new KeyValuePair($@"{prefix}WarningsAsErrors", $@"$({prefix}WarningsAsErrors); {ExpectedEventCode.ToLowerInvariant()}"), new KeyValuePair($@"{prefix}WarningsAsErrors", $@"$({prefix}WarningsAsErrors);ABC") - })); + }), + _output); VerifyBuildErrorEvent(logger); } @@ -304,7 +307,8 @@ public void TreatWarningsAsErrorsWhenSpecifiedThroughAdditivePropertyCombination new KeyValuePair("MSBuildWarningsAsErrors", $@"$(WarningsAsErrors); {ExpectedEventCode.ToLowerInvariant()}"), new KeyValuePair("WarningsAsErrors", "$(MSBuildWarningsAsErrors);ABC") - })); + }), + _output); VerifyBuildErrorEvent(logger); } @@ -318,7 +322,8 @@ public void NotTreatWarningsAsMessagesWhenCodeNotSpecified() { new KeyValuePair("MSBuildWarningsAsMessages", "123"), new KeyValuePair("MSBuildWarningsAsMessages", "$(MSBuildWarningsAsMessages);ABC") - })); + }), + _output); VerifyBuildWarningEvent(logger); } From 0ecb330fe42e16f72d45427a3e8592a0b5204ad9 Mon Sep 17 00:00:00 2001 From: SimaTian Date: Mon, 11 Nov 2024 17:53:35 +0100 Subject: [PATCH 4/6] working through the review. Some test improvements. Changewave used. Comments. --- documentation/wiki/ChangeWaves.md | 1 + .../WarningsAsMessagesAndErrors_Tests.cs | 115 +++++++++--------- .../RequestBuilder/RequestBuilder.cs | 80 ++++++------ src/Shared/Constants.cs | 28 +---- 4 files changed, 100 insertions(+), 124 deletions(-) diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md index 5df8c06508f..5ce6ed3dc8b 100644 --- a/documentation/wiki/ChangeWaves.md +++ b/documentation/wiki/ChangeWaves.md @@ -25,6 +25,7 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t ### 17.14 - [.SLNX support - use the new parser for .sln and .slnx](https://github.com/dotnet/msbuild/pull/10836) +- [TreatWarningsAsErrors, WarningsAsMessages, WarningsAsErrors, WarningsNotAsErrors are now supported on the engine side of MSBuild](https://github.com/dotnet/msbuild/pull/10942) ### 17.12 - [Log TaskParameterEvent for scalar parameters](https://github.com/dotnet/msbuild/pull/9908) diff --git a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs index 1174a41dd95..7a490b59def 100644 --- a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs +++ b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs @@ -174,40 +174,30 @@ public void TreatWarningsAsMessagesWhenSpecifiedIndirectly() VerifyBuildMessageEvent(logger); } - [Fact] - public void TreatWarningsAsMessagesWhenSpecifiedThroughAdditiveProperty() - { - MockLogger logger = ObjectModelHelpers.BuildProjectExpectSuccess( - GetTestProject( - customProperties: new List> - { - new KeyValuePair("MSBuildWarningsAsMessages", "123"), - new KeyValuePair("MSBuildWarningsAsMessages", $@"$(MSBuildWarningsAsMessages); - {ExpectedEventCode.ToLowerInvariant()}"), - new KeyValuePair("MSBuildWarningsAsMessages", "$(MSBuildWarningsAsMessages);ABC") - })); - - VerifyBuildMessageEvent(logger); - } - - - [Fact] - public void TreatWarningsAsMessagesWhenSpecifiedThroughAdditivePropertyNoPrefix() + [Theory] + [InlineData(true)] + [InlineData(false)] + public void TreatWarningsAsMessagesWhenSpecifiedThroughAdditiveProperty(bool usePrefix) { + string prefix = usePrefix ? "MSBuild" : ""; MockLogger logger = ObjectModelHelpers.BuildProjectExpectSuccess( GetTestProject( customProperties: new List> { - new KeyValuePair("WarningsAsMessages", "123"), - new KeyValuePair("WarningsAsMessages", $@"$(WarningsAsMessages); + new KeyValuePair($"{prefix}WarningsAsMessages", "123"), + new KeyValuePair($"{prefix}WarningsAsMessages", $@"$({prefix}WarningsAsMessages); {ExpectedEventCode.ToLowerInvariant()}"), - new KeyValuePair("WarningsAsMessages", "$(WarningsAsMessages);ABC") + new KeyValuePair($"{prefix}WarningsAsMessages", $"$({prefix}WarningsAsMessages);ABC") })); VerifyBuildMessageEvent(logger); } [Fact] + /// + /// This is for chaining the properties together via addition. + /// Furthermore it is intended to check if the prefix and no prefix variant interacts properly with each other. + /// public void TreatWarningsAsMessagesWhenSpecifiedThroughAdditivePropertyCombination() { MockLogger logger = ObjectModelHelpers.BuildProjectExpectSuccess( @@ -215,49 +205,14 @@ public void TreatWarningsAsMessagesWhenSpecifiedThroughAdditivePropertyCombinati customProperties: new List> { new KeyValuePair("MSBuildWarningsAsMessages", "123"), - new KeyValuePair("WarningsAsMessages", $@"$(BuildWarningsAsMessages); + new KeyValuePair("WarningsAsMessages", $@"$(MSBuildWarningsAsMessages); {ExpectedEventCode.ToLowerInvariant()}"), - new KeyValuePair("MSBuildWarningsAsMessages", "$(BuildWarningsAsMessages);ABC") + new KeyValuePair("MSBuildWarningsAsMessages", "$(WarningsAsMessages);ABC") })); VerifyBuildMessageEvent(logger); } - [Fact] - public void TreatWarningsNotAsErrorsWhenSpecifiedThroughAdditivePropertyx() - { - MockLogger logger = ObjectModelHelpers.BuildProjectExpectSuccess( - GetTestProject( - treatAllWarningsAsErrors: true, - customProperties: new List> - { - new KeyValuePair("MSBuildWarningsNotAsErrors", "123"), - new KeyValuePair("MSBuildWarningsNotAsErrors", $@"$(MSBuildWarningsNotAsErrors); - {ExpectedEventCode.ToLowerInvariant()}"), - new KeyValuePair("MSBuildWarningsNotAsErrors", "$(MSBuildWarningsNotAsErrors);ABC") - })); - - VerifyBuildWarningEvent(logger); - } - - [Fact] - public void TreatWarningsNotAsErrorsWhenSpecifiedThroughAdditivePropertyNoPrefix() - { - MockLogger logger = ObjectModelHelpers.BuildProjectExpectSuccess( - GetTestProject( - treatAllWarningsAsErrors: true, - customProperties: new List> - { - new KeyValuePair("WarningsNotAsErrors", "123"), - new KeyValuePair("WarningsNotAsErrors", $@"$(WarningsNotAsErrors); - {ExpectedEventCode.ToLowerInvariant()}"), - new KeyValuePair("WarningsNotAsErrors", "$(WarningsNotAsErrors);ABC") - }), - _output); - - VerifyBuildWarningEvent(logger); - } - [Fact] public void TreatWarningsNotAsErrorsWhenSpecifiedThroughAdditivePropertyCombination() { @@ -295,7 +250,6 @@ public void TreatWarningsAsErrorsWhenSpecifiedThroughAdditiveProperty(bool MSBui VerifyBuildErrorEvent(logger); } - [Fact] public void TreatWarningsAsErrorsWhenSpecifiedThroughAdditivePropertyCombination() { @@ -467,6 +421,47 @@ public void WarningsNotAsErrorsAndMessages_Tests(bool useMSPrefix) } } + + + [Theory] + [InlineData("TreatWarningsAsErrors", "true", false)] // All warnings are treated as errors + [InlineData("WarningsAsErrors", "MSB1007", false)] + [InlineData("WarningsAsMessages", "MSB1007", false)] + [InlineData("WarningsNotAsErrors", "MSB1007", true)] + public void WarningsChangeWaveTest(string property, string propertyData, bool treatWarningsAsErrors) + { + using (TestEnvironment env = TestEnvironment.Create(_output)) + { + string warningCode = "MSB1007"; + string treatWarningsAsErrorsCodeProperty = treatWarningsAsErrors ? "true" : ""; + env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", ChangeWaves.Wave17_14.ToString()); + TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@" + + + {treatWarningsAsErrorsCodeProperty} + <{property}>{propertyData} + + + + + "); + if (treatWarningsAsErrors) + { + // Since the "no prefix" variations can't do anything with the change wave disabled, this should always fail. + MockLogger logger = proj.BuildProjectExpectFailure(); + } + else + { + MockLogger logger = proj.BuildProjectExpectSuccess(); + + logger.WarningCount.ShouldBe(1); + logger.ErrorCount.ShouldBe(0); + + logger.AssertLogContains(warningCode); + } + } + } + /// /// Item1 and Item2 log warnings and continue, item 3 logs a warn-> error and prevents item 4 from running in the batched build. /// diff --git a/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs b/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs index 233c495a1d0..29d320ed5ce 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs @@ -8,6 +8,7 @@ using System.Globalization; using System.IO; using System.Linq; +using System.Reflection.Metadata.Ecma335; using System.Threading; using System.Threading.Tasks; using Microsoft.Build.BackEnd.Logging; @@ -1390,29 +1391,17 @@ private void ConfigureWarningsAsErrorsAndMessages() // Ensure everything that is required is available at this time if (project != null && buildEventContext != null && loggingService != null && buildEventContext.ProjectInstanceId != BuildEventContext.InvalidProjectInstanceId) { - if (String.Equals(project.GetEngineRequiredPropertyValue(MSBuildConstants.TreatWarningsAsErrors)?.Trim(), "true", StringComparison.OrdinalIgnoreCase) || - String.Equals(project.GetEngineRequiredPropertyValue(MSBuildConstants.TreatWarningsAsErrorsNoPrefix)?.Trim(), "true", StringComparison.OrdinalIgnoreCase)) + if (String.Equals(project.GetEngineRequiredPropertyValue(MSBuildConstants.MSBuildPrefix + MSBuildConstants.TreatWarningsAsErrors)?.Trim(), "true", StringComparison.OrdinalIgnoreCase) || + (String.Equals(project.GetEngineRequiredPropertyValue(MSBuildConstants.TreatWarningsAsErrors)?.Trim(), "true", StringComparison.OrdinalIgnoreCase) && + ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_14))) { // If signals the IEventSourceSink to treat all warnings as errors loggingService.AddWarningsAsErrors(buildEventContext, new HashSet()); } else { - ISet warningsAsErrors = ParseWarningCodes(project.GetEngineRequiredPropertyValue(MSBuildConstants.WarningsAsErrors)); - var warningsAsErrorsNoPrefix = ParseWarningCodes(project.GetEngineRequiredPropertyValue(MSBuildConstants.WarningsAsErrorsNoPrefix)); - if (warningsAsErrorsNoPrefix != null) - { - if (warningsAsErrors != null) - { - warningsAsErrors.UnionWith(warningsAsErrorsNoPrefix); - } - else - { - warningsAsErrors = warningsAsErrorsNoPrefix; - } - } - - + ISet warningsAsErrors = ParseWarningCodes(project.GetEngineRequiredPropertyValue(MSBuildConstants.MSBuildPrefix + MSBuildConstants.WarningsAsErrors), + project.GetEngineRequiredPropertyValue(MSBuildConstants.WarningsAsErrors)); if (warningsAsErrors?.Count > 0) { @@ -1420,20 +1409,8 @@ private void ConfigureWarningsAsErrorsAndMessages() } } - ISet warningsNotAsErrors = ParseWarningCodes(project.GetEngineRequiredPropertyValue(MSBuildConstants.WarningsNotAsErrors)); - var warningsNotAsErrorsNoPrefix = ParseWarningCodes(project.GetEngineRequiredPropertyValue(MSBuildConstants.WarningsNotAsErrorsNoPrefix)); - if (warningsNotAsErrorsNoPrefix != null) - { - if (warningsNotAsErrors != null) - { - warningsNotAsErrors.UnionWith(warningsNotAsErrorsNoPrefix); - } - else - { - warningsNotAsErrors = warningsNotAsErrorsNoPrefix; - } - } - + ISet warningsNotAsErrors = ParseWarningCodes(project.GetEngineRequiredPropertyValue(MSBuildConstants.MSBuildPrefix + MSBuildConstants.WarningsNotAsErrors), + project.GetEngineRequiredPropertyValue(MSBuildConstants.WarningsNotAsErrors)); if (warningsNotAsErrors?.Count > 0) @@ -1441,13 +1418,8 @@ private void ConfigureWarningsAsErrorsAndMessages() loggingService.AddWarningsNotAsErrors(buildEventContext, warningsNotAsErrors); } - ISet warningsAsMessages = ParseWarningCodes(project.GetEngineRequiredPropertyValue(MSBuildConstants.WarningsAsMessages)); - var warningsAsMessagesNoPrefix = ParseWarningCodes(project.GetEngineRequiredPropertyValue(MSBuildConstants.WarningsAsMessagesNoPrefix)); - if (warningsAsMessagesNoPrefix != null) - { - warningsAsMessages?.UnionWith(warningsAsMessagesNoPrefix); - warningsAsMessages ??= warningsAsMessagesNoPrefix; - } + ISet warningsAsMessages = ParseWarningCodes(project.GetEngineRequiredPropertyValue(MSBuildConstants.MSBuildPrefix + MSBuildConstants.WarningsAsMessages), + project.GetEngineRequiredPropertyValue(MSBuildConstants.WarningsAsMessages)); if (warningsAsMessages?.Count > 0) { @@ -1465,16 +1437,40 @@ private void ConfigureKnownImmutableFolders() } } - private static ISet ParseWarningCodes(string warnings) + private static ISet ParseWarningCodes(string warnings, string warningsNoPrefix) { - if (String.IsNullOrWhiteSpace(warnings)) + // When this changewave is rotated out and this gets deleted, please consider removing + // the $(NoWarn) + // and the two following lines from the msbuild/src/Tasks/Microsoft.Common.CurrentVersion.targets + if (!ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_14)) { - return null; + warningsNoPrefix = null; } - return new HashSet(ExpressionShredder.SplitSemiColonSeparatedList(warnings), StringComparer.OrdinalIgnoreCase); + HashSet result1 = null; + if (!String.IsNullOrWhiteSpace(warnings)) + { + result1 = new HashSet(ExpressionShredder.SplitSemiColonSeparatedList(warnings), StringComparer.OrdinalIgnoreCase); + } + HashSet result2 = null; + if (!String.IsNullOrWhiteSpace(warningsNoPrefix)) + { + result2 = new HashSet(ExpressionShredder.SplitSemiColonSeparatedList(warningsNoPrefix), StringComparer.OrdinalIgnoreCase); + } + + if (result1 != null) + { + if (result2 != null) + { + result1.UnionWith(result2); + } + return result1; + } + + return result2; } + private sealed class DedicatedThreadsTaskScheduler : TaskScheduler { private readonly BlockingCollection _tasks = new BlockingCollection(); diff --git a/src/Shared/Constants.cs b/src/Shared/Constants.cs index 246cff2c646..81d03ed9a0c 100644 --- a/src/Shared/Constants.cs +++ b/src/Shared/Constants.cs @@ -29,45 +29,29 @@ internal static class MSBuildConstants internal const string SdksPath = "MSBuildSDKsPath"; /// - /// Name of the property that indicates that all warnings should be treated as errors. + /// The prefix that was originally used. Now extracted out for the purpose of allowing even the non-prefixed variant. /// - internal const string TreatWarningsAsErrors = "MSBuildTreatWarningsAsErrors"; + internal const string MSBuildPrefix = "MSBuild"; /// /// Name of the property that indicates that all warnings should be treated as errors. /// - internal const string TreatWarningsAsErrorsNoPrefix = "TreatWarningsAsErrors"; + internal const string TreatWarningsAsErrors = "TreatWarningsAsErrors"; /// /// Name of the property that indicates a list of warnings to treat as errors. /// - internal const string WarningsAsErrors = "MSBuildWarningsAsErrors"; - - /// - /// Name of the property that indicates a list of warnings to treat as errors. - /// - internal const string WarningsAsErrorsNoPrefix = "WarningsAsErrors"; - - /// - /// Name of the property that indicates a list of warnings to not treat as errors. - /// - internal const string WarningsNotAsErrors = "MSBuildWarningsNotAsErrors"; + internal const string WarningsAsErrors = "WarningsAsErrors"; /// /// Name of the property that indicates a list of warnings to not treat as errors. /// - internal const string WarningsNotAsErrorsNoPrefix = "WarningsNotAsErrors"; - - /// - /// Name of the property that indicates the list of warnings to treat as messages. - /// - internal const string WarningsAsMessages = "MSBuildWarningsAsMessages"; - + internal const string WarningsNotAsErrors = "WarningsNotAsErrors"; /// /// Name of the property that indicates the list of warnings to treat as messages. /// - internal const string WarningsAsMessagesNoPrefix = "WarningsAsMessages"; + internal const string WarningsAsMessages = "WarningsAsMessages"; /// /// The name of the environment variable that users can specify to override where NuGet assemblies are loaded from in the NuGetSdkResolver. From c3092a0441ef2f10555ce1586c590592e5989be1 Mon Sep 17 00:00:00 2001 From: SimaTian Date: Mon, 18 Nov 2024 20:28:30 +0100 Subject: [PATCH 5/6] addressing review comments --- src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs | 4 ++++ src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs | 2 -- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs index 7a490b59def..a903ee5b609 100644 --- a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs +++ b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs @@ -428,6 +428,7 @@ public void WarningsNotAsErrorsAndMessages_Tests(bool useMSPrefix) [InlineData("WarningsAsErrors", "MSB1007", false)] [InlineData("WarningsAsMessages", "MSB1007", false)] [InlineData("WarningsNotAsErrors", "MSB1007", true)] + [InlineData("WarningsNotAsErrors", "MSB1007", false)] public void WarningsChangeWaveTest(string property, string propertyData, bool treatWarningsAsErrors) { using (TestEnvironment env = TestEnvironment.Create(_output)) @@ -449,6 +450,8 @@ public void WarningsChangeWaveTest(string property, string propertyData, bool tr { // Since the "no prefix" variations can't do anything with the change wave disabled, this should always fail. MockLogger logger = proj.BuildProjectExpectFailure(); + logger.ErrorCount.ShouldBe(1); + logger.AssertLogContains(warningCode); } else { @@ -459,6 +462,7 @@ public void WarningsChangeWaveTest(string property, string propertyData, bool tr logger.AssertLogContains(warningCode); } + ChangeWaves.ResetStateForTests(); } } diff --git a/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs b/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs index d1f15570b59..a5b8e6b9717 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs @@ -8,7 +8,6 @@ using System.Globalization; using System.IO; using System.Linq; -using System.Reflection.Metadata.Ecma335; using System.Threading; using System.Threading.Tasks; using Microsoft.Build.BackEnd.Logging; @@ -1470,7 +1469,6 @@ private static ISet ParseWarningCodes(string warnings, string warningsNo return result2; } - private sealed class DedicatedThreadsTaskScheduler : TaskScheduler { private readonly BlockingCollection _tasks = new BlockingCollection(); From 424796f1863bcc50d6a8e994b0b3084e7d68cd8d Mon Sep 17 00:00:00 2001 From: SimaTian Date: Wed, 20 Nov 2024 11:16:58 +0100 Subject: [PATCH 6/6] final review round, minor test update --- src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs index a903ee5b609..6041fbc45ac 100644 --- a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs +++ b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs @@ -451,6 +451,8 @@ public void WarningsChangeWaveTest(string property, string propertyData, bool tr // Since the "no prefix" variations can't do anything with the change wave disabled, this should always fail. MockLogger logger = proj.BuildProjectExpectFailure(); logger.ErrorCount.ShouldBe(1); + logger.AssertLogContains($"error {warningCode}"); + logger.AssertLogContains(warningCode); } else @@ -458,11 +460,11 @@ public void WarningsChangeWaveTest(string property, string propertyData, bool tr MockLogger logger = proj.BuildProjectExpectSuccess(); logger.WarningCount.ShouldBe(1); + logger.AssertLogContains($"warning {warningCode}"); logger.ErrorCount.ShouldBe(0); logger.AssertLogContains(warningCode); } - ChangeWaves.ResetStateForTests(); } }