From 7d4f1ce304e4d532bbcb5bd8263987be05d82c54 Mon Sep 17 00:00:00 2001 From: Chet Husk Date: Thu, 23 Jun 2022 10:14:29 -0500 Subject: [PATCH 01/12] add simple token-per-line replacement support --- src/Cli/dotnet/Parser.cs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/Cli/dotnet/Parser.cs b/src/Cli/dotnet/Parser.cs index 26dd91304ec1..958a11d50f27 100644 --- a/src/Cli/dotnet/Parser.cs +++ b/src/Cli/dotnet/Parser.cs @@ -109,6 +109,24 @@ public static Command GetBuiltInCommand(string commandName) .FirstOrDefault(c => c.Name.Equals(commandName, StringComparison.OrdinalIgnoreCase)); } + /// + /// Implements token-per-line response file handling for the CLI. We use this instead of the built-in S.CL handling + /// to ensure backwards-compatibility with MSBuild. + /// + public static bool TokenPerLine(string tokenToReplace, out IReadOnlyList replacementTokens, out string errorMessage) { + var filePath = Path.GetFullPath(tokenToReplace); + if (File.Exists(filePath)) { + replacementTokens = File.ReadAllLines(filePath).Where(line => !line.StartsWith("#")).ToArray(); + errorMessage = null; + return true; + } else { + // don't report an error here, since MSBuild doesn't in the case of nonexistent response files + replacementTokens = null; + errorMessage = null; + return false; + } + } + public static System.CommandLine.Parsing.Parser Instance { get; } = new CommandLineBuilder(ConfigureCommandLine(RootCommand)) .UseExceptionHandler(ExceptionHandler) .UseHelp() @@ -118,6 +136,7 @@ public static Command GetBuiltInCommand(string commandName) .UseSuggestDirective() .DisablePosixBinding() .EnableLegacyDoubleDashBehavior() + .UseTokenReplacer(TokenPerLine) .Build(); private static void ExceptionHandler(Exception exception, InvocationContext context) From 9825b83e89c0c3f8631a72cdf44d0eaf0b2fdab5 Mon Sep 17 00:00:00 2001 From: Chet Husk Date: Thu, 23 Jun 2022 14:15:41 -0500 Subject: [PATCH 02/12] wrap line tokens in quotes to ensure correct parsing --- src/Cli/dotnet/Parser.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Cli/dotnet/Parser.cs b/src/Cli/dotnet/Parser.cs index 958a11d50f27..337bec216b1a 100644 --- a/src/Cli/dotnet/Parser.cs +++ b/src/Cli/dotnet/Parser.cs @@ -116,7 +116,7 @@ public static Command GetBuiltInCommand(string commandName) public static bool TokenPerLine(string tokenToReplace, out IReadOnlyList replacementTokens, out string errorMessage) { var filePath = Path.GetFullPath(tokenToReplace); if (File.Exists(filePath)) { - replacementTokens = File.ReadAllLines(filePath).Where(line => !line.StartsWith("#")).ToArray(); + replacementTokens = File.ReadAllLines(filePath).Where(line => !line.StartsWith("#")).Select(line => $"\"{line}\"").ToArray(); errorMessage = null; return true; } else { From 08e36611abdebffb19990bca403eb69ad3c28845 Mon Sep 17 00:00:00 2001 From: Chet Husk Date: Thu, 23 Jun 2022 14:35:37 -0500 Subject: [PATCH 03/12] add test for token replacement --- .../ParserTests/ResponseFileTests.cs | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 src/Tests/dotnet.Tests/ParserTests/ResponseFileTests.cs diff --git a/src/Tests/dotnet.Tests/ParserTests/ResponseFileTests.cs b/src/Tests/dotnet.Tests/ParserTests/ResponseFileTests.cs new file mode 100644 index 000000000000..1c25b368c5c6 --- /dev/null +++ b/src/Tests/dotnet.Tests/ParserTests/ResponseFileTests.cs @@ -0,0 +1,37 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System.CommandLine.Parsing; +using FluentAssertions; +using Microsoft.DotNet.Cli; +using Xunit; +using Xunit.Abstractions; +using Parser = Microsoft.DotNet.Cli.Parser; +using System.IO; +using System.Linq; + +namespace Microsoft.DotNet.Tests.ParserTests +{ + public class ResponseFileTests + { + [Fact] + public void Can_safely_expand_response_file_lines() { + var tempFilePath = Path.GetTempFileName(); + var lines = new[] { + "build", + "a b", + "/p=\".;c:/program files/\"" + }; + File.WriteAllLines(tempFilePath, lines); + + var quoted = lines.Select(l => $"\"{l}\""); + var parser = Parser.Instance; + var parseResult = parser.Parse(new []{ "dotnet", $"@{tempFilePath}" }); + var tokens = parseResult.Tokens.Select(t => t.Value); + tokens.Should().HaveCount(lines.Length + 1); // dotnet token too + tokens.Should().StartWith("dotnet"); + tokens.Skip(1).Should().BeEquivalentTo(quoted); + } + } +} + \ No newline at end of file From 81bbfe983b48adfeba5d2f5194e6d2ac1412139e Mon Sep 17 00:00:00 2001 From: Chet Husk Date: Thu, 23 Jun 2022 15:10:30 -0500 Subject: [PATCH 04/12] correct error message handling --- src/Cli/dotnet/CommonLocalizableStrings.resx | 3 +++ src/Cli/dotnet/Parser.cs | 3 +-- src/Cli/dotnet/xlf/CommonLocalizableStrings.cs.xlf | 5 +++++ src/Cli/dotnet/xlf/CommonLocalizableStrings.de.xlf | 5 +++++ src/Cli/dotnet/xlf/CommonLocalizableStrings.es.xlf | 5 +++++ src/Cli/dotnet/xlf/CommonLocalizableStrings.fr.xlf | 5 +++++ src/Cli/dotnet/xlf/CommonLocalizableStrings.it.xlf | 5 +++++ src/Cli/dotnet/xlf/CommonLocalizableStrings.ja.xlf | 5 +++++ src/Cli/dotnet/xlf/CommonLocalizableStrings.ko.xlf | 5 +++++ src/Cli/dotnet/xlf/CommonLocalizableStrings.pl.xlf | 5 +++++ src/Cli/dotnet/xlf/CommonLocalizableStrings.pt-BR.xlf | 5 +++++ src/Cli/dotnet/xlf/CommonLocalizableStrings.ru.xlf | 5 +++++ src/Cli/dotnet/xlf/CommonLocalizableStrings.tr.xlf | 5 +++++ src/Cli/dotnet/xlf/CommonLocalizableStrings.zh-Hans.xlf | 5 +++++ src/Cli/dotnet/xlf/CommonLocalizableStrings.zh-Hant.xlf | 5 +++++ 15 files changed, 69 insertions(+), 2 deletions(-) diff --git a/src/Cli/dotnet/CommonLocalizableStrings.resx b/src/Cli/dotnet/CommonLocalizableStrings.resx index c5a8fc30e679..98894eda579d 100644 --- a/src/Cli/dotnet/CommonLocalizableStrings.resx +++ b/src/Cli/dotnet/CommonLocalizableStrings.resx @@ -699,4 +699,7 @@ The default is 'true' if a runtime identifier is specified. The '--self-contained' and '--no-self-contained' options cannot be used together. + + Response file '{0}' does not exist. + diff --git a/src/Cli/dotnet/Parser.cs b/src/Cli/dotnet/Parser.cs index 337bec216b1a..1f8e20b814f8 100644 --- a/src/Cli/dotnet/Parser.cs +++ b/src/Cli/dotnet/Parser.cs @@ -120,9 +120,8 @@ public static bool TokenPerLine(string tokenToReplace, out IReadOnlyList errorMessage = null; return true; } else { - // don't report an error here, since MSBuild doesn't in the case of nonexistent response files replacementTokens = null; - errorMessage = null; + errorMessage = string.Format(CommonLocalizableStrings.ResponseFileNotFound, tokenToReplace); return false; } } diff --git a/src/Cli/dotnet/xlf/CommonLocalizableStrings.cs.xlf b/src/Cli/dotnet/xlf/CommonLocalizableStrings.cs.xlf index 8b21e9c6da70..ab55535b4e0d 100644 --- a/src/Cli/dotnet/xlf/CommonLocalizableStrings.cs.xlf +++ b/src/Cli/dotnet/xlf/CommonLocalizableStrings.cs.xlf @@ -165,6 +165,11 @@ export PATH="$PATH:{0}" Balíček + + Response file '{0}' does not exist. + Response file '{0}' does not exist. + + The '--self-contained' and '--no-self-contained' options cannot be used together. Možnosti --self-contained a --no-self-contained nelze použít společně. diff --git a/src/Cli/dotnet/xlf/CommonLocalizableStrings.de.xlf b/src/Cli/dotnet/xlf/CommonLocalizableStrings.de.xlf index 55f1241ade52..af311703c7b5 100644 --- a/src/Cli/dotnet/xlf/CommonLocalizableStrings.de.xlf +++ b/src/Cli/dotnet/xlf/CommonLocalizableStrings.de.xlf @@ -165,6 +165,11 @@ export PATH="$PATH:{0}" Paket + + Response file '{0}' does not exist. + Response file '{0}' does not exist. + + The '--self-contained' and '--no-self-contained' options cannot be used together. Die Optionen „--self-contained“ und „--no-self-contained“ können nicht gemeinsam verwendet werden. diff --git a/src/Cli/dotnet/xlf/CommonLocalizableStrings.es.xlf b/src/Cli/dotnet/xlf/CommonLocalizableStrings.es.xlf index d022ec3fd8ce..c6dd08052b2a 100644 --- a/src/Cli/dotnet/xlf/CommonLocalizableStrings.es.xlf +++ b/src/Cli/dotnet/xlf/CommonLocalizableStrings.es.xlf @@ -165,6 +165,11 @@ export PATH="$PATH:{0}" Paquete + + Response file '{0}' does not exist. + Response file '{0}' does not exist. + + The '--self-contained' and '--no-self-contained' options cannot be used together. Las opciones '--self-contained' y '--no-self-contained' no se pueden usar juntas. diff --git a/src/Cli/dotnet/xlf/CommonLocalizableStrings.fr.xlf b/src/Cli/dotnet/xlf/CommonLocalizableStrings.fr.xlf index c9c7c017e20d..fd9d0ca24ba4 100644 --- a/src/Cli/dotnet/xlf/CommonLocalizableStrings.fr.xlf +++ b/src/Cli/dotnet/xlf/CommonLocalizableStrings.fr.xlf @@ -165,6 +165,11 @@ export PATH="$PATH:{0}" Package + + Response file '{0}' does not exist. + Response file '{0}' does not exist. + + The '--self-contained' and '--no-self-contained' options cannot be used together. Les options « --autonome » et « --non-autonome » ne peuvent pas être utilisées ensemble. diff --git a/src/Cli/dotnet/xlf/CommonLocalizableStrings.it.xlf b/src/Cli/dotnet/xlf/CommonLocalizableStrings.it.xlf index 98983855a9ec..ba3845df641b 100644 --- a/src/Cli/dotnet/xlf/CommonLocalizableStrings.it.xlf +++ b/src/Cli/dotnet/xlf/CommonLocalizableStrings.it.xlf @@ -165,6 +165,11 @@ export PATH="$PATH:{0}" Pacchetto + + Response file '{0}' does not exist. + Response file '{0}' does not exist. + + The '--self-contained' and '--no-self-contained' options cannot be used together. Non è possibile usare contemporaneamente le opzioni '--self-contained' e ‘--no-self-contained'. diff --git a/src/Cli/dotnet/xlf/CommonLocalizableStrings.ja.xlf b/src/Cli/dotnet/xlf/CommonLocalizableStrings.ja.xlf index 26ea992f6010..02e775fcbc1c 100644 --- a/src/Cli/dotnet/xlf/CommonLocalizableStrings.ja.xlf +++ b/src/Cli/dotnet/xlf/CommonLocalizableStrings.ja.xlf @@ -165,6 +165,11 @@ export PATH="$PATH:{0}" パッケージ + + Response file '{0}' does not exist. + Response file '{0}' does not exist. + + The '--self-contained' and '--no-self-contained' options cannot be used together. '--self-contained' と '--no-self-contained' オプションは同時に使用できません。 diff --git a/src/Cli/dotnet/xlf/CommonLocalizableStrings.ko.xlf b/src/Cli/dotnet/xlf/CommonLocalizableStrings.ko.xlf index 7f8dcd3075aa..cb6f226b31e6 100644 --- a/src/Cli/dotnet/xlf/CommonLocalizableStrings.ko.xlf +++ b/src/Cli/dotnet/xlf/CommonLocalizableStrings.ko.xlf @@ -165,6 +165,11 @@ export PATH="$PATH:{0}" 패키지 + + Response file '{0}' does not exist. + Response file '{0}' does not exist. + + The '--self-contained' and '--no-self-contained' options cannot be used together. '--self-contained' 및 '--no-self-contained' 옵션은 함께 사용할 수 없습니다. diff --git a/src/Cli/dotnet/xlf/CommonLocalizableStrings.pl.xlf b/src/Cli/dotnet/xlf/CommonLocalizableStrings.pl.xlf index f71dfe006c4f..1edbab21c835 100644 --- a/src/Cli/dotnet/xlf/CommonLocalizableStrings.pl.xlf +++ b/src/Cli/dotnet/xlf/CommonLocalizableStrings.pl.xlf @@ -165,6 +165,11 @@ export PATH="$PATH:{0}" Pakiet + + Response file '{0}' does not exist. + Response file '{0}' does not exist. + + The '--self-contained' and '--no-self-contained' options cannot be used together. Opcji „--self-contained” i „--no-self-contained” nie można używać razem. diff --git a/src/Cli/dotnet/xlf/CommonLocalizableStrings.pt-BR.xlf b/src/Cli/dotnet/xlf/CommonLocalizableStrings.pt-BR.xlf index 6d99aa9c08e1..dfbecdb49bdf 100644 --- a/src/Cli/dotnet/xlf/CommonLocalizableStrings.pt-BR.xlf +++ b/src/Cli/dotnet/xlf/CommonLocalizableStrings.pt-BR.xlf @@ -165,6 +165,11 @@ export PATH="$PATH:{0}" Pacote + + Response file '{0}' does not exist. + Response file '{0}' does not exist. + + The '--self-contained' and '--no-self-contained' options cannot be used together. As opções '--auto--contidas ' e '--não-independentes ' não podem ser usadas juntas. diff --git a/src/Cli/dotnet/xlf/CommonLocalizableStrings.ru.xlf b/src/Cli/dotnet/xlf/CommonLocalizableStrings.ru.xlf index 6ae4c885b46d..a70c3976fbcb 100644 --- a/src/Cli/dotnet/xlf/CommonLocalizableStrings.ru.xlf +++ b/src/Cli/dotnet/xlf/CommonLocalizableStrings.ru.xlf @@ -165,6 +165,11 @@ export PATH="$PATH:{0}" Пакет + + Response file '{0}' does not exist. + Response file '{0}' does not exist. + + The '--self-contained' and '--no-self-contained' options cannot be used together. Параметры "--self-contained" и "--no-self-contained" нельзя использовать вместе. diff --git a/src/Cli/dotnet/xlf/CommonLocalizableStrings.tr.xlf b/src/Cli/dotnet/xlf/CommonLocalizableStrings.tr.xlf index 472e4c6cf1f3..e4f91923860d 100644 --- a/src/Cli/dotnet/xlf/CommonLocalizableStrings.tr.xlf +++ b/src/Cli/dotnet/xlf/CommonLocalizableStrings.tr.xlf @@ -165,6 +165,11 @@ export PATH="$PATH:{0}" Paket + + Response file '{0}' does not exist. + Response file '{0}' does not exist. + + The '--self-contained' and '--no-self-contained' options cannot be used together. '--self-contained' ve '--no-self-contained' seçenekleri birlikte kullanılamaz. diff --git a/src/Cli/dotnet/xlf/CommonLocalizableStrings.zh-Hans.xlf b/src/Cli/dotnet/xlf/CommonLocalizableStrings.zh-Hans.xlf index d14001bc78b3..22b55dbff8e8 100644 --- a/src/Cli/dotnet/xlf/CommonLocalizableStrings.zh-Hans.xlf +++ b/src/Cli/dotnet/xlf/CommonLocalizableStrings.zh-Hans.xlf @@ -165,6 +165,11 @@ EOF 打包 + + Response file '{0}' does not exist. + Response file '{0}' does not exist. + + The '--self-contained' and '--no-self-contained' options cannot be used together. “--自包含”和“--非自包含”选项不能一起使用。 diff --git a/src/Cli/dotnet/xlf/CommonLocalizableStrings.zh-Hant.xlf b/src/Cli/dotnet/xlf/CommonLocalizableStrings.zh-Hant.xlf index 4e361b5e19c3..0f062a717a13 100644 --- a/src/Cli/dotnet/xlf/CommonLocalizableStrings.zh-Hant.xlf +++ b/src/Cli/dotnet/xlf/CommonLocalizableStrings.zh-Hant.xlf @@ -165,6 +165,11 @@ export PATH="$PATH:{0}" 套件 + + Response file '{0}' does not exist. + Response file '{0}' does not exist. + + The '--self-contained' and '--no-self-contained' options cannot be used together. 不能同時使用 '--self-contained' 和 '--no-self-contained' 選項。 From d3326e460c27e8b94342dd3c367593f836b06710 Mon Sep 17 00:00:00 2001 From: Chet Husk Date: Thu, 23 Jun 2022 17:14:48 -0500 Subject: [PATCH 05/12] don't needlessly quote things, instead intelligently forward and escape msbuild parameters --- src/Cli/dotnet/CommonOptions.cs | 2 +- src/Cli/dotnet/OptionForwardingExtensions.cs | 6 ++- src/Cli/dotnet/Parser.cs | 18 ++++++++- .../dotnet-msbuild/MSBuildForwardingApp.cs | 2 + .../ParserTests/ResponseFileTests.cs | 37 +++++++++++++------ 5 files changed, 51 insertions(+), 14 deletions(-) diff --git a/src/Cli/dotnet/CommonOptions.cs b/src/Cli/dotnet/CommonOptions.cs index 3da02f7aba1e..dc3830e55807 100644 --- a/src/Cli/dotnet/CommonOptions.cs +++ b/src/Cli/dotnet/CommonOptions.cs @@ -17,7 +17,7 @@ namespace Microsoft.DotNet.Cli internal static class CommonOptions { public static Option PropertiesOption = - new ForwardedOption(new string[] { "-property", "/p" }) + new ForwardedOption(new string[] { "-property", "/p", "-p" }) { IsHidden = true }.ForwardAsProperty() diff --git a/src/Cli/dotnet/OptionForwardingExtensions.cs b/src/Cli/dotnet/OptionForwardingExtensions.cs index c67a4a7f8d3d..c05a0e109b0c 100644 --- a/src/Cli/dotnet/OptionForwardingExtensions.cs +++ b/src/Cli/dotnet/OptionForwardingExtensions.cs @@ -19,7 +19,11 @@ public static class OptionForwardingExtensions public static ForwardedOption ForwardAsSingle(this ForwardedOption option, Func format) => option.SetForwardingFunction(format); public static ForwardedOption ForwardAsProperty(this ForwardedOption option) => option - .SetForwardingFunction((optionVals) => optionVals.SelectMany(optionVal => new string[] { $"{option.Aliases.FirstOrDefault()}:{optionVal.Replace("roperty:", string.Empty)}" })); + .SetForwardingFunction((optionVals) => + optionVals + .Select(optionVal => optionVal.Replace(";", "%3B")) // must escape semicolon-delimited property values when forwarding them to MSBuild + .SelectMany(optionVal => new string[] { $"{option.Aliases.FirstOrDefault()}:{optionVal.Replace("roperty:", string.Empty)}" }) + ); public static Option ForwardAsMany(this ForwardedOption option, Func> format) => option.SetForwardingFunction(format); diff --git a/src/Cli/dotnet/Parser.cs b/src/Cli/dotnet/Parser.cs index 1f8e20b814f8..4c0cf639ba52 100644 --- a/src/Cli/dotnet/Parser.cs +++ b/src/Cli/dotnet/Parser.cs @@ -116,7 +116,23 @@ public static Command GetBuiltInCommand(string commandName) public static bool TokenPerLine(string tokenToReplace, out IReadOnlyList replacementTokens, out string errorMessage) { var filePath = Path.GetFullPath(tokenToReplace); if (File.Exists(filePath)) { - replacementTokens = File.ReadAllLines(filePath).Where(line => !line.StartsWith("#")).Select(line => $"\"{line}\"").ToArray(); + var lines = File.ReadAllLines(filePath); + var trimmedLines = + lines + // Remove content in the lines that contain #, starting from the point of the # + .Select(line => { + var hashPos = line.IndexOf('#'); + if (hashPos == -1) { + return line; + } else if (hashPos == 0) { + return ""; + } else { + return line.Substring(hashPos).Trim(); + } + }) + // Remove empty lines + .Where(line => line.Length > 0); + replacementTokens = trimmedLines.ToArray(); errorMessage = null; return true; } else { diff --git a/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs b/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs index 18a402fdb855..3c36a0c6b9e8 100644 --- a/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs +++ b/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs @@ -57,6 +57,8 @@ public MSBuildForwardingApp(IEnumerable argsToForward, string msbuildPat } } + public IEnumerable MSBuildArguments { get { return _forwardingAppWithoutLogging.GetAllArguments(); } } + public void EnvironmentVariable(string name, string value) { _forwardingAppWithoutLogging.EnvironmentVariable(name, value); diff --git a/src/Tests/dotnet.Tests/ParserTests/ResponseFileTests.cs b/src/Tests/dotnet.Tests/ParserTests/ResponseFileTests.cs index 1c25b368c5c6..53e7a19f7a83 100644 --- a/src/Tests/dotnet.Tests/ParserTests/ResponseFileTests.cs +++ b/src/Tests/dotnet.Tests/ParserTests/ResponseFileTests.cs @@ -1,37 +1,52 @@ // Copyright (c) .NET Foundation and contributors. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. -using System.CommandLine.Parsing; using FluentAssertions; using Microsoft.DotNet.Cli; -using Xunit; -using Xunit.Abstractions; +using Microsoft.NET.TestFramework; using Parser = Microsoft.DotNet.Cli.Parser; +using System.CommandLine.Parsing; using System.IO; using System.Linq; +using Xunit; +using Xunit.Abstractions; namespace Microsoft.DotNet.Tests.ParserTests { - public class ResponseFileTests + public class ResponseFileTests : SdkTest { + public ResponseFileTests(ITestOutputHelper output) : base(output) + { + } + [Fact] public void Can_safely_expand_response_file_lines() { - var tempFilePath = Path.GetTempFileName(); + var tempFileDir = _testAssetsManager.CreateTestDirectory().Path; + var tempFilePath = Path.Combine(tempFileDir, "params.rsp"); var lines = new[] { "build", "a b", - "/p=\".;c:/program files/\"" + "-p:VSTestTestAdapterPath=\".;/opt/buildagent/plugins/dotnet/tools/vstest15\"" }; File.WriteAllLines(tempFilePath, lines); - var quoted = lines.Select(l => $"\"{l}\""); var parser = Parser.Instance; var parseResult = parser.Parse(new []{ "dotnet", $"@{tempFilePath}" }); + var tokens = parseResult.Tokens.Select(t => t.Value); - tokens.Should().HaveCount(lines.Length + 1); // dotnet token too - tokens.Should().StartWith("dotnet"); - tokens.Skip(1).Should().BeEquivalentTo(quoted); + var tokenString = string.Join(", ", tokens); + var bc = Microsoft.DotNet.Tools.Build.BuildCommand.FromParseResult(parseResult); + var tokenized = new [] { + "build", + "a b", + "-p", + "VSTestTestAdapterPath=\".;/opt/buildagent/plugins/dotnet/tools/vstest15\"" + }; + + Log.WriteLine($"MSbuild Args are {string.Join(" ", bc.MSBuildArguments)}"); + Log.WriteLine($"Parse Diagram is {parseResult.Diagram()}"); + Log.WriteLine($"Token string is {tokenString}"); + tokens.Skip(1).Should().BeEquivalentTo(tokenized); } } } - \ No newline at end of file From fadc9aa5cd6e0ca10ebd2a978bba54261dd1b7a6 Mon Sep 17 00:00:00 2001 From: Chet Husk Date: Thu, 23 Jun 2022 23:52:39 -0500 Subject: [PATCH 06/12] fix property forwarding tests --- .../ParserTests/MSBuildArgumentCommandLineParserTests.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs b/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs index 44a365c38d7c..1f5d13225e1f 100644 --- a/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs +++ b/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs @@ -31,9 +31,10 @@ public void MSBuildArgumentsAreForwardedCorrectly(string[] arguments, bool build RestoringCommand command = buildCommand ? BuildCommand.FromArgs(arguments) : PublishCommand.FromArgs(arguments); + var expectedArguments = arguments.Select(a => a.Replace("-p:", "-property:")); command.GetArgumentsToMSBuild().Split(' ') .Should() - .Contain(arguments); + .Contain(expectedArguments); } } } From 5da0f6f0f645c918010afae15fa7834f63cb676c Mon Sep 17 00:00:00 2001 From: Chet Husk Date: Thu, 23 Jun 2022 23:54:26 -0500 Subject: [PATCH 07/12] fix property forwarding test --- .../ParserTests/MSBuildArgumentCommandLineParserTests.cs | 1 + .../dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs b/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs index 1f5d13225e1f..5813c28b4ec4 100644 --- a/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs +++ b/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs @@ -5,6 +5,7 @@ using Microsoft.DotNet.Tools; using Microsoft.DotNet.Tools.Build; using Microsoft.DotNet.Tools.Publish; +using System.Linq; using Xunit; using Xunit.Abstractions; diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs index f738ad894ca5..bc0e34bb44db 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs @@ -56,7 +56,7 @@ public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalA [Theory] [InlineData(new string[] { "-f", "tfm" }, "-target:Restore", "-property:TargetFramework=tfm")] - [InlineData(new string[] { "-p:TargetFramework=tfm" }, "-target:Restore", "-p:TargetFramework=tfm")] + [InlineData(new string[] { "-p:TargetFramework=tfm" }, "-target:Restore", "-property:TargetFramework=tfm")] [InlineData(new string[] { "/p:TargetFramework=tfm" }, "-target:Restore", "-property:TargetFramework=tfm")] [InlineData(new string[] { "-t:Run", "-f", "tfm" }, "-target:Restore", "-property:TargetFramework=tfm -t:Run")] [InlineData(new string[] { "/t:Run", "-f", "tfm" }, "-target:Restore", "-property:TargetFramework=tfm /t:Run")] From 71acc3a5e1c8ba51196314ae458306fd09dafe9e Mon Sep 17 00:00:00 2001 From: Chet Husk Date: Fri, 24 Jun 2022 12:07:37 -0500 Subject: [PATCH 08/12] additional property forms and testing --- src/Cli/dotnet/CommonOptions.cs | 3 ++- src/Cli/dotnet/OptionForwardingExtensions.cs | 2 +- .../MSBuildArgumentCommandLineParserTests.cs | 14 +++++++++++++- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/Cli/dotnet/CommonOptions.cs b/src/Cli/dotnet/CommonOptions.cs index dc3830e55807..864004b20bc2 100644 --- a/src/Cli/dotnet/CommonOptions.cs +++ b/src/Cli/dotnet/CommonOptions.cs @@ -17,7 +17,8 @@ namespace Microsoft.DotNet.Cli internal static class CommonOptions { public static Option PropertiesOption = - new ForwardedOption(new string[] { "-property", "/p", "-p" }) + // these are all of the forms that the property switch can be understood by in MSBuild + new ForwardedOption(new string[] { "--property", "-property", "/property", "/p", "-p", "--p" }) { IsHidden = true }.ForwardAsProperty() diff --git a/src/Cli/dotnet/OptionForwardingExtensions.cs b/src/Cli/dotnet/OptionForwardingExtensions.cs index c05a0e109b0c..b5d24dd29555 100644 --- a/src/Cli/dotnet/OptionForwardingExtensions.cs +++ b/src/Cli/dotnet/OptionForwardingExtensions.cs @@ -22,7 +22,7 @@ public static ForwardedOption ForwardAsProperty(this ForwardedOption optionVals .Select(optionVal => optionVal.Replace(";", "%3B")) // must escape semicolon-delimited property values when forwarding them to MSBuild - .SelectMany(optionVal => new string[] { $"{option.Aliases.FirstOrDefault()}:{optionVal.Replace("roperty:", string.Empty)}" }) + .Select(optionVal => $"{option.Aliases.FirstOrDefault()}:{optionVal}") ); public static Option ForwardAsMany(this ForwardedOption option, Func> format) => option.SetForwardingFunction(format); diff --git a/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs b/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs index 5813c28b4ec4..deb5d9313da8 100644 --- a/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs +++ b/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs @@ -2,9 +2,11 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using FluentAssertions; +using Microsoft.DotNet.Cli; using Microsoft.DotNet.Tools; using Microsoft.DotNet.Tools.Build; using Microsoft.DotNet.Tools.Publish; +using System.CommandLine; using System.Linq; using Xunit; using Xunit.Abstractions; @@ -32,10 +34,20 @@ public void MSBuildArgumentsAreForwardedCorrectly(string[] arguments, bool build RestoringCommand command = buildCommand ? BuildCommand.FromArgs(arguments) : PublishCommand.FromArgs(arguments); - var expectedArguments = arguments.Select(a => a.Replace("-p:", "-property:")); + var expectedArguments = arguments.Select(a => a.Replace("-property:", "--property:").Replace("-p:", "--property:")); command.GetArgumentsToMSBuild().Split(' ') .Should() .Contain(expectedArguments); } + + [Theory] + [InlineData(new string[] { "-p:prop1=true", "-p:prop2=false" }, new string[] { "--property:prop1=true", "--property:prop2=false" })] + [InlineData(new string[] { "-p:prop1=\".;/opt/usr\"" }, new string[] { "--property:prop1=\".%3B/opt/usr\"" })] + public void Can_pass_msbuild_properties_safely(string[] tokens, string[] forwardedTokens) { + var forwardingFunction = (CommonOptions.PropertiesOption as ForwardedOption).GetForwardingFunction(); + var result = CommonOptions.PropertiesOption.Parse(tokens); + var parsedTokens = forwardingFunction(result); + parsedTokens.Should().BeEquivalentTo(forwardedTokens); + } } } From 5d1d8321c06397cb4691f92a5f0f7d8d5925e399 Mon Sep 17 00:00:00 2001 From: Chet Husk Date: Fri, 24 Jun 2022 12:16:42 -0500 Subject: [PATCH 09/12] tests for hash-token stripping in response files --- src/Cli/dotnet/Parser.cs | 2 +- .../ParserTests/ResponseFileTests.cs | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/Cli/dotnet/Parser.cs b/src/Cli/dotnet/Parser.cs index 4c0cf639ba52..84aa1d4f9d99 100644 --- a/src/Cli/dotnet/Parser.cs +++ b/src/Cli/dotnet/Parser.cs @@ -127,7 +127,7 @@ public static bool TokenPerLine(string tokenToReplace, out IReadOnlyList } else if (hashPos == 0) { return ""; } else { - return line.Substring(hashPos).Trim(); + return line.Substring(0, hashPos).Trim(); } }) // Remove empty lines diff --git a/src/Tests/dotnet.Tests/ParserTests/ResponseFileTests.cs b/src/Tests/dotnet.Tests/ParserTests/ResponseFileTests.cs index 53e7a19f7a83..42eb04f39b6c 100644 --- a/src/Tests/dotnet.Tests/ParserTests/ResponseFileTests.cs +++ b/src/Tests/dotnet.Tests/ParserTests/ResponseFileTests.cs @@ -48,5 +48,28 @@ public void Can_safely_expand_response_file_lines() { Log.WriteLine($"Token string is {tokenString}"); tokens.Skip(1).Should().BeEquivalentTo(tokenized); } + + [Fact] + public void Can_skip_empty_and_commented_lines() { + var tempFileDir = _testAssetsManager.CreateTestDirectory().Path; + var tempFilePath = Path.Combine(tempFileDir, "skips.rsp"); + var lines = new[] { + "build", + "", + "run# but skip this", + "# and this" + }; + File.WriteAllLines(tempFilePath, lines); + + var parser = Parser.Instance; + var parseResult = parser.Parse(new []{ "dotnet", $"@{tempFilePath}" }); + var tokens = parseResult.Tokens.Select(t => t.Value); + var tokenized = new [] { + "build", + "run" + }; + + tokens.Skip(1).Should().BeEquivalentTo(tokenized); + } } } From 0c8e4ac097890cb31f0b782c8ff7bba39b159972 Mon Sep 17 00:00:00 2001 From: Chet Husk Date: Fri, 24 Jun 2022 16:04:17 -0500 Subject: [PATCH 10/12] fix tests that have new expectations now that -- is the canonical prefix for property --- src/Cli/dotnet/commands/RestoringCommand.cs | 2 +- src/Tests/dotnet.Tests/ParserTests/RestoreParserTests.cs | 4 ++-- .../dotnet-msbuild/GivenDotnetBuildInvocation.cs | 6 +++--- .../dotnet-msbuild/GivenDotnetPublishInvocation.cs | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Cli/dotnet/commands/RestoringCommand.cs b/src/Cli/dotnet/commands/RestoringCommand.cs index b668dfd82bc8..8412ef08593e 100644 --- a/src/Cli/dotnet/commands/RestoringCommand.cs +++ b/src/Cli/dotnet/commands/RestoringCommand.cs @@ -75,7 +75,7 @@ private static IEnumerable Prepend(string argument, IEnumerable private static bool HasArgumentToExcludeFromRestore(IEnumerable arguments) => arguments.Any(a => IsExcludedFromRestore(a)); - private static readonly string[] propertyPrefixes = new string[]{ "-", "/" }; + private static readonly string[] propertyPrefixes = new string[]{ "-", "/", "--" }; private static bool IsExcludedFromRestore(string argument) => propertyPrefixes.Any(prefix => argument.StartsWith($"{prefix}property:TargetFramework=", StringComparison.Ordinal)) || diff --git a/src/Tests/dotnet.Tests/ParserTests/RestoreParserTests.cs b/src/Tests/dotnet.Tests/ParserTests/RestoreParserTests.cs index 11ff2d90913a..8ebc57d43d15 100644 --- a/src/Tests/dotnet.Tests/ParserTests/RestoreParserTests.cs +++ b/src/Tests/dotnet.Tests/ParserTests/RestoreParserTests.cs @@ -28,7 +28,7 @@ public void RestoreCapturesArgumentsToForwardToMSBuildWhenTargetIsSpecified() var result = Parser.Instance.Parse(@"dotnet restore .\some.csproj --packages c:\.nuget\packages /p:SkipInvalidConfigurations=true"); result.GetValueForArgument>(RestoreCommandParser.SlnOrProjectArgument).Should().BeEquivalentTo(@".\some.csproj"); - result.OptionValuesToBeForwarded(RestoreCommandParser.GetCommand()).Should().Contain(@"-property:SkipInvalidConfigurations=true"); + result.OptionValuesToBeForwarded(RestoreCommandParser.GetCommand()).Should().Contain(@"--property:SkipInvalidConfigurations=true"); } [Fact] @@ -36,7 +36,7 @@ public void RestoreCapturesArgumentsToForwardToMSBuildWhenTargetIsNotSpecified() { var result = Parser.Instance.Parse(@"dotnet restore --packages c:\.nuget\packages /p:SkipInvalidConfigurations=true"); - result.OptionValuesToBeForwarded(RestoreCommandParser.GetCommand()).Should().Contain(@"-property:SkipInvalidConfigurations=true"); + result.OptionValuesToBeForwarded(RestoreCommandParser.GetCommand()).Should().Contain(@"--property:SkipInvalidConfigurations=true"); } [Fact] diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs index bc0e34bb44db..a8faa7332a2d 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs @@ -19,7 +19,7 @@ public class GivenDotnetBuildInvocation : IClassFixturefoo")] - [InlineData(new string[] { "-property:Verbosity=diag" }, "-property:Verbosity=diag")] + [InlineData(new string[] { "-property:Verbosity=diag" }, "--property:Verbosity=diag")] [InlineData(new string[] { "--output", "foo" }, "-property:OutputPath=foo")] [InlineData(new string[] { "-o", "foo1 foo2" }, "\"-property:OutputPath=foo1 foo2\"")] [InlineData(new string[] { "--no-incremental" }, "-target:Rebuild")] @@ -56,8 +56,8 @@ public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalA [Theory] [InlineData(new string[] { "-f", "tfm" }, "-target:Restore", "-property:TargetFramework=tfm")] - [InlineData(new string[] { "-p:TargetFramework=tfm" }, "-target:Restore", "-property:TargetFramework=tfm")] - [InlineData(new string[] { "/p:TargetFramework=tfm" }, "-target:Restore", "-property:TargetFramework=tfm")] + [InlineData(new string[] { "-p:TargetFramework=tfm" }, "-target:Restore", "--property:TargetFramework=tfm")] + [InlineData(new string[] { "/p:TargetFramework=tfm" }, "-target:Restore", "--property:TargetFramework=tfm")] [InlineData(new string[] { "-t:Run", "-f", "tfm" }, "-target:Restore", "-property:TargetFramework=tfm -t:Run")] [InlineData(new string[] { "/t:Run", "-f", "tfm" }, "-target:Restore", "-property:TargetFramework=tfm /t:Run")] [InlineData(new string[] { "-o", "myoutput", "-f", "tfm", "-v", "diag", "/ArbitrarySwitchForMSBuild" }, diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs index c63e577bcbd8..a7a4fd6be55f 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs @@ -104,7 +104,7 @@ public void CommandAcceptsMultipleCustomProperties() command.GetArgumentsToMSBuild() .Should() - .Be($"{ExpectedPrefix} -restore -target:Publish -property:Prop1=prop1 -property:Prop2=prop2"); + .Be($"{ExpectedPrefix} -restore -target:Publish --property:Prop1=prop1 --property:Prop2=prop2"); } } } From ee0b575d93a5d8e4d058a0a6d4d5935bc3be888a Mon Sep 17 00:00:00 2001 From: Chet Husk Date: Tue, 28 Jun 2022 12:55:19 -0500 Subject: [PATCH 11/12] expand tests for space-containing options --- .../MSBuildArgumentCommandLineParserTests.cs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs b/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs index deb5d9313da8..e6d236ef3720 100644 --- a/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs +++ b/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs @@ -25,22 +25,28 @@ public MSBuildArgumentCommandLineParserTests(ITestOutputHelper output) [Theory] [InlineData(new string[] { "-property:prop1=true", "-p:prop2=false" }, true)] [InlineData(new string[] { "-property:prop1=true", "-p:prop2=false" }, false)] + [InlineData(new string[] { "-p:teamcity_buildConfName=\"Build, Test and Publish\"" }, false)] + [InlineData(new string[] { "-p:teamcity_buildConfName=\"Build, Test and Publish\"" }, true)] [InlineData(new string[] { "-detailedSummary" }, true)] [InlineData(new string[] { "-clp:NoSummary" }, true)] [InlineData(new string[] { "-orc" }, true)] [InlineData(new string[] { "-orc" }, false)] public void MSBuildArgumentsAreForwardedCorrectly(string[] arguments, bool buildCommand) { - RestoringCommand command = buildCommand ? - BuildCommand.FromArgs(arguments) : + RestoringCommand command = buildCommand ? + BuildCommand.FromArgs(arguments) : PublishCommand.FromArgs(arguments); var expectedArguments = arguments.Select(a => a.Replace("-property:", "--property:").Replace("-p:", "--property:")); - command.GetArgumentsToMSBuild().Split(' ') - .Should() - .Contain(expectedArguments); + var argString = command.GetArgumentsToMSBuild(); + + foreach (var expectedArg in expectedArguments) + { + argString.Should().Contain(expectedArg); + } } [Theory] + [InlineData(new string[] { "-p:teamcity_buildConfName=\"Build, Test and Publish\"" }, new string[] { "--property:teamcity_buildConfName=\"Build, Test and Publish\"" })] [InlineData(new string[] { "-p:prop1=true", "-p:prop2=false" }, new string[] { "--property:prop1=true", "--property:prop2=false" })] [InlineData(new string[] { "-p:prop1=\".;/opt/usr\"" }, new string[] { "--property:prop1=\".%3B/opt/usr\"" })] public void Can_pass_msbuild_properties_safely(string[] tokens, string[] forwardedTokens) { From 133f25e21efb96d36216995b352b316bb5e0725f Mon Sep 17 00:00:00 2001 From: Chet Husk Date: Tue, 28 Jun 2022 15:28:01 -0500 Subject: [PATCH 12/12] fix test baseline to not used the out-of-proc escaped versions for comparison --- .../commands/dotnet-complete/ParseCommand.cs | 15 +++++++++------ .../MSBuildArgumentCommandLineParserTests.cs | 2 +- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/Cli/dotnet/commands/dotnet-complete/ParseCommand.cs b/src/Cli/dotnet/commands/dotnet-complete/ParseCommand.cs index cbce2e4d7abc..f4e44b5512f0 100644 --- a/src/Cli/dotnet/commands/dotnet-complete/ParseCommand.cs +++ b/src/Cli/dotnet/commands/dotnet-complete/ParseCommand.cs @@ -11,26 +11,29 @@ public static int Run(ParseResult result) { result.HandleDebugSwitch(); - Console.WriteLine(result.Diagram()); + var tokens = result.Tokens.Skip(1).Select(t => t.Value).ToArray(); + var reparsed = Microsoft.DotNet.Cli.Parser.Instance.Parse(tokens); + Console.WriteLine(reparsed.Diagram()); - if (result.UnparsedTokens.Any()) + + if (reparsed.UnparsedTokens.Any()) { Console.WriteLine("Unparsed Tokens: "); - Console.WriteLine(string.Join(" ", result.UnparsedTokens)); + Console.WriteLine(string.Join(" ", reparsed.UnparsedTokens)); } - var optionValuesToBeForwarded = result.OptionValuesToBeForwarded(ParseCommandParser.GetCommand()); + var optionValuesToBeForwarded = reparsed.OptionValuesToBeForwarded(ParseCommandParser.GetCommand()); if (optionValuesToBeForwarded.Any()) { Console.WriteLine("Option values to be forwarded: "); Console.WriteLine(string.Join(" ", optionValuesToBeForwarded)); } - if (result.Errors.Any()) + if (reparsed.Errors.Any()) { Console.WriteLine(); Console.WriteLine("ERRORS"); Console.WriteLine(); - foreach (var error in result.Errors) + foreach (var error in reparsed.Errors) { Console.WriteLine(error?.Message); } diff --git a/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs b/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs index e6d236ef3720..2ba24972fa82 100644 --- a/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs +++ b/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs @@ -37,7 +37,7 @@ public void MSBuildArgumentsAreForwardedCorrectly(string[] arguments, bool build BuildCommand.FromArgs(arguments) : PublishCommand.FromArgs(arguments); var expectedArguments = arguments.Select(a => a.Replace("-property:", "--property:").Replace("-p:", "--property:")); - var argString = command.GetArgumentsToMSBuild(); + var argString = command.MSBuildArguments; foreach (var expectedArg in expectedArguments) {