From 95c2b0907239e0c7ebeee59560b6e12aaa4dfeae Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Mon, 29 Mar 2021 14:52:28 +0200 Subject: [PATCH 01/22] Eliminate build process hop by loading MSBuild.dll into CLI process --- .../MSBuildForwardingAppWithoutLogging.cs | 76 +++++++++++++++++-- src/Cli/dotnet/PerformanceLogEventSource.cs | 4 +- .../dotnet-msbuild/MSBuildForwardingApp.cs | 29 +++++-- 3 files changed, 93 insertions(+), 16 deletions(-) diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs index 8e2c546ff50c..06f840879c8f 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs @@ -6,6 +6,7 @@ using System.Diagnostics; using System.IO; using System.Linq; +using System.Reflection; using System.Runtime.InteropServices; using System.Text; @@ -13,12 +14,21 @@ namespace Microsoft.DotNet.Cli.Utils { internal class MSBuildForwardingAppWithoutLogging { + internal static readonly bool executeMSBuildOutOfProc = Env.GetEnvironmentVariableAsBool("DOTNET_CLI_EXEC_MSBUILD"); + + private const string MSBuildAppClassName = "Microsoft.Build.CommandLine.MSBuildApp"; + private const string MSBuildExeName = "MSBuild.dll"; private const string SdksDirectoryName = "Sdks"; + // Null if we're running MSBuild in-proc. private readonly ForwardingAppImplementation _forwardingApp; + private IEnumerable _argsToForward; + + private string _msbuildPath; + internal static string MSBuildExtensionsPathTestHook = null; private readonly Dictionary _msbuildRequiredEnvironmentVariables = @@ -34,26 +44,79 @@ internal class MSBuildForwardingAppWithoutLogging public MSBuildForwardingAppWithoutLogging(IEnumerable argsToForward, string msbuildPath = null) { - _forwardingApp = new ForwardingAppImplementation( - msbuildPath ?? GetMSBuildExePath(), - _msbuildRequiredParameters.Concat(argsToForward.Select(Escape)), - environmentVariables: _msbuildRequiredEnvironmentVariables); + _argsToForward = argsToForward; + _msbuildPath = msbuildPath ?? GetMSBuildExePath(); + + if (executeMSBuildOutOfProc) + { + _forwardingApp = new ForwardingAppImplementation( + _msbuildPath, + _msbuildRequiredParameters.Concat(argsToForward.Select(Escape)), + environmentVariables: _msbuildRequiredEnvironmentVariables); + } } public virtual ProcessStartInfo GetProcessStartInfo() { + Debug.Assert(_forwardingApp != null, "Can't get ProcessStartInfo when not executing out-of-proc"); return _forwardingApp .GetProcessStartInfo(); } + public string[] GetAllArgumentsUnescaped() + { + return _msbuildRequiredParameters.Concat(_argsToForward).ToArray(); + } + public void EnvironmentVariable(string name, string value) { - _forwardingApp.WithEnvironmentVariable(name, value); + if (_forwardingApp != null) + { + _forwardingApp.WithEnvironmentVariable(name, value); + } + else + { + _msbuildRequiredEnvironmentVariables.Add(name, value); + } } public int Execute() { - return GetProcessStartInfo().Execute(); + if (executeMSBuildOutOfProc) + { + return GetProcessStartInfo().Execute(); + } + else + { + return ExecuteInProc(GetAllArgumentsUnescaped()); + } + } + + public int ExecuteInProc(string[] arguments) + { + // Save current environment variables before overwriting them. + Dictionary savedEnvironmentVariables = new Dictionary(); + try + { + foreach (KeyValuePair kvp in _msbuildRequiredEnvironmentVariables) + { + savedEnvironmentVariables[kvp.Key] = Environment.GetEnvironmentVariable(kvp.Key); + Environment.SetEnvironmentVariable(kvp.Key, kvp.Value); + } + + Assembly assembly = Assembly.LoadFrom(_msbuildPath); + Type type = assembly.GetType(MSBuildAppClassName); + MethodInfo mi = type.GetMethod("Main"); + return (int)mi.Invoke(null, new object[] { arguments }); + } + finally + { + // Restore saved environment variables. + foreach (KeyValuePair kvp in savedEnvironmentVariables) + { + Environment.SetEnvironmentVariable(kvp.Key, kvp.Value); + } + } } private static string Escape(string arg) => @@ -98,4 +161,3 @@ private static bool IsRestoreSources(string arg) } } } - diff --git a/src/Cli/dotnet/PerformanceLogEventSource.cs b/src/Cli/dotnet/PerformanceLogEventSource.cs index beb4d2388968..1737bc52d702 100644 --- a/src/Cli/dotnet/PerformanceLogEventSource.cs +++ b/src/Cli/dotnet/PerformanceLogEventSource.cs @@ -264,11 +264,11 @@ internal void MemoryConfiguration(int memoryLoad, int availablePhysicalMB, int t } [NonEvent] - internal void LogMSBuildStart(ProcessStartInfo startInfo) + internal void LogMSBuildStart(string fileName, string arguments) { if (IsEnabled()) { - MSBuildStart($"{startInfo.FileName} {startInfo.Arguments}"); + MSBuildStart($"{fileName} {arguments}"); } } diff --git a/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs b/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs index 6f1f1ecfca95..c6d8d32beb95 100644 --- a/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs +++ b/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs @@ -71,15 +71,30 @@ public ProcessStartInfo GetProcessStartInfo() public virtual int Execute() { - // Ignore Ctrl-C for the remainder of the command's execution - // Forwarding commands will just spawn the child process and exit - Console.CancelKeyPress += (sender, e) => { e.Cancel = true; }; + int exitCode; - ProcessStartInfo startInfo = GetProcessStartInfo(); + if (MSBuildForwardingAppWithoutLogging.executeMSBuildOutOfProc) + { + // Ignore Ctrl-C for the remainder of the command's execution + // Forwarding commands will just spawn the child process and exit + Console.CancelKeyPress += (sender, e) => { e.Cancel = true; }; + + ProcessStartInfo startInfo = GetProcessStartInfo(); - PerformanceLogEventSource.Log.LogMSBuildStart(startInfo); - int exitCode = startInfo.Execute(); - PerformanceLogEventSource.Log.MSBuildStop(exitCode); + PerformanceLogEventSource.Log.LogMSBuildStart(startInfo.FileName, startInfo.Arguments); + exitCode = startInfo.Execute(); + PerformanceLogEventSource.Log.MSBuildStop(exitCode); + } + else + { + string[] arguments = _forwardingAppWithoutLogging.GetAllArgumentsUnescaped(); + if (PerformanceLogEventSource.Log.IsEnabled()) + { + PerformanceLogEventSource.Log.LogMSBuildStart(string.Empty, ArgumentEscaper.EscapeAndConcatenateArgArrayForProcessStart(arguments)); + } + exitCode = _forwardingAppWithoutLogging.ExecuteInProc(arguments); + PerformanceLogEventSource.Log.MSBuildStop(exitCode); + } return exitCode; } From 1ae669ccf6731c2b75cf5b7a4a1cb61faa554704 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Tue, 30 Mar 2021 14:05:34 +0200 Subject: [PATCH 02/22] Handle TargetInvocationException --- .../MSBuildForwardingAppWithoutLogging.cs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs index 06f840879c8f..d7cb7bf4067f 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs @@ -107,7 +107,18 @@ public int ExecuteInProc(string[] arguments) Assembly assembly = Assembly.LoadFrom(_msbuildPath); Type type = assembly.GetType(MSBuildAppClassName); MethodInfo mi = type.GetMethod("Main"); - return (int)mi.Invoke(null, new object[] { arguments }); + + try + { + return (int)mi.Invoke(null, new object[] { arguments }); + } + catch (TargetInvocationException targetException) + { + Console.Error.Write("Unhandled exception: "); + Console.Error.WriteLine(targetException.InnerException.ToString()); + + return unchecked((int)0xe0434352); // EXCEPTION_COMPLUS + } } finally { From 90bfa7c537e679784f30ff6e0e99ebb3b7f0e2b0 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Tue, 30 Mar 2021 14:06:04 +0200 Subject: [PATCH 03/22] Add Microsoft.Build.Runtime package reference --- .../Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj b/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj index a358de76f0ee..bbb696e5e5a0 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj @@ -15,6 +15,7 @@ + From 5c8e93223b7e7bd5dae9dc4b57ead79272865bd8 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Tue, 30 Mar 2021 14:07:09 +0200 Subject: [PATCH 04/22] Capture stdout/stderr of in-proc MSBuild invocation --- .../ProjectToolsCommandResolver.cs | 36 +++++++++++++++++-- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/src/Cli/dotnet/CommandFactory/CommandResolution/ProjectToolsCommandResolver.cs b/src/Cli/dotnet/CommandFactory/CommandResolution/ProjectToolsCommandResolver.cs index d365e4f33b99..c5421e0340c4 100644 --- a/src/Cli/dotnet/CommandFactory/CommandResolution/ProjectToolsCommandResolver.cs +++ b/src/Cli/dotnet/CommandFactory/CommandResolution/ProjectToolsCommandResolver.cs @@ -399,9 +399,39 @@ internal void GenerateDepsJsonFile( Reporter.Verbose.WriteLine(string.Format(LocalizableStrings.MSBuildArgs, ArgumentEscaper.EscapeAndConcatenateArgArrayForProcessStart(args))); - var result = new MSBuildForwardingAppWithoutLogging(args, msBuildExePath) - .GetProcessStartInfo() - .ExecuteAndCaptureOutput(out string stdOut, out string stdErr); + int result; + string stdOut; + string stdErr; + + var forwardingAppWithoutLogging = new MSBuildForwardingAppWithoutLogging(args, msBuildExePath); + if (MSBuildForwardingAppWithoutLogging.executeMSBuildOutOfProc) + { + result = forwardingAppWithoutLogging + .GetProcessStartInfo() + .ExecuteAndCaptureOutput(out stdOut, out stdErr); + } + else + { + var outWriter = new StringWriter(); + var errWriter = new StringWriter(); + var savedOutWriter = Console.Out; + var savedErrWriter = Console.Error; + try + { + Console.SetOut(outWriter); + Console.SetError(errWriter); + + result = forwardingAppWithoutLogging.Execute(); + } + finally + { + stdOut = outWriter.ToString(); + stdErr = errWriter.ToString(); + + Console.SetOut(savedOutWriter); + Console.SetError(savedErrWriter); + } + } if (result != 0) { From 84e5e329d4891a59d2473f58bb5a6cac85119135 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Tue, 30 Mar 2021 14:07:46 +0200 Subject: [PATCH 05/22] First batch of test fixes --- .../MSBuildForwardingAppWithoutLogging.cs | 2 +- .../dotnet-msbuild/MSBuildForwardingApp.cs | 6 ++++ .../MSBuildArgumentCommandLineParserTests.cs | 2 +- .../DotnetMsbuildInProcTests.cs | 16 ++-------- .../GivenDotnetBuildInvocation.cs | 3 +- .../GivenDotnetCleanInvocation.cs | 2 +- .../GivenDotnetPackInvocation.cs | 2 +- .../GivenDotnetPublishInvocation.cs | 3 +- .../GivenDotnetRestoreInvocation.cs | 2 +- .../GivenDotnetStoreInvocation.cs | 6 ++-- .../GivenMsbuildForwardingApp.cs | 29 +++++++++++++++++++ 11 files changed, 48 insertions(+), 25 deletions(-) diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs index d7cb7bf4067f..2008ea3187aa 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs @@ -14,7 +14,7 @@ namespace Microsoft.DotNet.Cli.Utils { internal class MSBuildForwardingAppWithoutLogging { - internal static readonly bool executeMSBuildOutOfProc = Env.GetEnvironmentVariableAsBool("DOTNET_CLI_EXEC_MSBUILD"); + internal static bool executeMSBuildOutOfProc = Env.GetEnvironmentVariableAsBool("DOTNET_CLI_EXEC_MSBUILD"); private const string MSBuildAppClassName = "Microsoft.Build.CommandLine.MSBuildApp"; diff --git a/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs b/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs index c6d8d32beb95..051449eaabf7 100644 --- a/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs +++ b/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs @@ -69,6 +69,12 @@ public ProcessStartInfo GetProcessStartInfo() return _forwardingAppWithoutLogging.GetProcessStartInfo(); } + public string GetConcatenatedArguments() + { + var argumentsUnescaped = _forwardingAppWithoutLogging.GetAllArgumentsUnescaped(); + return Cli.Utils.ArgumentEscaper.EscapeAndConcatenateArgArrayForProcessStart(argumentsUnescaped); + } + public virtual int Execute() { int exitCode; diff --git a/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs b/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs index 2bafc6654218..4bc2385603ad 100644 --- a/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs +++ b/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs @@ -31,7 +31,7 @@ public void MSBuildArgumentsAreForwardedCorrectly(string[] arguments, bool build RestoringCommand command = buildCommand ? BuildCommand.FromArgs(arguments) : PublishCommand.FromArgs(arguments); - command.GetProcessStartInfo().Arguments.Split(' ') + command.GetConcatenatedArguments().Split(' ') .Should() .Contain(arguments); } diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/DotnetMsbuildInProcTests.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/DotnetMsbuildInProcTests.cs index 3c659ee21a87..9878dd3ce0bb 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/DotnetMsbuildInProcTests.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/DotnetMsbuildInProcTests.cs @@ -60,22 +60,12 @@ private string[] GetArgsForMSBuild(Func sentinelExists, out Telemetry.Tele MSBuildForwardingApp msBuildForwardingApp = new MSBuildForwardingApp(Enumerable.Empty()); - object forwardingAppWithoutLogging = msBuildForwardingApp + var forwardingAppWithoutLogging = msBuildForwardingApp .GetType() .GetField("_forwardingAppWithoutLogging", BindingFlags.Instance | BindingFlags.NonPublic) - ?.GetValue(msBuildForwardingApp); + ?.GetValue(msBuildForwardingApp) as Cli.Utils.MSBuildForwardingAppWithoutLogging; - FieldInfo forwardingAppFieldInfo = forwardingAppWithoutLogging - .GetType() - .GetField("_forwardingApp", BindingFlags.Instance | BindingFlags.NonPublic); - - object forwardingApp = forwardingAppFieldInfo?.GetValue(forwardingAppWithoutLogging); - - FieldInfo allArgsFieldinfo = forwardingApp? - .GetType() - .GetField("_allArgs", BindingFlags.Instance | BindingFlags.NonPublic); - - return allArgsFieldinfo?.GetValue(forwardingApp) as string[]; + return forwardingAppWithoutLogging?.GetAllArgumentsUnescaped(); } } public sealed class MockFirstTimeUseNoticeSentinel : IFirstTimeUseNoticeSentinel diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs index 4d850ddef9af..26c4b516979c 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs @@ -46,8 +46,7 @@ public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalA command.SeparateRestoreCommand.Should().BeNull(); - command.GetProcessStartInfo() - .Arguments.Should() + command.GetConcatenatedArguments().Should() .Be($"{ExpectedPrefix} -restore -consoleloggerparameters:Summary{expectedAdditionalArgs}"); }); } diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetCleanInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetCleanInvocation.cs index d826bc5b328a..38a54c5af169 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetCleanInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetCleanInvocation.cs @@ -45,7 +45,7 @@ public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalA var msbuildPath = ""; CleanCommand.FromArgs(args, msbuildPath) - .GetProcessStartInfo().Arguments.Should().Be($"{ExpectedPrefix}{expectedAdditionalArgs}"); + .GetConcatenatedArguments().Should().Be($"{ExpectedPrefix}{expectedAdditionalArgs}"); }); } } diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPackInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPackInvocation.cs index 01da7c8e7206..1c695cb0154d 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPackInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPackInvocation.cs @@ -48,7 +48,7 @@ public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalA var expectedPrefix = args.FirstOrDefault() == "--no-build" ? ExpectedNoBuildPrefix : ExpectedPrefix; command.SeparateRestoreCommand.Should().BeNull(); - command.GetProcessStartInfo().Arguments.Should().Be($"{expectedPrefix}{expectedAdditionalArgs}"); + command.GetConcatenatedArguments().Should().Be($"{expectedPrefix}{expectedAdditionalArgs}"); }); } } diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs index 8cfb2d49e89b..a9f7b4e0fadd 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs @@ -101,8 +101,7 @@ public void CommandAcceptsMultipleCustomProperties() var msbuildPath = ""; var command = PublishCommand.FromArgs(new[] { "/p:Prop1=prop1", "/p:Prop2=prop2" }, msbuildPath); - command.GetProcessStartInfo() - .Arguments + command.GetConcatenatedArguments() .Should() .Be($"{ExpectedPrefix} -restore -target:Publish -property:Prop1=prop1 -property:Prop2=prop2"); } diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetRestoreInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetRestoreInvocation.cs index 839266df1ec8..b67b3fea5ff7 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetRestoreInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetRestoreInvocation.cs @@ -49,7 +49,7 @@ public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalA var msbuildPath = ""; RestoreCommand.FromArgs(args, msbuildPath) - .GetProcessStartInfo().Arguments + .GetConcatenatedArguments() .Should().Be($"{ExpectedPrefix}{expectedAdditionalArgs}"); }); } diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetStoreInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetStoreInvocation.cs index 3d0af3f96baa..ca780d414d65 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetStoreInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetStoreInvocation.cs @@ -25,7 +25,7 @@ public void ItAddsProjectToMsbuildInvocation(string optionName) var msbuildPath = ""; string[] args = new string[] { optionName, "" }; StoreCommand.FromArgs(args, msbuildPath) - .GetProcessStartInfo().Arguments.Should().Be($"{ExpectedPrefix}"); + .GetConcatenatedArguments().Should().Be($"{ExpectedPrefix}"); } [Theory] @@ -46,7 +46,7 @@ public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalA var msbuildPath = ""; StoreCommand.FromArgs(args, msbuildPath) - .GetProcessStartInfo().Arguments.Should().Be($"{ExpectedPrefix}{expectedAdditionalArgs}"); + .GetConcatenatedArguments().Should().Be($"{ExpectedPrefix}{expectedAdditionalArgs}"); }); } @@ -60,7 +60,7 @@ public void ItAddsOutputPathToMsBuildInvocation(string optionName) var msbuildPath = ""; StoreCommand.FromArgs(args, msbuildPath) - .GetProcessStartInfo().Arguments.Should().Be($"{ExpectedPrefix} -property:ComposeDir={Path.GetFullPath(path)}"); + .GetConcatenatedArguments().Should().Be($"{ExpectedPrefix} -property:ComposeDir={Path.GetFullPath(path)}"); } } } diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenMsbuildForwardingApp.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenMsbuildForwardingApp.cs index c320b4f9c123..e9cbfe8af43d 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenMsbuildForwardingApp.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenMsbuildForwardingApp.cs @@ -14,6 +14,21 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests { public class GivenMsbuildForwardingApp : SdkTest { + private struct OutOfProcMSBuildHolder : IDisposable + { + private bool _originalExecuteMSBuildOutOfProc; + public OutOfProcMSBuildHolder(bool executeMSBuildOutOfProc) + { + _originalExecuteMSBuildOutOfProc = Cli.Utils.MSBuildForwardingAppWithoutLogging.executeMSBuildOutOfProc; + Cli.Utils.MSBuildForwardingAppWithoutLogging.executeMSBuildOutOfProc = executeMSBuildOutOfProc; + + } + public void Dispose() + { + Cli.Utils.MSBuildForwardingAppWithoutLogging.executeMSBuildOutOfProc = _originalExecuteMSBuildOutOfProc; + } + } + public GivenMsbuildForwardingApp(ITestOutputHelper log) : base(log) { } @@ -21,6 +36,8 @@ public GivenMsbuildForwardingApp(ITestOutputHelper log) : base(log) [WindowsOnlyFact] public void DotnetExeIsExecuted() { + using var holder = new OutOfProcMSBuildHolder(true); + var msbuildPath = ""; new MSBuildForwardingApp(new string[0], msbuildPath) .GetProcessStartInfo().FileName.Should().Be("dotnet.exe"); @@ -29,6 +46,8 @@ public void DotnetExeIsExecuted() [UnixOnlyFact] public void DotnetIsExecuted() { + using var holder = new OutOfProcMSBuildHolder(true); + var msbuildPath = ""; new MSBuildForwardingApp(new string[0], msbuildPath) .GetProcessStartInfo().FileName.Should().Be("dotnet"); @@ -40,6 +59,8 @@ public void DotnetIsExecuted() [InlineData("DOTNET_CLI_TELEMETRY_SESSIONID")] public void ItSetsEnvironmentalVariables(string envVarName) { + using var holder = new OutOfProcMSBuildHolder(true); + var msbuildPath = ""; var startInfo = new MSBuildForwardingApp(new string[0], msbuildPath).GetProcessStartInfo(); startInfo.Environment.ContainsKey(envVarName).Should().BeTrue(); @@ -48,6 +69,8 @@ public void ItSetsEnvironmentalVariables(string envVarName) [Fact] public void ItSetsMSBuildExtensionPathToExistingPath() { + using var holder = new OutOfProcMSBuildHolder(true); + var msbuildPath = ""; var envVar = "MSBuildExtensionsPath"; new DirectoryInfo(new MSBuildForwardingApp(new string[0], msbuildPath) @@ -60,6 +83,8 @@ public void ItSetsMSBuildExtensionPathToExistingPath() [Fact(Skip ="Test app base folder doesn't have Sdks")] public void ItSetsMSBuildSDKsPathToExistingPath() { + using var holder = new OutOfProcMSBuildHolder(true); + var msbuildPath = ""; var envVar = "MSBuildSDKsPath"; new DirectoryInfo(new MSBuildForwardingApp(new string[0], msbuildPath) @@ -72,6 +97,8 @@ public void ItSetsMSBuildSDKsPathToExistingPath() [Fact] public void ItSetsOrIgnoresTelemetrySessionId() { + using var holder = new OutOfProcMSBuildHolder(true); + var msbuildPath = ""; var envVar = "DOTNET_CLI_TELEMETRY_SESSIONID"; var startInfo = new MSBuildForwardingApp(new string[0], msbuildPath) @@ -93,6 +120,8 @@ public void ItSetsOrIgnoresTelemetrySessionId() [Fact] public void ItDoesNotSetCurrentWorkingDirectory() { + using var holder = new OutOfProcMSBuildHolder(true); + var msbuildPath = ""; var startInfo = new MSBuildForwardingApp(new string[0], msbuildPath) .GetProcessStartInfo().WorkingDirectory.Should().Be(""); From 549656421038ded404834545a6f5222e60423850 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Tue, 30 Mar 2021 17:05:24 +0200 Subject: [PATCH 06/22] Add some argument escaping even when calling MSBuild in-proc --- .../MSBuildForwardingAppWithoutLogging.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs index 2008ea3187aa..2c13c4042315 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs @@ -65,7 +65,7 @@ public virtual ProcessStartInfo GetProcessStartInfo() public string[] GetAllArgumentsUnescaped() { - return _msbuildRequiredParameters.Concat(_argsToForward).ToArray(); + return _msbuildRequiredParameters.Concat(_argsToForward.Select(Escape)).ToArray(); } public void EnvironmentVariable(string name, string value) From a510842c93496a7ce8a68d12fa227f2350c00dc6 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Tue, 30 Mar 2021 17:06:26 +0200 Subject: [PATCH 07/22] Do not call MSBuild in-proc if an empty env variable is passed, and more test fixes --- .../MSBuildForwardingAppWithoutLogging.cs | 26 ++++++++--- .../ProjectToolsCommandResolver.cs | 2 +- .../dotnet-msbuild/MSBuildForwardingApp.cs | 7 +-- .../GivenDotnetBuildInvocation.cs | 2 +- .../GivenDotnetCleanInvocation.cs | 4 +- .../GivenDotnetPackInvocation.cs | 4 +- .../GivenDotnetPublishInvocation.cs | 2 +- .../GivenDotnetRestoreInvocation.cs | 2 +- .../GivenDotnetStoreInvocation.cs | 2 +- .../GivenMsbuildForwardingApp.cs | 43 +++---------------- 10 files changed, 39 insertions(+), 55 deletions(-) diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs index 2c13c4042315..121739b3b96e 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs @@ -14,7 +14,7 @@ namespace Microsoft.DotNet.Cli.Utils { internal class MSBuildForwardingAppWithoutLogging { - internal static bool executeMSBuildOutOfProc = Env.GetEnvironmentVariableAsBool("DOTNET_CLI_EXEC_MSBUILD"); + private static readonly bool ExecuteMSBuildOutOfProcByDefault = Env.GetEnvironmentVariableAsBool("DOTNET_CLI_EXEC_MSBUILD"); private const string MSBuildAppClassName = "Microsoft.Build.CommandLine.MSBuildApp"; @@ -23,7 +23,7 @@ internal class MSBuildForwardingAppWithoutLogging private const string SdksDirectoryName = "Sdks"; // Null if we're running MSBuild in-proc. - private readonly ForwardingAppImplementation _forwardingApp; + private ForwardingAppImplementation _forwardingApp; private IEnumerable _argsToForward; @@ -31,6 +31,8 @@ internal class MSBuildForwardingAppWithoutLogging internal static string MSBuildExtensionsPathTestHook = null; + internal bool ExecuteMSBuildOutOfProc => _forwardingApp != null; + private readonly Dictionary _msbuildRequiredEnvironmentVariables = new Dictionary { @@ -42,12 +44,12 @@ internal class MSBuildForwardingAppWithoutLogging private readonly IEnumerable _msbuildRequiredParameters = new List { "-maxcpucount", "-verbosity:m" }; - public MSBuildForwardingAppWithoutLogging(IEnumerable argsToForward, string msbuildPath = null) + public MSBuildForwardingAppWithoutLogging(IEnumerable argsToForward, string msbuildPath = null, bool? executeOutOfProc = null) { _argsToForward = argsToForward; _msbuildPath = msbuildPath ?? GetMSBuildExePath(); - if (executeMSBuildOutOfProc) + if (executeOutOfProc == true || (executeOutOfProc == null && ExecuteMSBuildOutOfProcByDefault)) { _forwardingApp = new ForwardingAppImplementation( _msbuildPath, @@ -59,8 +61,7 @@ public MSBuildForwardingAppWithoutLogging(IEnumerable argsToForward, str public virtual ProcessStartInfo GetProcessStartInfo() { Debug.Assert(_forwardingApp != null, "Can't get ProcessStartInfo when not executing out-of-proc"); - return _forwardingApp - .GetProcessStartInfo(); + return _forwardingApp.GetProcessStartInfo(); } public string[] GetAllArgumentsUnescaped() @@ -78,11 +79,22 @@ public void EnvironmentVariable(string name, string value) { _msbuildRequiredEnvironmentVariables.Add(name, value); } + + if (value == string.Empty || value == "\0") + { + // Unlike ProcessStartInfo.EnvironmentVariables, Environment.SetEnvironmentVariable can't set a variable + // to an empty value, so we just fall back to calling MSBuild out-of-proc if we encounter this. + // https://github.com/dotnet/runtime/issues/34446 + _forwardingApp = new ForwardingAppImplementation( + _msbuildPath, + _msbuildRequiredParameters.Concat(_argsToForward.Select(Escape)), + environmentVariables: _msbuildRequiredEnvironmentVariables); + } } public int Execute() { - if (executeMSBuildOutOfProc) + if (_forwardingApp != null) { return GetProcessStartInfo().Execute(); } diff --git a/src/Cli/dotnet/CommandFactory/CommandResolution/ProjectToolsCommandResolver.cs b/src/Cli/dotnet/CommandFactory/CommandResolution/ProjectToolsCommandResolver.cs index c5421e0340c4..7a8f6319ddd2 100644 --- a/src/Cli/dotnet/CommandFactory/CommandResolution/ProjectToolsCommandResolver.cs +++ b/src/Cli/dotnet/CommandFactory/CommandResolution/ProjectToolsCommandResolver.cs @@ -404,7 +404,7 @@ internal void GenerateDepsJsonFile( string stdErr; var forwardingAppWithoutLogging = new MSBuildForwardingAppWithoutLogging(args, msBuildExePath); - if (MSBuildForwardingAppWithoutLogging.executeMSBuildOutOfProc) + if (forwardingAppWithoutLogging.ExecuteMSBuildOutOfProc) { result = forwardingAppWithoutLogging .GetProcessStartInfo() diff --git a/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs b/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs index 051449eaabf7..8b13ef248130 100644 --- a/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs +++ b/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs @@ -44,11 +44,12 @@ private static IEnumerable ConcatTelemetryLogger(IEnumerable arg return argsToForward; } - public MSBuildForwardingApp(IEnumerable argsToForward, string msbuildPath = null) + public MSBuildForwardingApp(IEnumerable argsToForward, string msbuildPath = null, bool? executeOutOfProc = null) { _forwardingAppWithoutLogging = new MSBuildForwardingAppWithoutLogging( ConcatTelemetryLogger(argsToForward), - msbuildPath); + msbuildPath, + executeOutOfProc); // Add the performance log location to the environment of the target process. if (PerformanceLogManager.Instance != null && !string.IsNullOrEmpty(PerformanceLogManager.Instance.CurrentLogDirectory)) @@ -79,7 +80,7 @@ public virtual int Execute() { int exitCode; - if (MSBuildForwardingAppWithoutLogging.executeMSBuildOutOfProc) + if (_forwardingAppWithoutLogging.ExecuteMSBuildOutOfProc) { // Ignore Ctrl-C for the remainder of the command's execution // Forwarding commands will just spawn the child process and exit diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs index 26c4b516979c..6a04b7e72b57 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs @@ -9,7 +9,7 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests { public class GivenDotnetBuildInvocation : IClassFixture { - const string ExpectedPrefix = "exec -maxcpucount -verbosity:m"; + const string ExpectedPrefix = "-maxcpucount -verbosity:m"; private static readonly string WorkingDirectory = TestPathUtilities.FormatAbsolutePath(nameof(GivenDotnetBuildInvocation)); diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetCleanInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetCleanInvocation.cs index 38a54c5af169..4bef90dd408d 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetCleanInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetCleanInvocation.cs @@ -12,7 +12,7 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests { public class GivenDotnetCleanInvocation : IClassFixture { - const string ExpectedPrefix = "exec -maxcpucount -verbosity:m -verbosity:normal -target:Clean"; + const string ExpectedPrefix = "-maxcpucount -verbosity:m -verbosity:normal -target:Clean"; private static readonly string WorkingDirectory = TestPathUtilities.FormatAbsolutePath(nameof(GivenDotnetCleanInvocation)); @@ -22,7 +22,7 @@ public void ItAddsProjectToMsbuildInvocation() { var msbuildPath = ""; CleanCommand.FromArgs(new string[] { "" }, msbuildPath) - .GetProcessStartInfo().Arguments.Should().Be("exec -maxcpucount -verbosity:m -verbosity:normal -target:Clean"); + .GetConcatenatedArguments().Should().Be("-maxcpucount -verbosity:m -verbosity:normal -target:Clean"); } [Theory] diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPackInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPackInvocation.cs index 1c695cb0154d..f9e381c3fe21 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPackInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPackInvocation.cs @@ -14,8 +14,8 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests { public class GivenDotnetPackInvocation : IClassFixture { - const string ExpectedPrefix = "exec -maxcpucount -verbosity:m -restore -target:pack"; - const string ExpectedNoBuildPrefix = "exec -maxcpucount -verbosity:m -target:pack"; + const string ExpectedPrefix = "-maxcpucount -verbosity:m -restore -target:pack"; + const string ExpectedNoBuildPrefix = "-maxcpucount -verbosity:m -target:pack"; private static readonly string WorkingDirectory = TestPathUtilities.FormatAbsolutePath(nameof(GivenDotnetPackInvocation)); diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs index a9f7b4e0fadd..b1db35b00165 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs @@ -20,7 +20,7 @@ public GivenDotnetPublishInvocation(ITestOutputHelper output) this.output = output; } - const string ExpectedPrefix = "exec -maxcpucount -verbosity:m"; + const string ExpectedPrefix = "-maxcpucount -verbosity:m"; [Theory] [InlineData(new string[] { }, "")] diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetRestoreInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetRestoreInvocation.cs index b67b3fea5ff7..fe9108c9a254 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetRestoreInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetRestoreInvocation.cs @@ -13,7 +13,7 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests public class GivenDotnetRestoreInvocation : IClassFixture { private const string ExpectedPrefix = - "exec -maxcpucount -verbosity:m -nologo -target:Restore"; + "-maxcpucount -verbosity:m -nologo -target:Restore"; private static readonly string WorkingDirectory = TestPathUtilities.FormatAbsolutePath(nameof(GivenDotnetRestoreInvocation)); diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetStoreInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetStoreInvocation.cs index ca780d414d65..65551f8d557b 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetStoreInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetStoreInvocation.cs @@ -12,7 +12,7 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests { public class GivenDotnetStoreInvocation : IClassFixture { - const string ExpectedPrefix = "exec -maxcpucount -verbosity:m -target:ComposeStore "; + const string ExpectedPrefix = "-maxcpucount -verbosity:m -target:ComposeStore "; static readonly string[] ArgsPrefix = { "--manifest", "" }; private static readonly string WorkingDirectory = TestPathUtilities.FormatAbsolutePath(nameof(GivenDotnetStoreInvocation)); diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenMsbuildForwardingApp.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenMsbuildForwardingApp.cs index e9cbfe8af43d..a3689873bf95 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenMsbuildForwardingApp.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenMsbuildForwardingApp.cs @@ -14,21 +14,6 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests { public class GivenMsbuildForwardingApp : SdkTest { - private struct OutOfProcMSBuildHolder : IDisposable - { - private bool _originalExecuteMSBuildOutOfProc; - public OutOfProcMSBuildHolder(bool executeMSBuildOutOfProc) - { - _originalExecuteMSBuildOutOfProc = Cli.Utils.MSBuildForwardingAppWithoutLogging.executeMSBuildOutOfProc; - Cli.Utils.MSBuildForwardingAppWithoutLogging.executeMSBuildOutOfProc = executeMSBuildOutOfProc; - - } - public void Dispose() - { - Cli.Utils.MSBuildForwardingAppWithoutLogging.executeMSBuildOutOfProc = _originalExecuteMSBuildOutOfProc; - } - } - public GivenMsbuildForwardingApp(ITestOutputHelper log) : base(log) { } @@ -36,20 +21,16 @@ public GivenMsbuildForwardingApp(ITestOutputHelper log) : base(log) [WindowsOnlyFact] public void DotnetExeIsExecuted() { - using var holder = new OutOfProcMSBuildHolder(true); - var msbuildPath = ""; - new MSBuildForwardingApp(new string[0], msbuildPath) + new MSBuildForwardingApp(new string[0], msbuildPath, executeOutOfProc: true) .GetProcessStartInfo().FileName.Should().Be("dotnet.exe"); } [UnixOnlyFact] public void DotnetIsExecuted() { - using var holder = new OutOfProcMSBuildHolder(true); - var msbuildPath = ""; - new MSBuildForwardingApp(new string[0], msbuildPath) + new MSBuildForwardingApp(new string[0], msbuildPath, executeOutOfProc: true) .GetProcessStartInfo().FileName.Should().Be("dotnet"); } @@ -59,21 +40,17 @@ public void DotnetIsExecuted() [InlineData("DOTNET_CLI_TELEMETRY_SESSIONID")] public void ItSetsEnvironmentalVariables(string envVarName) { - using var holder = new OutOfProcMSBuildHolder(true); - var msbuildPath = ""; - var startInfo = new MSBuildForwardingApp(new string[0], msbuildPath).GetProcessStartInfo(); + var startInfo = new MSBuildForwardingApp(new string[0], msbuildPath, executeOutOfProc: true).GetProcessStartInfo(); startInfo.Environment.ContainsKey(envVarName).Should().BeTrue(); } [Fact] public void ItSetsMSBuildExtensionPathToExistingPath() { - using var holder = new OutOfProcMSBuildHolder(true); - var msbuildPath = ""; var envVar = "MSBuildExtensionsPath"; - new DirectoryInfo(new MSBuildForwardingApp(new string[0], msbuildPath) + new DirectoryInfo(new MSBuildForwardingApp(new string[0], msbuildPath, executeOutOfProc: true) .GetProcessStartInfo() .Environment[envVar]) .Should() @@ -83,11 +60,9 @@ public void ItSetsMSBuildExtensionPathToExistingPath() [Fact(Skip ="Test app base folder doesn't have Sdks")] public void ItSetsMSBuildSDKsPathToExistingPath() { - using var holder = new OutOfProcMSBuildHolder(true); - var msbuildPath = ""; var envVar = "MSBuildSDKsPath"; - new DirectoryInfo(new MSBuildForwardingApp(new string[0], msbuildPath) + new DirectoryInfo(new MSBuildForwardingApp(new string[0], msbuildPath, executeOutOfProc: true) .GetProcessStartInfo() .Environment[envVar]) .Should() @@ -97,11 +72,9 @@ public void ItSetsMSBuildSDKsPathToExistingPath() [Fact] public void ItSetsOrIgnoresTelemetrySessionId() { - using var holder = new OutOfProcMSBuildHolder(true); - var msbuildPath = ""; var envVar = "DOTNET_CLI_TELEMETRY_SESSIONID"; - var startInfo = new MSBuildForwardingApp(new string[0], msbuildPath) + var startInfo = new MSBuildForwardingApp(new string[0], msbuildPath, executeOutOfProc: true) .GetProcessStartInfo(); string sessionId = startInfo.Environment[envVar]; @@ -120,10 +93,8 @@ public void ItSetsOrIgnoresTelemetrySessionId() [Fact] public void ItDoesNotSetCurrentWorkingDirectory() { - using var holder = new OutOfProcMSBuildHolder(true); - var msbuildPath = ""; - var startInfo = new MSBuildForwardingApp(new string[0], msbuildPath) + var startInfo = new MSBuildForwardingApp(new string[0], msbuildPath, executeOutOfProc: true) .GetProcessStartInfo().WorkingDirectory.Should().Be(""); } } From ddf25465b117578995a8c9f120f4e124a3e2325a Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Wed, 31 Mar 2021 08:22:06 +0200 Subject: [PATCH 08/22] More test fixes --- .../dotnet-msbuild/GivenDotnetBuildInvocation.cs | 7 ++++--- .../GivenDotnetPublishInvocation.cs | 16 +++++++--------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs index 6a04b7e72b57..3feb25c05d5b 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs @@ -46,7 +46,8 @@ public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalA command.SeparateRestoreCommand.Should().BeNull(); - command.GetConcatenatedArguments().Should() + command.GetConcatenatedArguments() + .Should() .Be($"{ExpectedPrefix} -restore -consoleloggerparameters:Summary{expectedAdditionalArgs}"); }); } @@ -79,8 +80,8 @@ public void MsbuildInvocationIsCorrectForSeparateRestore( .Arguments.Should() .Be($"{ExpectedPrefix} {expectedAdditionalArgsForRestore}"); - command.GetProcessStartInfo() - .Arguments.Should() + command.GetConcatenatedArguments() + .Should() .Be($"{ExpectedPrefix} -nologo -consoleloggerparameters:Summary{expectedAdditionalArgs}"); }); diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs index b1db35b00165..2ca415270712 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs @@ -52,8 +52,8 @@ public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalA .Should() .BeNull(); - command.GetProcessStartInfo() - .Arguments.Should() + command.GetConcatenatedArguments() + .Should() .Be($"{ExpectedPrefix} -restore -target:Publish{expectedAdditionalArgs}"); }); } @@ -68,13 +68,12 @@ public void MsbuildInvocationIsCorrectForSeparateRestore(string[] args, string e var msbuildPath = ""; var command = PublishCommand.FromArgs(args, msbuildPath); - command.SeparateRestoreCommand - .GetProcessStartInfo() - .Arguments.Should() + command.GetConcatenatedArguments() + .Should() .Be($"{ExpectedPrefix} -target:Restore"); - command.GetProcessStartInfo() - .Arguments.Should() + command.GetConcatenatedArguments() + .Should() .Be($"{ExpectedPrefix} -nologo -target:Publish{expectedAdditionalArgs}"); } @@ -89,8 +88,7 @@ public void MsbuildInvocationIsCorrectForNoBuild() .BeNull(); // NOTE --no-build implies no-restore hence no -restore argument to msbuild below. - command.GetProcessStartInfo() - .Arguments + command.GetConcatenatedArguments() .Should() .Be($"{ExpectedPrefix} -target:Publish -property:NoBuild=true"); } From 335228ec1557e55dcca81e3bf95a99f476c5e964 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Wed, 31 Mar 2021 09:16:36 +0200 Subject: [PATCH 09/22] Even more previously missed test fixes --- .../dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs index 3feb25c05d5b..1feb5c889155 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs @@ -76,8 +76,8 @@ public void MsbuildInvocationIsCorrectForSeparateRestore( var msbuildPath = ""; var command = BuildCommand.FromArgs(args, msbuildPath); - command.SeparateRestoreCommand.GetProcessStartInfo() - .Arguments.Should() + command.SeparateRestoreCommand.GetConcatenatedArguments() + .Should() .Be($"{ExpectedPrefix} {expectedAdditionalArgsForRestore}"); command.GetConcatenatedArguments() From 7a164163d765d6a6eb38ae6a2ff24acb4ed6e5b0 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Wed, 31 Mar 2021 10:25:37 +0200 Subject: [PATCH 10/22] Even more previously missed test fixes --- .../dotnet-msbuild/GivenDotnetPublishInvocation.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs index 2ca415270712..8a52789ee575 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs @@ -68,7 +68,8 @@ public void MsbuildInvocationIsCorrectForSeparateRestore(string[] args, string e var msbuildPath = ""; var command = PublishCommand.FromArgs(args, msbuildPath); - command.GetConcatenatedArguments() + command.SeparateRestoreCommand + .GetConcatenatedArguments() .Should() .Be($"{ExpectedPrefix} -target:Restore"); From 29f8e6b1a5f135e0e7ba9424058c7a1a400f544a Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Wed, 31 Mar 2021 14:30:44 +0200 Subject: [PATCH 11/22] Early bind Microsoft.Build.CommandLine.MSBuildApp --- .../MSBuildForwardingAppWithoutLogging.cs | 11 +++-------- .../Microsoft.DotNet.Cli.Utils.csproj | 13 ++++++++++++- .../Microsoft.NET.TestFramework/TestContext.cs | 2 ++ 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs index 121739b3b96e..fd42a2618738 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs @@ -6,7 +6,6 @@ using System.Diagnostics; using System.IO; using System.Linq; -using System.Reflection; using System.Runtime.InteropServices; using System.Text; @@ -116,18 +115,14 @@ public int ExecuteInProc(string[] arguments) Environment.SetEnvironmentVariable(kvp.Key, kvp.Value); } - Assembly assembly = Assembly.LoadFrom(_msbuildPath); - Type type = assembly.GetType(MSBuildAppClassName); - MethodInfo mi = type.GetMethod("Main"); - try { - return (int)mi.Invoke(null, new object[] { arguments }); + return Microsoft.Build.CommandLine.MSBuildApp.Main(arguments); } - catch (TargetInvocationException targetException) + catch (Exception exception) { Console.Error.Write("Unhandled exception: "); - Console.Error.WriteLine(targetException.InnerException.ToString()); + Console.Error.WriteLine(exception.ToString()); return unchecked((int)0xe0434352); // EXCEPTION_COMPLUS } diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj b/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj index bbb696e5e5a0..17f7fde8e1d2 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj @@ -15,7 +15,10 @@ - + @@ -28,6 +31,14 @@ + + + + + + diff --git a/src/Tests/Microsoft.NET.TestFramework/TestContext.cs b/src/Tests/Microsoft.NET.TestFramework/TestContext.cs index 2c287db8ffe3..40998fde91fb 100644 --- a/src/Tests/Microsoft.NET.TestFramework/TestContext.cs +++ b/src/Tests/Microsoft.NET.TestFramework/TestContext.cs @@ -208,8 +208,10 @@ public static void Initialize(TestCommandLine commandLine) "MSBuildSDKsPath", Path.Combine(testContext.ToolsetUnderTest.SdksPath)); +#if NETCOREAPP DotNet.Cli.Utils.MSBuildForwardingAppWithoutLogging.MSBuildExtensionsPathTestHook = testContext.ToolsetUnderTest.SdkFolderUnderTest; +#endif } public static string GetRepoRoot() From 6dadaa45d381e4b5e0bd21148b5ac15ec0ef8931 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Wed, 31 Mar 2021 22:40:50 +0200 Subject: [PATCH 12/22] Build against the right MSBuild.dll --- .../Microsoft.DotNet.Cli.Utils.csproj | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj b/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj index 17f7fde8e1d2..7d45fa0334fc 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj @@ -16,6 +16,7 @@ @@ -32,9 +33,8 @@ - + From f1c94cad9441f16b001a96a10dd0873bbddce894 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Fri, 2 Apr 2021 13:29:14 +0200 Subject: [PATCH 13/22] Make sure Microsoft.Build.Tasks.Core gets binplaced to bin\redist where tests run dotnet.dll from --- eng/Versions.props | 1 + src/Tests/dotnet.Tests/dotnet.Tests.csproj | 1 + 2 files changed, 2 insertions(+) diff --git a/eng/Versions.props b/eng/Versions.props index 4887461e63fe..e4102ef8f0b6 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -98,6 +98,7 @@ $(MicrosoftBuildPackageVersion) $(MicrosoftBuildFrameworkPackageVersion) $(MicrosoftBuildUtilitiesCorePackageVersion) + $(MicrosoftBuildPackageVersion) diff --git a/src/Tests/dotnet.Tests/dotnet.Tests.csproj b/src/Tests/dotnet.Tests/dotnet.Tests.csproj index 53973a9c49a7..7b08bb598e88 100644 --- a/src/Tests/dotnet.Tests/dotnet.Tests.csproj +++ b/src/Tests/dotnet.Tests/dotnet.Tests.csproj @@ -148,5 +148,6 @@ + From 7d842ac294f8e15d5b1906c55e903faaa751d09f Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Mon, 5 Apr 2021 16:19:00 +0200 Subject: [PATCH 14/22] Final tweaks --- eng/Versions.props | 3 +- .../MSBuildForwardingAppWithoutLogging.cs | 36 +++++++++++-------- .../ProjectToolsCommandResolver.cs | 7 ++-- .../dotnet-msbuild/MSBuildForwardingApp.cs | 18 +++++----- .../GivenMsbuildForwardingApp.cs | 14 ++++---- 5 files changed, 44 insertions(+), 34 deletions(-) diff --git a/eng/Versions.props b/eng/Versions.props index e4102ef8f0b6..53c539fd008a 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -95,10 +95,11 @@ $(MicrosoftBuildPackageVersion) 16.10.0-preview-21175-01 $(MicrosoftBuildPackageVersion) + $(MicrosoftBuildPackageVersion) $(MicrosoftBuildPackageVersion) $(MicrosoftBuildFrameworkPackageVersion) $(MicrosoftBuildUtilitiesCorePackageVersion) - $(MicrosoftBuildPackageVersion) + $(MicrosoftBuildTasksCorePackageVersion) diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs index fd42a2618738..2f1e0d802a37 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs @@ -13,9 +13,7 @@ namespace Microsoft.DotNet.Cli.Utils { internal class MSBuildForwardingAppWithoutLogging { - private static readonly bool ExecuteMSBuildOutOfProcByDefault = Env.GetEnvironmentVariableAsBool("DOTNET_CLI_EXEC_MSBUILD"); - - private const string MSBuildAppClassName = "Microsoft.Build.CommandLine.MSBuildApp"; + private static readonly bool AlwaysExecuteMSBuildOutOfProc = Env.GetEnvironmentVariableAsBool("DOTNET_CLI_EXEC_MSBUILD"); private const string MSBuildExeName = "MSBuild.dll"; @@ -24,13 +22,16 @@ internal class MSBuildForwardingAppWithoutLogging // Null if we're running MSBuild in-proc. private ForwardingAppImplementation _forwardingApp; - private IEnumerable _argsToForward; - - private string _msbuildPath; + // Command line arguments we're asked to forward to MSBuild. + private readonly IEnumerable _argsToForward; internal static string MSBuildExtensionsPathTestHook = null; - internal bool ExecuteMSBuildOutOfProc => _forwardingApp != null; + // Path to the MSBuild binary to use. + public string MSBuildPath { get; } + + // True if, given current state of the class, MSBuild would be executed in its own process. + public bool ExecuteMSBuildOutOfProc => _forwardingApp != null; private readonly Dictionary _msbuildRequiredEnvironmentVariables = new Dictionary @@ -43,15 +44,18 @@ internal class MSBuildForwardingAppWithoutLogging private readonly IEnumerable _msbuildRequiredParameters = new List { "-maxcpucount", "-verbosity:m" }; - public MSBuildForwardingAppWithoutLogging(IEnumerable argsToForward, string msbuildPath = null, bool? executeOutOfProc = null) + public MSBuildForwardingAppWithoutLogging(IEnumerable argsToForward, string msbuildPath = null) { + string defaultMSBuildPath = GetMSBuildExePath(); + _argsToForward = argsToForward; - _msbuildPath = msbuildPath ?? GetMSBuildExePath(); + MSBuildPath = msbuildPath ?? defaultMSBuildPath; - if (executeOutOfProc == true || (executeOutOfProc == null && ExecuteMSBuildOutOfProcByDefault)) + // If DOTNET_CLI_EXEC_MSBUILD is set or we're asked to execute a non-default binary, call MSBuild out-of-proc. + if (AlwaysExecuteMSBuildOutOfProc || string.Compare(MSBuildPath, defaultMSBuildPath, StringComparison.OrdinalIgnoreCase) != 0) { _forwardingApp = new ForwardingAppImplementation( - _msbuildPath, + MSBuildPath, _msbuildRequiredParameters.Concat(argsToForward.Select(Escape)), environmentVariables: _msbuildRequiredEnvironmentVariables); } @@ -82,10 +86,10 @@ public void EnvironmentVariable(string name, string value) if (value == string.Empty || value == "\0") { // Unlike ProcessStartInfo.EnvironmentVariables, Environment.SetEnvironmentVariable can't set a variable - // to an empty value, so we just fall back to calling MSBuild out-of-proc if we encounter this. - // https://github.com/dotnet/runtime/issues/34446 + // to an empty value, so we just fall back to calling MSBuild out-of-proc if we encounter this case. + // https://github.com/dotnet/runtime/issues/50554 _forwardingApp = new ForwardingAppImplementation( - _msbuildPath, + MSBuildPath, _msbuildRequiredParameters.Concat(_argsToForward.Select(Escape)), environmentVariables: _msbuildRequiredEnvironmentVariables); } @@ -117,10 +121,14 @@ public int ExecuteInProc(string[] arguments) try { + // Execute MSBuild in the current process by calling its Main method. return Microsoft.Build.CommandLine.MSBuildApp.Main(arguments); } catch (Exception exception) { + // MSBuild, like all well-behaved CLI tools, handles all exceptions. In the unlikely case + // that something still escapes, we print the exception and fail the call. Non-localized + // string is OK here. Console.Error.Write("Unhandled exception: "); Console.Error.WriteLine(exception.ToString()); diff --git a/src/Cli/dotnet/CommandFactory/CommandResolution/ProjectToolsCommandResolver.cs b/src/Cli/dotnet/CommandFactory/CommandResolution/ProjectToolsCommandResolver.cs index 7a8f6319ddd2..c6e3a4bc2a1c 100644 --- a/src/Cli/dotnet/CommandFactory/CommandResolution/ProjectToolsCommandResolver.cs +++ b/src/Cli/dotnet/CommandFactory/CommandResolution/ProjectToolsCommandResolver.cs @@ -412,6 +412,7 @@ internal void GenerateDepsJsonFile( } else { + // Execute and capture output of MSBuild running in-process. var outWriter = new StringWriter(); var errWriter = new StringWriter(); var savedOutWriter = Console.Out; @@ -422,12 +423,12 @@ internal void GenerateDepsJsonFile( Console.SetError(errWriter); result = forwardingAppWithoutLogging.Execute(); + + stdOut = outWriter.ToString(); + stdErr = errWriter.ToString(); } finally { - stdOut = outWriter.ToString(); - stdErr = errWriter.ToString(); - Console.SetOut(savedOutWriter); Console.SetError(savedErrWriter); } diff --git a/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs b/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs index 8b13ef248130..32e2ba7c98a9 100644 --- a/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs +++ b/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs @@ -44,12 +44,11 @@ private static IEnumerable ConcatTelemetryLogger(IEnumerable arg return argsToForward; } - public MSBuildForwardingApp(IEnumerable argsToForward, string msbuildPath = null, bool? executeOutOfProc = null) + public MSBuildForwardingApp(IEnumerable argsToForward, string msbuildPath = null) { _forwardingAppWithoutLogging = new MSBuildForwardingAppWithoutLogging( ConcatTelemetryLogger(argsToForward), - msbuildPath, - executeOutOfProc); + msbuildPath); // Add the performance log location to the environment of the target process. if (PerformanceLogManager.Instance != null && !string.IsNullOrEmpty(PerformanceLogManager.Instance.CurrentLogDirectory)) @@ -78,14 +77,13 @@ public string GetConcatenatedArguments() public virtual int Execute() { - int exitCode; + // Ignore Ctrl-C for the remainder of the command's execution + // Forwarding commands will just spawn the child process and exit + Console.CancelKeyPress += (sender, e) => { e.Cancel = true; }; + int exitCode; if (_forwardingAppWithoutLogging.ExecuteMSBuildOutOfProc) { - // Ignore Ctrl-C for the remainder of the command's execution - // Forwarding commands will just spawn the child process and exit - Console.CancelKeyPress += (sender, e) => { e.Cancel = true; }; - ProcessStartInfo startInfo = GetProcessStartInfo(); PerformanceLogEventSource.Log.LogMSBuildStart(startInfo.FileName, startInfo.Arguments); @@ -97,7 +95,9 @@ public virtual int Execute() string[] arguments = _forwardingAppWithoutLogging.GetAllArgumentsUnescaped(); if (PerformanceLogEventSource.Log.IsEnabled()) { - PerformanceLogEventSource.Log.LogMSBuildStart(string.Empty, ArgumentEscaper.EscapeAndConcatenateArgArrayForProcessStart(arguments)); + PerformanceLogEventSource.Log.LogMSBuildStart( + _forwardingAppWithoutLogging.MSBuildPath, + ArgumentEscaper.EscapeAndConcatenateArgArrayForProcessStart(arguments)); } exitCode = _forwardingAppWithoutLogging.ExecuteInProc(arguments); PerformanceLogEventSource.Log.MSBuildStop(exitCode); diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenMsbuildForwardingApp.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenMsbuildForwardingApp.cs index a3689873bf95..c320b4f9c123 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenMsbuildForwardingApp.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenMsbuildForwardingApp.cs @@ -22,7 +22,7 @@ public GivenMsbuildForwardingApp(ITestOutputHelper log) : base(log) public void DotnetExeIsExecuted() { var msbuildPath = ""; - new MSBuildForwardingApp(new string[0], msbuildPath, executeOutOfProc: true) + new MSBuildForwardingApp(new string[0], msbuildPath) .GetProcessStartInfo().FileName.Should().Be("dotnet.exe"); } @@ -30,7 +30,7 @@ public void DotnetExeIsExecuted() public void DotnetIsExecuted() { var msbuildPath = ""; - new MSBuildForwardingApp(new string[0], msbuildPath, executeOutOfProc: true) + new MSBuildForwardingApp(new string[0], msbuildPath) .GetProcessStartInfo().FileName.Should().Be("dotnet"); } @@ -41,7 +41,7 @@ public void DotnetIsExecuted() public void ItSetsEnvironmentalVariables(string envVarName) { var msbuildPath = ""; - var startInfo = new MSBuildForwardingApp(new string[0], msbuildPath, executeOutOfProc: true).GetProcessStartInfo(); + var startInfo = new MSBuildForwardingApp(new string[0], msbuildPath).GetProcessStartInfo(); startInfo.Environment.ContainsKey(envVarName).Should().BeTrue(); } @@ -50,7 +50,7 @@ public void ItSetsMSBuildExtensionPathToExistingPath() { var msbuildPath = ""; var envVar = "MSBuildExtensionsPath"; - new DirectoryInfo(new MSBuildForwardingApp(new string[0], msbuildPath, executeOutOfProc: true) + new DirectoryInfo(new MSBuildForwardingApp(new string[0], msbuildPath) .GetProcessStartInfo() .Environment[envVar]) .Should() @@ -62,7 +62,7 @@ public void ItSetsMSBuildSDKsPathToExistingPath() { var msbuildPath = ""; var envVar = "MSBuildSDKsPath"; - new DirectoryInfo(new MSBuildForwardingApp(new string[0], msbuildPath, executeOutOfProc: true) + new DirectoryInfo(new MSBuildForwardingApp(new string[0], msbuildPath) .GetProcessStartInfo() .Environment[envVar]) .Should() @@ -74,7 +74,7 @@ public void ItSetsOrIgnoresTelemetrySessionId() { var msbuildPath = ""; var envVar = "DOTNET_CLI_TELEMETRY_SESSIONID"; - var startInfo = new MSBuildForwardingApp(new string[0], msbuildPath, executeOutOfProc: true) + var startInfo = new MSBuildForwardingApp(new string[0], msbuildPath) .GetProcessStartInfo(); string sessionId = startInfo.Environment[envVar]; @@ -94,7 +94,7 @@ public void ItSetsOrIgnoresTelemetrySessionId() public void ItDoesNotSetCurrentWorkingDirectory() { var msbuildPath = ""; - var startInfo = new MSBuildForwardingApp(new string[0], msbuildPath, executeOutOfProc: true) + var startInfo = new MSBuildForwardingApp(new string[0], msbuildPath) .GetProcessStartInfo().WorkingDirectory.Should().Be(""); } } From f1f3cabd80b6de3e588d770bd26dd661bf8f6c9b Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Mon, 12 Apr 2021 08:53:46 +0200 Subject: [PATCH 15/22] PR feedback: More descriptive ennvironment variable --- .../MSBuildForwardingAppWithoutLogging.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs index 2f1e0d802a37..5d46292b0a2e 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs @@ -13,7 +13,7 @@ namespace Microsoft.DotNet.Cli.Utils { internal class MSBuildForwardingAppWithoutLogging { - private static readonly bool AlwaysExecuteMSBuildOutOfProc = Env.GetEnvironmentVariableAsBool("DOTNET_CLI_EXEC_MSBUILD"); + private static readonly bool AlwaysExecuteMSBuildOutOfProc = Env.GetEnvironmentVariableAsBool("DOTNET_CLI_RUN_MSBUILD_OUTOFPROC"); private const string MSBuildExeName = "MSBuild.dll"; @@ -51,7 +51,7 @@ public MSBuildForwardingAppWithoutLogging(IEnumerable argsToForward, str _argsToForward = argsToForward; MSBuildPath = msbuildPath ?? defaultMSBuildPath; - // If DOTNET_CLI_EXEC_MSBUILD is set or we're asked to execute a non-default binary, call MSBuild out-of-proc. + // If DOTNET_CLI_RUN_MSBUILD_OUTOFPROC is set or we're asked to execute a non-default binary, call MSBuild out-of-proc. if (AlwaysExecuteMSBuildOutOfProc || string.Compare(MSBuildPath, defaultMSBuildPath, StringComparison.OrdinalIgnoreCase) != 0) { _forwardingApp = new ForwardingAppImplementation( From b2d89a5800e2eda2d0fa6a897b7736a5aa8d6f7c Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Mon, 12 Apr 2021 08:56:26 +0200 Subject: [PATCH 16/22] PR feedback: String.Compare -> String.Equals --- .../MSBuildForwardingAppWithoutLogging.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs index 5d46292b0a2e..4a3c66d3da97 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs @@ -52,7 +52,7 @@ public MSBuildForwardingAppWithoutLogging(IEnumerable argsToForward, str MSBuildPath = msbuildPath ?? defaultMSBuildPath; // If DOTNET_CLI_RUN_MSBUILD_OUTOFPROC is set or we're asked to execute a non-default binary, call MSBuild out-of-proc. - if (AlwaysExecuteMSBuildOutOfProc || string.Compare(MSBuildPath, defaultMSBuildPath, StringComparison.OrdinalIgnoreCase) != 0) + if (AlwaysExecuteMSBuildOutOfProc || !string.Equals(MSBuildPath, defaultMSBuildPath, StringComparison.OrdinalIgnoreCase)) { _forwardingApp = new ForwardingAppImplementation( MSBuildPath, From b0a83a7b60faf98346971de08a8704adc2a45530 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Mon, 12 Apr 2021 09:02:26 +0200 Subject: [PATCH 17/22] PR feedback: GetAllArgumentsUnescaped -> GetAllArguments --- .../MSBuildForwardingAppWithoutLogging.cs | 4 ++-- .../dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs | 4 ++-- .../dotnet.Tests/dotnet-msbuild/DotnetMsbuildInProcTests.cs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs index 4a3c66d3da97..33b7977a1c9f 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs @@ -67,7 +67,7 @@ public virtual ProcessStartInfo GetProcessStartInfo() return _forwardingApp.GetProcessStartInfo(); } - public string[] GetAllArgumentsUnescaped() + public string[] GetAllArguments() { return _msbuildRequiredParameters.Concat(_argsToForward.Select(Escape)).ToArray(); } @@ -103,7 +103,7 @@ public int Execute() } else { - return ExecuteInProc(GetAllArgumentsUnescaped()); + return ExecuteInProc(GetAllArguments()); } } diff --git a/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs b/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs index 32e2ba7c98a9..80a2ab269559 100644 --- a/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs +++ b/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs @@ -71,7 +71,7 @@ public ProcessStartInfo GetProcessStartInfo() public string GetConcatenatedArguments() { - var argumentsUnescaped = _forwardingAppWithoutLogging.GetAllArgumentsUnescaped(); + var argumentsUnescaped = _forwardingAppWithoutLogging.GetAllArguments(); return Cli.Utils.ArgumentEscaper.EscapeAndConcatenateArgArrayForProcessStart(argumentsUnescaped); } @@ -92,7 +92,7 @@ public virtual int Execute() } else { - string[] arguments = _forwardingAppWithoutLogging.GetAllArgumentsUnescaped(); + string[] arguments = _forwardingAppWithoutLogging.GetAllArguments(); if (PerformanceLogEventSource.Log.IsEnabled()) { PerformanceLogEventSource.Log.LogMSBuildStart( diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/DotnetMsbuildInProcTests.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/DotnetMsbuildInProcTests.cs index 9878dd3ce0bb..82b0ffe27433 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/DotnetMsbuildInProcTests.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/DotnetMsbuildInProcTests.cs @@ -65,7 +65,7 @@ private string[] GetArgsForMSBuild(Func sentinelExists, out Telemetry.Tele .GetField("_forwardingAppWithoutLogging", BindingFlags.Instance | BindingFlags.NonPublic) ?.GetValue(msBuildForwardingApp) as Cli.Utils.MSBuildForwardingAppWithoutLogging; - return forwardingAppWithoutLogging?.GetAllArgumentsUnescaped(); + return forwardingAppWithoutLogging?.GetAllArguments(); } } public sealed class MockFirstTimeUseNoticeSentinel : IFirstTimeUseNoticeSentinel From e119eb11d37fc0802661fa8e4b98d402314cb4b2 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Mon, 12 Apr 2021 09:24:31 +0200 Subject: [PATCH 18/22] PR feedback: Factor out InitializeForOutOfProcForwarding --- .../MSBuildForwardingAppWithoutLogging.cs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs index 33b7977a1c9f..c9cc3c607f6d 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs @@ -54,13 +54,18 @@ public MSBuildForwardingAppWithoutLogging(IEnumerable argsToForward, str // If DOTNET_CLI_RUN_MSBUILD_OUTOFPROC is set or we're asked to execute a non-default binary, call MSBuild out-of-proc. if (AlwaysExecuteMSBuildOutOfProc || !string.Equals(MSBuildPath, defaultMSBuildPath, StringComparison.OrdinalIgnoreCase)) { - _forwardingApp = new ForwardingAppImplementation( - MSBuildPath, - _msbuildRequiredParameters.Concat(argsToForward.Select(Escape)), - environmentVariables: _msbuildRequiredEnvironmentVariables); + InitializeForOutOfProcForwarding(); } } + private void InitializeForOutOfProcForwarding() + { + _forwardingApp = new ForwardingAppImplementation( + MSBuildPath, + _msbuildRequiredParameters.Concat(_argsToForward.Select(Escape)), + environmentVariables: _msbuildRequiredEnvironmentVariables); + } + public virtual ProcessStartInfo GetProcessStartInfo() { Debug.Assert(_forwardingApp != null, "Can't get ProcessStartInfo when not executing out-of-proc"); @@ -88,10 +93,7 @@ public void EnvironmentVariable(string name, string value) // Unlike ProcessStartInfo.EnvironmentVariables, Environment.SetEnvironmentVariable can't set a variable // to an empty value, so we just fall back to calling MSBuild out-of-proc if we encounter this case. // https://github.com/dotnet/runtime/issues/50554 - _forwardingApp = new ForwardingAppImplementation( - MSBuildPath, - _msbuildRequiredParameters.Concat(_argsToForward.Select(Escape)), - environmentVariables: _msbuildRequiredEnvironmentVariables); + InitializeForOutOfProcForwarding(); } } From 860a7ab1b184b5e9ad074fc691dd38b32745a457 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Mon, 12 Apr 2021 10:36:21 +0200 Subject: [PATCH 19/22] PR feedback: Add validation for the non-standard MSBuild.dll reference --- .../Microsoft.DotNet.Cli.Utils.csproj | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj b/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj index 7d45fa0334fc..5a653268bd25 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj @@ -14,6 +14,16 @@ + + + $(PkgMicrosoft_Build_Runtime)\contentFiles\any\net5.0\MSBuild.dll + + + + + + + - - From 7a8b822916f5500d773ab9fb00820d84dcd45941 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Mon, 12 Apr 2021 10:40:27 +0200 Subject: [PATCH 20/22] PR feedback: -> #if NET --- .../MSBuildForwardingAppWithoutLogging.cs | 4 ++++ .../Microsoft.DotNet.Cli.Utils.csproj | 4 ---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs index c9cc3c607f6d..372a331399e6 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs @@ -1,6 +1,8 @@ // 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. +#if NET + using System; using System.Collections.Generic; using System.Diagnostics; @@ -189,3 +191,5 @@ private static bool IsRestoreSources(string arg) } } } + +#endif diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj b/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj index 5a653268bd25..e240c7f8700c 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj @@ -44,8 +44,4 @@ - - - - From d45763d80cd48808d619d96e1d75d3baa4b97b22 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Mon, 12 Apr 2021 10:43:04 +0200 Subject: [PATCH 21/22] PR feedback: Make GetProcessStartInfo non-virtual --- .../MSBuildForwardingAppWithoutLogging.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs index 372a331399e6..c4c2758580ae 100644 --- a/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs +++ b/src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs @@ -68,7 +68,7 @@ private void InitializeForOutOfProcForwarding() environmentVariables: _msbuildRequiredEnvironmentVariables); } - public virtual ProcessStartInfo GetProcessStartInfo() + public ProcessStartInfo GetProcessStartInfo() { Debug.Assert(_forwardingApp != null, "Can't get ProcessStartInfo when not executing out-of-proc"); return _forwardingApp.GetProcessStartInfo(); From f55adc98632babce7a28572e3a4bebc7167c692d Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Mon, 12 Apr 2021 10:47:08 +0200 Subject: [PATCH 22/22] PR feedback: GetConcatenatedArguments -> GetArgumentsToMSBuild --- .../commands/dotnet-msbuild/MSBuildForwardingApp.cs | 5 ++++- .../MSBuildArgumentCommandLineParserTests.cs | 2 +- .../dotnet-msbuild/GivenDotnetBuildInvocation.cs | 6 +++--- .../dotnet-msbuild/GivenDotnetCleanInvocation.cs | 4 ++-- .../dotnet-msbuild/GivenDotnetPackInvocation.cs | 2 +- .../dotnet-msbuild/GivenDotnetPublishInvocation.cs | 10 +++++----- .../dotnet-msbuild/GivenDotnetRestoreInvocation.cs | 2 +- .../dotnet-msbuild/GivenDotnetStoreInvocation.cs | 6 +++--- 8 files changed, 20 insertions(+), 17 deletions(-) diff --git a/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs b/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs index 80a2ab269559..18a402fdb855 100644 --- a/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs +++ b/src/Cli/dotnet/commands/dotnet-msbuild/MSBuildForwardingApp.cs @@ -69,7 +69,10 @@ public ProcessStartInfo GetProcessStartInfo() return _forwardingAppWithoutLogging.GetProcessStartInfo(); } - public string GetConcatenatedArguments() + /// + /// Test hook returning concatenated and escaped command line arguments that would be passed to MSBuild. + /// + internal string GetArgumentsToMSBuild() { var argumentsUnescaped = _forwardingAppWithoutLogging.GetAllArguments(); return Cli.Utils.ArgumentEscaper.EscapeAndConcatenateArgArrayForProcessStart(argumentsUnescaped); diff --git a/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs b/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs index 4bc2385603ad..44a365c38d7c 100644 --- a/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs +++ b/src/Tests/dotnet.Tests/ParserTests/MSBuildArgumentCommandLineParserTests.cs @@ -31,7 +31,7 @@ public void MSBuildArgumentsAreForwardedCorrectly(string[] arguments, bool build RestoringCommand command = buildCommand ? BuildCommand.FromArgs(arguments) : PublishCommand.FromArgs(arguments); - command.GetConcatenatedArguments().Split(' ') + command.GetArgumentsToMSBuild().Split(' ') .Should() .Contain(arguments); } diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs index 1feb5c889155..0aa34445315a 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs @@ -46,7 +46,7 @@ public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalA command.SeparateRestoreCommand.Should().BeNull(); - command.GetConcatenatedArguments() + command.GetArgumentsToMSBuild() .Should() .Be($"{ExpectedPrefix} -restore -consoleloggerparameters:Summary{expectedAdditionalArgs}"); }); @@ -76,11 +76,11 @@ public void MsbuildInvocationIsCorrectForSeparateRestore( var msbuildPath = ""; var command = BuildCommand.FromArgs(args, msbuildPath); - command.SeparateRestoreCommand.GetConcatenatedArguments() + command.SeparateRestoreCommand.GetArgumentsToMSBuild() .Should() .Be($"{ExpectedPrefix} {expectedAdditionalArgsForRestore}"); - command.GetConcatenatedArguments() + command.GetArgumentsToMSBuild() .Should() .Be($"{ExpectedPrefix} -nologo -consoleloggerparameters:Summary{expectedAdditionalArgs}"); }); diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetCleanInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetCleanInvocation.cs index 4bef90dd408d..ad99ee29cb8a 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetCleanInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetCleanInvocation.cs @@ -22,7 +22,7 @@ public void ItAddsProjectToMsbuildInvocation() { var msbuildPath = ""; CleanCommand.FromArgs(new string[] { "" }, msbuildPath) - .GetConcatenatedArguments().Should().Be("-maxcpucount -verbosity:m -verbosity:normal -target:Clean"); + .GetArgumentsToMSBuild().Should().Be("-maxcpucount -verbosity:m -verbosity:normal -target:Clean"); } [Theory] @@ -45,7 +45,7 @@ public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalA var msbuildPath = ""; CleanCommand.FromArgs(args, msbuildPath) - .GetConcatenatedArguments().Should().Be($"{ExpectedPrefix}{expectedAdditionalArgs}"); + .GetArgumentsToMSBuild().Should().Be($"{ExpectedPrefix}{expectedAdditionalArgs}"); }); } } diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPackInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPackInvocation.cs index f9e381c3fe21..132427003303 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPackInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPackInvocation.cs @@ -48,7 +48,7 @@ public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalA var expectedPrefix = args.FirstOrDefault() == "--no-build" ? ExpectedNoBuildPrefix : ExpectedPrefix; command.SeparateRestoreCommand.Should().BeNull(); - command.GetConcatenatedArguments().Should().Be($"{expectedPrefix}{expectedAdditionalArgs}"); + command.GetArgumentsToMSBuild().Should().Be($"{expectedPrefix}{expectedAdditionalArgs}"); }); } } diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs index 8a52789ee575..5e53b797b6d5 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetPublishInvocation.cs @@ -52,7 +52,7 @@ public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalA .Should() .BeNull(); - command.GetConcatenatedArguments() + command.GetArgumentsToMSBuild() .Should() .Be($"{ExpectedPrefix} -restore -target:Publish{expectedAdditionalArgs}"); }); @@ -69,11 +69,11 @@ public void MsbuildInvocationIsCorrectForSeparateRestore(string[] args, string e var command = PublishCommand.FromArgs(args, msbuildPath); command.SeparateRestoreCommand - .GetConcatenatedArguments() + .GetArgumentsToMSBuild() .Should() .Be($"{ExpectedPrefix} -target:Restore"); - command.GetConcatenatedArguments() + command.GetArgumentsToMSBuild() .Should() .Be($"{ExpectedPrefix} -nologo -target:Publish{expectedAdditionalArgs}"); } @@ -89,7 +89,7 @@ public void MsbuildInvocationIsCorrectForNoBuild() .BeNull(); // NOTE --no-build implies no-restore hence no -restore argument to msbuild below. - command.GetConcatenatedArguments() + command.GetArgumentsToMSBuild() .Should() .Be($"{ExpectedPrefix} -target:Publish -property:NoBuild=true"); } @@ -100,7 +100,7 @@ public void CommandAcceptsMultipleCustomProperties() var msbuildPath = ""; var command = PublishCommand.FromArgs(new[] { "/p:Prop1=prop1", "/p:Prop2=prop2" }, msbuildPath); - command.GetConcatenatedArguments() + command.GetArgumentsToMSBuild() .Should() .Be($"{ExpectedPrefix} -restore -target:Publish -property:Prop1=prop1 -property:Prop2=prop2"); } diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetRestoreInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetRestoreInvocation.cs index fe9108c9a254..1363cf97ce68 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetRestoreInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetRestoreInvocation.cs @@ -49,7 +49,7 @@ public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalA var msbuildPath = ""; RestoreCommand.FromArgs(args, msbuildPath) - .GetConcatenatedArguments() + .GetArgumentsToMSBuild() .Should().Be($"{ExpectedPrefix}{expectedAdditionalArgs}"); }); } diff --git a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetStoreInvocation.cs b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetStoreInvocation.cs index 65551f8d557b..480e97afbb1b 100644 --- a/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetStoreInvocation.cs +++ b/src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetStoreInvocation.cs @@ -25,7 +25,7 @@ public void ItAddsProjectToMsbuildInvocation(string optionName) var msbuildPath = ""; string[] args = new string[] { optionName, "" }; StoreCommand.FromArgs(args, msbuildPath) - .GetConcatenatedArguments().Should().Be($"{ExpectedPrefix}"); + .GetArgumentsToMSBuild().Should().Be($"{ExpectedPrefix}"); } [Theory] @@ -46,7 +46,7 @@ public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalA var msbuildPath = ""; StoreCommand.FromArgs(args, msbuildPath) - .GetConcatenatedArguments().Should().Be($"{ExpectedPrefix}{expectedAdditionalArgs}"); + .GetArgumentsToMSBuild().Should().Be($"{ExpectedPrefix}{expectedAdditionalArgs}"); }); } @@ -60,7 +60,7 @@ public void ItAddsOutputPathToMsBuildInvocation(string optionName) var msbuildPath = ""; StoreCommand.FromArgs(args, msbuildPath) - .GetConcatenatedArguments().Should().Be($"{ExpectedPrefix} -property:ComposeDir={Path.GetFullPath(path)}"); + .GetArgumentsToMSBuild().Should().Be($"{ExpectedPrefix} -property:ComposeDir={Path.GetFullPath(path)}"); } } }