From 61233143f53746784868c2e33123d6174f45cd11 Mon Sep 17 00:00:00 2001 From: Farhad Alizada Date: Fri, 24 Nov 2023 10:48:02 +0100 Subject: [PATCH 1/8] Always unquote target parameters --- src/Framework/Traits.cs | 8 ++++ .../CommandLineSwitches_Tests.cs | 37 +++++++++++++++++++ src/MSBuild/CommandLineSwitches.cs | 25 +++++++++++++ 3 files changed, 70 insertions(+) diff --git a/src/Framework/Traits.cs b/src/Framework/Traits.cs index b107fc16186..b99b9d57d9e 100644 --- a/src/Framework/Traits.cs +++ b/src/Framework/Traits.cs @@ -400,6 +400,14 @@ public bool EnableWarningOnCustomBuildEvent } } + public bool UnquoteSwitchParameterForTragetParametrizedSwitch + { + get + { + return ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10); + } + } + private bool? _isBinaryFormatterSerializationAllowed; public bool IsBinaryFormatterSerializationAllowed { diff --git a/src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs b/src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs index a3f744978d0..4f292ded297 100644 --- a/src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs +++ b/src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs @@ -1055,6 +1055,43 @@ public void AppendParameterizedSwitchesTests2() Assert.Equal("build", parameters[2]); } + /// + /// Verifies that the Target property is unquoted and parsed properly. + /// This will remove the possibility to have the ';' in the target name. + /// + [Theory] + [InlineData("/t:Clean;Build", "\"Clean;Build\"")] + [InlineData("/t:Clean;Build", "Clean;Build")] + public void ParameterizedSwitchTargetQuotedTest(string commandLineArg, string switchParameters) + { + CommandLineSwitches switches = new CommandLineSwitches(); + switches.SetParameterizedSwitch(CommandLineSwitches.ParameterizedSwitch.Target, commandLineArg, switchParameters, true, true, false); + Assert.True(switches.IsParameterizedSwitchSet(CommandLineSwitches.ParameterizedSwitch.Target)); + + switches[CommandLineSwitches.ParameterizedSwitch.Target].Length.ShouldBe(2); + switches[CommandLineSwitches.ParameterizedSwitch.Target][0].ShouldBe("Clean"); + switches[CommandLineSwitches.ParameterizedSwitch.Target][1].ShouldBe("Build"); + } + + /// + /// Verifies that the parsing behaviour of quoted target properties is not change when ChangeWave configured. + /// + [Fact] + public void ParameterizedSwitchTargetQuotedChangeWaveTest() + { + using (TestEnvironment env = TestEnvironment.Create()) + { + env.SetEnvironmentVariable("MSBUILDDISABLEFEATURESFROMVERSION", "17.10"); + + CommandLineSwitches switches = new CommandLineSwitches(); + switches.SetParameterizedSwitch(CommandLineSwitches.ParameterizedSwitch.Target, "/t:Clean;Build", "\"Clean;Build\"", true, true, false); + Assert.True(switches.IsParameterizedSwitchSet(CommandLineSwitches.ParameterizedSwitch.Target)); + + switches[CommandLineSwitches.ParameterizedSwitch.Target].Length.ShouldBe(1); + switches[CommandLineSwitches.ParameterizedSwitch.Target][0].ShouldBe("Clean;Build"); + } + } + [Fact] public void AppendParameterizedSwitchesTests3() { diff --git a/src/MSBuild/CommandLineSwitches.cs b/src/MSBuild/CommandLineSwitches.cs index 847da8ba276..5531321c307 100644 --- a/src/MSBuild/CommandLineSwitches.cs +++ b/src/MSBuild/CommandLineSwitches.cs @@ -488,6 +488,11 @@ internal bool SetParameterizedSwitch( } else { + if (IsMultipleAllowedSwithParameterDueToUnquote(unquoteParameters, parameterizedSwitch)) + { + switchParameters = QuotingUtilities.Unquote(switchParameters); + } + // store all the switch parameters int emptyParameters; _parameterizedSwitches[(int)parameterizedSwitch].parameters.AddRange(QuotingUtilities.SplitUnquoted(switchParameters, int.MaxValue, false /* discard empty parameters */, unquoteParameters, out emptyParameters, s_parameterSeparators)); @@ -651,6 +656,26 @@ internal string[][] GetFileLoggerParameters() return groupedFileLoggerParameters; } + /// + /// Checks if the provided parametrized switch needs to be unquoted. + /// The method will return 'true' in case: + /// The changewave 17.10 is not set and + /// The parametrized switch is 'Target' + /// + private bool IsMultipleAllowedSwithParameterDueToUnquote(bool unquoteParameter, ParameterizedSwitch parameterizedSwitch) + { + if (!unquoteParameter || !Traits.Instance.EscapeHatches.UnquoteSwitchParameterForTragetParametrizedSwitch) + { + return false; + } + if (parameterizedSwitch == ParameterizedSwitch.Target) + { + return true; + } + + return false; + } + /// /// If the specified parameterized switch is set, returns the array of parameters. /// Otherwise, if the specified parameterless switch is set, returns an empty array. From a6945bd21d71d244b52fea9d523b81d9272dfa3b Mon Sep 17 00:00:00 2001 From: Farhad Alizada Date: Fri, 24 Nov 2023 10:55:59 +0100 Subject: [PATCH 2/8] Update the name of EscapeHatches variable --- src/Framework/Traits.cs | 2 +- src/MSBuild/CommandLineSwitches.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Framework/Traits.cs b/src/Framework/Traits.cs index b99b9d57d9e..16f54527c99 100644 --- a/src/Framework/Traits.cs +++ b/src/Framework/Traits.cs @@ -400,7 +400,7 @@ public bool EnableWarningOnCustomBuildEvent } } - public bool UnquoteSwitchParameterForTragetParametrizedSwitch + public bool UnquoteTragetSwitchParameters { get { diff --git a/src/MSBuild/CommandLineSwitches.cs b/src/MSBuild/CommandLineSwitches.cs index 5531321c307..226e4dc0f62 100644 --- a/src/MSBuild/CommandLineSwitches.cs +++ b/src/MSBuild/CommandLineSwitches.cs @@ -664,7 +664,7 @@ internal string[][] GetFileLoggerParameters() /// private bool IsMultipleAllowedSwithParameterDueToUnquote(bool unquoteParameter, ParameterizedSwitch parameterizedSwitch) { - if (!unquoteParameter || !Traits.Instance.EscapeHatches.UnquoteSwitchParameterForTragetParametrizedSwitch) + if (!unquoteParameter || !Traits.Instance.EscapeHatches.UnquoteTragetSwitchParameters) { return false; } From fd690f91e2602088dd6b3bb8b5f8a5aa4fb16124 Mon Sep 17 00:00:00 2001 From: Farhad Alizada Date: Fri, 24 Nov 2023 10:59:09 +0100 Subject: [PATCH 3/8] Update the ChangeWaves file --- documentation/wiki/ChangeWaves.md | 1 + 1 file changed, 1 insertion(+) diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md index ee6534cdb36..3a00343839a 100644 --- a/documentation/wiki/ChangeWaves.md +++ b/documentation/wiki/ChangeWaves.md @@ -27,6 +27,7 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t - [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) - [Cache SDK resolver data process-wide](https://github.com/dotnet/msbuild/pull/9335) +- [Target parameters will be unquoted, this will remove the possibility to have ';' in target name ](https://github.com/dotnet/msbuild/pull/9452) ### 17.8 - [[RAR] Don't do I/O on SDK-provided references](https://github.com/dotnet/msbuild/pull/8688) From 40fc0458c249fcbeac9228abbfbe0a0bf96bfec3 Mon Sep 17 00:00:00 2001 From: Farhad Alizada Date: Mon, 27 Nov 2023 14:46:47 +0100 Subject: [PATCH 4/8] Address PR comments, include the more detailed comment about why --- documentation/wiki/ChangeWaves.md | 2 +- src/MSBuild/CommandLineSwitches.cs | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md index 3a00343839a..8164814173e 100644 --- a/documentation/wiki/ChangeWaves.md +++ b/documentation/wiki/ChangeWaves.md @@ -27,7 +27,7 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t - [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) - [Cache SDK resolver data process-wide](https://github.com/dotnet/msbuild/pull/9335) -- [Target parameters will be unquoted, this will remove the possibility to have ';' in target name ](https://github.com/dotnet/msbuild/pull/9452) +- [Target parameters will be unquoted](https://github.com/dotnet/msbuild/pull/9452), meaning the ';' symbol in the target name will be treated as separator ### 17.8 - [[RAR] Don't do I/O on SDK-provided references](https://github.com/dotnet/msbuild/pull/8688) diff --git a/src/MSBuild/CommandLineSwitches.cs b/src/MSBuild/CommandLineSwitches.cs index 226e4dc0f62..3469126c56b 100644 --- a/src/MSBuild/CommandLineSwitches.cs +++ b/src/MSBuild/CommandLineSwitches.cs @@ -668,6 +668,10 @@ private bool IsMultipleAllowedSwithParameterDueToUnquote(bool unquoteParameter, { return false; } + + // issue: https://github.com/dotnet/msbuild/issues/9442 + // In order to align the parsing behaviour of Target property when MSBuild invoked from PowerShell or CMD, + // the target property value will be unquoted before processing further if (parameterizedSwitch == ParameterizedSwitch.Target) { return true; From fec95b64db7554302090eac3167b4f462f509abb Mon Sep 17 00:00:00 2001 From: Farhad Alizada Date: Thu, 30 Nov 2023 12:54:56 +0100 Subject: [PATCH 5/8] Address PR comments, fix namings --- src/Framework/Traits.cs | 2 +- src/MSBuild/CommandLineSwitches.cs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Framework/Traits.cs b/src/Framework/Traits.cs index 16f54527c99..daf68c5c7dc 100644 --- a/src/Framework/Traits.cs +++ b/src/Framework/Traits.cs @@ -400,7 +400,7 @@ public bool EnableWarningOnCustomBuildEvent } } - public bool UnquoteTragetSwitchParameters + public bool UnquoteTargetSwitchParameters { get { diff --git a/src/MSBuild/CommandLineSwitches.cs b/src/MSBuild/CommandLineSwitches.cs index 3469126c56b..73d36380875 100644 --- a/src/MSBuild/CommandLineSwitches.cs +++ b/src/MSBuild/CommandLineSwitches.cs @@ -488,7 +488,7 @@ internal bool SetParameterizedSwitch( } else { - if (IsMultipleAllowedSwithParameterDueToUnquote(unquoteParameters, parameterizedSwitch)) + if (IsMultipleAllowedSwitchParameterDueToUnquote(unquoteParameters, parameterizedSwitch)) { switchParameters = QuotingUtilities.Unquote(switchParameters); } @@ -662,9 +662,9 @@ internal string[][] GetFileLoggerParameters() /// The changewave 17.10 is not set and /// The parametrized switch is 'Target' /// - private bool IsMultipleAllowedSwithParameterDueToUnquote(bool unquoteParameter, ParameterizedSwitch parameterizedSwitch) + private bool IsMultipleAllowedSwitchParameterDueToUnquote(bool unquoteParameter, ParameterizedSwitch parameterizedSwitch) { - if (!unquoteParameter || !Traits.Instance.EscapeHatches.UnquoteTragetSwitchParameters) + if (!unquoteParameter || !Traits.Instance.EscapeHatches.UnquoteTargetSwitchParameters) { return false; } From d86f924814b1a93d57993fd16f09148456f57621 Mon Sep 17 00:00:00 2001 From: Farhad Alizada Date: Fri, 1 Dec 2023 11:14:19 +0100 Subject: [PATCH 6/8] Address PR comments, update code documentation --- src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs | 7 ++++--- src/MSBuild/CommandLineSwitches.cs | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs b/src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs index 4f292ded297..735b63da358 100644 --- a/src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs +++ b/src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs @@ -1066,15 +1066,16 @@ public void ParameterizedSwitchTargetQuotedTest(string commandLineArg, string sw { CommandLineSwitches switches = new CommandLineSwitches(); switches.SetParameterizedSwitch(CommandLineSwitches.ParameterizedSwitch.Target, commandLineArg, switchParameters, true, true, false); - Assert.True(switches.IsParameterizedSwitchSet(CommandLineSwitches.ParameterizedSwitch.Target)); + switches.IsParameterizedSwitchSet(CommandLineSwitches.ParameterizedSwitch.Target).ShouldBeTrue(); switches[CommandLineSwitches.ParameterizedSwitch.Target].Length.ShouldBe(2); switches[CommandLineSwitches.ParameterizedSwitch.Target][0].ShouldBe("Clean"); switches[CommandLineSwitches.ParameterizedSwitch.Target][1].ShouldBe("Build"); + switches.GetParameterizedSwitchCommandLineArg(CommandLineSwitches.ParameterizedSwitch.Target).ShouldBe(commandLineArg); } /// - /// Verifies that the parsing behaviour of quoted target properties is not change when ChangeWave configured. + /// Verifies that the parsing behavior of quoted target properties is not changed when ChangeWave configured. /// [Fact] public void ParameterizedSwitchTargetQuotedChangeWaveTest() @@ -1085,7 +1086,7 @@ public void ParameterizedSwitchTargetQuotedChangeWaveTest() CommandLineSwitches switches = new CommandLineSwitches(); switches.SetParameterizedSwitch(CommandLineSwitches.ParameterizedSwitch.Target, "/t:Clean;Build", "\"Clean;Build\"", true, true, false); - Assert.True(switches.IsParameterizedSwitchSet(CommandLineSwitches.ParameterizedSwitch.Target)); + switches.IsParameterizedSwitchSet(CommandLineSwitches.ParameterizedSwitch.Target).ShouldBeTrue(); switches[CommandLineSwitches.ParameterizedSwitch.Target].Length.ShouldBe(1); switches[CommandLineSwitches.ParameterizedSwitch.Target][0].ShouldBe("Clean;Build"); diff --git a/src/MSBuild/CommandLineSwitches.cs b/src/MSBuild/CommandLineSwitches.cs index 73d36380875..3da040a200a 100644 --- a/src/MSBuild/CommandLineSwitches.cs +++ b/src/MSBuild/CommandLineSwitches.cs @@ -657,7 +657,7 @@ internal string[][] GetFileLoggerParameters() } /// - /// Checks if the provided parametrized switch needs to be unquoted. + /// Checks if the provided multiple valued parametrized switch needs to be unquoted. /// The method will return 'true' in case: /// The changewave 17.10 is not set and /// The parametrized switch is 'Target' From 6f31e78663cf1d98798775a6f2d6b711e085cfd3 Mon Sep 17 00:00:00 2001 From: Farhad Alizada Date: Fri, 1 Dec 2023 11:59:06 +0100 Subject: [PATCH 7/8] Update change wave description --- documentation/wiki/ChangeWaves.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md index 8164814173e..f2c3ae6f35c 100644 --- a/documentation/wiki/ChangeWaves.md +++ b/documentation/wiki/ChangeWaves.md @@ -27,7 +27,7 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t - [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) - [Cache SDK resolver data process-wide](https://github.com/dotnet/msbuild/pull/9335) -- [Target parameters will be unquoted](https://github.com/dotnet/msbuild/pull/9452), meaning the ';' symbol in the target name will be treated as separator +- [Target parameters will be unquoted](https://github.com/dotnet/msbuild/pull/9452), meaning the ';' symbol in the target name will always be treated as separator ### 17.8 - [[RAR] Don't do I/O on SDK-provided references](https://github.com/dotnet/msbuild/pull/8688) From e32b40ae1347105d948d4b26a248b23eeb7788fd Mon Sep 17 00:00:00 2001 From: Farhad Alizada Date: Fri, 1 Dec 2023 11:59:50 +0100 Subject: [PATCH 8/8] specify parameter in change wave --- documentation/wiki/ChangeWaves.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md index f2c3ae6f35c..a2a867cec05 100644 --- a/documentation/wiki/ChangeWaves.md +++ b/documentation/wiki/ChangeWaves.md @@ -27,7 +27,7 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t - [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) - [Cache SDK resolver data process-wide](https://github.com/dotnet/msbuild/pull/9335) -- [Target parameters will be unquoted](https://github.com/dotnet/msbuild/pull/9452), meaning the ';' symbol in the target name will always be treated as separator +- [Target parameters will be unquoted](https://github.com/dotnet/msbuild/pull/9452), meaning the ';' symbol in the parameter target name will always be treated as separator ### 17.8 - [[RAR] Don't do I/O on SDK-provided references](https://github.com/dotnet/msbuild/pull/8688)