From 20b0b820a8944dd4402f4776fe193a42707edd91 Mon Sep 17 00:00:00 2001 From: Nick Guerrera Date: Tue, 24 Oct 2017 15:04:24 -0700 Subject: [PATCH 1/3] Remove unnecessary handling of verbosity in RestoreCommand We were taking care to set the console verbosity to minimal, but only when no verbosity argument is passed. However, the default verbosity for all CLI msbuild commands is already minimal and so we can just get out of the way. --- src/dotnet/commands/dotnet-restore/Program.cs | 14 +------------- .../GivenDotnetRestoreInvocation.cs | 15 +-------------- 2 files changed, 2 insertions(+), 27 deletions(-) diff --git a/src/dotnet/commands/dotnet-restore/Program.cs b/src/dotnet/commands/dotnet-restore/Program.cs index b0269370f..6cc324098 100644 --- a/src/dotnet/commands/dotnet-restore/Program.cs +++ b/src/dotnet/commands/dotnet-restore/Program.cs @@ -19,7 +19,7 @@ namespace Microsoft.DotNet.Tools.Restore { } - public static RestoreCommand FromArgs(string[] args, string msbuildPath = null) + public static RestoreCommand FromArgs(string[] args, string msbuildPath = null, bool noLogo = true) { DebugHelper.HandleDebugSwitch(ref args); @@ -37,11 +37,6 @@ namespace Microsoft.DotNet.Tools.Restore "/t:Restore" }; - if (!HasVerbosityOption(parsedRestore)) - { - msbuildArgs.Add("/ConsoleLoggerParameters:Verbosity=Minimal"); - } - msbuildArgs.AddRange(parsedRestore.OptionValuesToBeForwarded()); msbuildArgs.AddRange(parsedRestore.Arguments); @@ -65,12 +60,5 @@ namespace Microsoft.DotNet.Tools.Restore return cmd.Execute(); } - - private static bool HasVerbosityOption(AppliedOption parsedRestore) - { - return parsedRestore.HasOption("verbosity") || - parsedRestore.Arguments.Any(a => a.Contains("/v:")) || - parsedRestore.Arguments.Any(a => a.Contains("/verbosity:")); - } } } \ No newline at end of file diff --git a/test/dotnet-msbuild.Tests/GivenDotnetRestoreInvocation.cs b/test/dotnet-msbuild.Tests/GivenDotnetRestoreInvocation.cs index dd55e2abd..1c1802b2d 100644 --- a/test/dotnet-msbuild.Tests/GivenDotnetRestoreInvocation.cs +++ b/test/dotnet-msbuild.Tests/GivenDotnetRestoreInvocation.cs @@ -13,9 +13,6 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests private const string ExpectedPrefix = "exec /m /v:m /NoLogo /t:Restore"; - private string ExpectedPrefixWithConsoleLoggerParamaters = - $"{ExpectedPrefix} /ConsoleLoggerParameters:Verbosity=Minimal"; - [Theory] [InlineData(new string[] { }, "")] [InlineData(new string[] { "-s", "" }, "/p:RestoreSources=")] @@ -30,19 +27,9 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests [InlineData(new string[] { "--no-cache" }, "/p:RestoreNoCache=true")] [InlineData(new string[] { "--ignore-failed-sources" }, "/p:RestoreIgnoreFailedSources=true")] [InlineData(new string[] { "--no-dependencies" }, "/p:RestoreRecursive=false")] - public void MsbuildInvocationWithConsoleLoggerParametersIsCorrect(string[] args, string expectedAdditionalArgs) - { - expectedAdditionalArgs = (string.IsNullOrEmpty(expectedAdditionalArgs) ? "" : $" {expectedAdditionalArgs}"); - - var msbuildPath = ""; - RestoreCommand.FromArgs(args, msbuildPath) - .GetProcessStartInfo().Arguments - .Should().Be($"{ExpectedPrefixWithConsoleLoggerParamaters}{expectedAdditionalArgs}"); - } - [InlineData(new string[] { "-v", "minimal" }, @"/verbosity:minimal")] [InlineData(new string[] { "--verbosity", "minimal" }, @"/verbosity:minimal")] - public void MsbuildInvocationWithVerbosityIsCorrect(string[] args, string expectedAdditionalArgs) + public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalArgs) { expectedAdditionalArgs = (string.IsNullOrEmpty(expectedAdditionalArgs) ? "" : $" {expectedAdditionalArgs}"); From 8fead788d77ddf4003cb271fb00787ec7959938b Mon Sep 17 00:00:00 2001 From: Nick Guerrera Date: Wed, 25 Oct 2017 10:42:08 -0700 Subject: [PATCH 2/3] Fix `dotnet build /clp:NoSummary` --- src/dotnet/commands/dotnet-build/BuildCommand.cs | 4 ++-- test/dotnet-msbuild.Tests/GivenDotnetBuildInvocation.cs | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/dotnet/commands/dotnet-build/BuildCommand.cs b/src/dotnet/commands/dotnet-build/BuildCommand.cs index c7127353b..3fcee0925 100644 --- a/src/dotnet/commands/dotnet-build/BuildCommand.cs +++ b/src/dotnet/commands/dotnet-build/BuildCommand.cs @@ -37,6 +37,8 @@ namespace Microsoft.DotNet.Tools.Build var appliedBuildOptions = result["dotnet"]["build"]; + msbuildArgs.Add($"/clp:Summary"); + if (appliedBuildOptions.HasOption("--no-incremental")) { msbuildArgs.Add("/t:Rebuild"); @@ -50,8 +52,6 @@ namespace Microsoft.DotNet.Tools.Build msbuildArgs.AddRange(appliedBuildOptions.Arguments); - msbuildArgs.Add($"/clp:Summary"); - bool noRestore = appliedBuildOptions.HasOption("--no-restore"); return new BuildCommand( diff --git a/test/dotnet-msbuild.Tests/GivenDotnetBuildInvocation.cs b/test/dotnet-msbuild.Tests/GivenDotnetBuildInvocation.cs index 259d5e257..e0742823f 100644 --- a/test/dotnet-msbuild.Tests/GivenDotnetBuildInvocation.cs +++ b/test/dotnet-msbuild.Tests/GivenDotnetBuildInvocation.cs @@ -9,8 +9,7 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests { public class GivenDotnetBuildInvocation { - const string ExpectedPrefix = "exec /m /v:m"; - const string ExpectedSuffix = "/clp:Summary"; + const string ExpectedPrefix = "exec /m /v:m /clp:Summary"; [Theory] [InlineData(new string[] { }, "/t:Build")] @@ -36,7 +35,7 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests var msbuildPath = ""; BuildCommand.FromArgs(args, msbuildPath) - .GetProcessStartInfo().Arguments.Should().Be($"{ExpectedPrefix}{expectedAdditionalArgs} {ExpectedSuffix}"); + .GetProcessStartInfo().Arguments.Should().Be($"{ExpectedPrefix}{expectedAdditionalArgs}"); } } } From 35b7ad27891aa1fdfc7505900cfe83e1aa6cdec3 Mon Sep 17 00:00:00 2001 From: Nick Guerrera Date: Tue, 24 Oct 2017 15:50:43 -0700 Subject: [PATCH 3/3] Use msbuild /restore instead of separate invocations where possible It is not currently possible when there is a -f|--framework argument because we cannot force a TargetFramework global property on to the restore evaluation. Doing so completely breaks restore by applying the TargetFramework to all projects transitively. The correct behavior is to restore for all frameworks, then build/publish/etc for the given target framework. Achieving that still requires two distinct msbuild invocations. This also changes the verbosity of implicit restore from quiet to that of the subsequent command (default=minimal). Similar to global properties, we cannot specify a distinct console verbosity for the /restore portion of the overall execution. For consistency, we apply the same verbosity change to the case where we still use two separate msbuild invocations. This also fixes an issue where the separate restore invocation's msbuild log would be overwritten by the subsequent command execution. However, this remains unfixed in the case where we still use two separate msbuild invocations. --- src/dotnet/commands/RestoringCommand.cs | 80 +++++++++++++------ src/dotnet/commands/dotnet-restore/Program.cs | 11 ++- .../GivenDotnetBuildInvocation.cs | 41 ++++++++-- .../GivenDotnetPackInvocation.cs | 8 +- .../GivenDotnetPublishInvocation.cs | 37 +++++++-- .../GivenDotnetRestoreInvocation.cs | 2 +- 6 files changed, 132 insertions(+), 47 deletions(-) diff --git a/src/dotnet/commands/RestoringCommand.cs b/src/dotnet/commands/RestoringCommand.cs index 0239fd69b..8d4f3c923 100644 --- a/src/dotnet/commands/RestoringCommand.cs +++ b/src/dotnet/commands/RestoringCommand.cs @@ -1,6 +1,7 @@ // 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; using System.Collections.Generic; using System.Linq; using Microsoft.DotNet.Tools.MSBuild; @@ -10,26 +11,7 @@ namespace Microsoft.DotNet.Tools { public class RestoringCommand : MSBuildForwardingApp { - private bool NoRestore { get; } - - private IEnumerable ParsedArguments { get; } - - private IEnumerable TrailingArguments { get; } - - private IEnumerable ArgsToForwardToRestore() - { - var restoreArguments = ParsedArguments.Where(a => - !a.StartsWith("/p:TargetFramework")); - - if (!restoreArguments.Any(a => a.StartsWith("/verbosity:"))) - { - restoreArguments = restoreArguments.Concat(new string[] { "/verbosity:q" }); - } - - return restoreArguments.Concat(TrailingArguments); - } - - private bool ShouldRunImplicitRestore => !NoRestore; + public RestoreCommand SeparateRestoreCommand { get; } public RestoringCommand( IEnumerable msbuildArgs, @@ -37,18 +19,64 @@ namespace Microsoft.DotNet.Tools IEnumerable trailingArguments, bool noRestore, string msbuildPath = null) - : base(msbuildArgs, msbuildPath) + : base(GetCommandArguments(msbuildArgs, parsedArguments, noRestore), msbuildPath) { - NoRestore = noRestore; - ParsedArguments = parsedArguments; - TrailingArguments = trailingArguments; + SeparateRestoreCommand = GetSeparateRestoreCommand(parsedArguments, trailingArguments, noRestore, msbuildPath); } + private static IEnumerable GetCommandArguments( + IEnumerable msbuildArgs, + IEnumerable parsedArguments, + bool noRestore) + { + if (noRestore) + { + return msbuildArgs; + } + + if (HasArgumentToExcludeFromRestore(parsedArguments)) + { + return Prepend("/nologo", msbuildArgs); + } + + return Prepend("/restore", msbuildArgs); + } + + private static RestoreCommand GetSeparateRestoreCommand( + IEnumerable parsedArguments, + IEnumerable trailingArguments, + bool noRestore, + string msbuildPath) + { + if (noRestore || !HasArgumentToExcludeFromRestore(parsedArguments)) + { + return null; + } + + var restoreArguments = parsedArguments + .Where(a => !IsExcludedFromRestore(a)) + .Concat(trailingArguments); + + return RestoreCommand.FromArgs( + restoreArguments.ToArray(), + msbuildPath, + noLogo: false); + } + + private static IEnumerable Prepend(string argument, IEnumerable arguments) + => new[] { argument }.Concat(arguments); + + private static bool HasArgumentToExcludeFromRestore(IEnumerable arguments) + => arguments.Any(a => IsExcludedFromRestore(a)); + + private static bool IsExcludedFromRestore(string argument) + => argument.StartsWith("/p:TargetFramework=", StringComparison.Ordinal); + public override int Execute() { - if (ShouldRunImplicitRestore) + if (SeparateRestoreCommand != null) { - int exitCode = RestoreCommand.Run(ArgsToForwardToRestore().ToArray()); + int exitCode = SeparateRestoreCommand.Execute(); if (exitCode != 0) { return exitCode; diff --git a/src/dotnet/commands/dotnet-restore/Program.cs b/src/dotnet/commands/dotnet-restore/Program.cs index 6cc324098..b432150c3 100644 --- a/src/dotnet/commands/dotnet-restore/Program.cs +++ b/src/dotnet/commands/dotnet-restore/Program.cs @@ -31,11 +31,14 @@ namespace Microsoft.DotNet.Tools.Restore var parsedRestore = result["dotnet"]["restore"]; - var msbuildArgs = new List + var msbuildArgs = new List(); + + if (noLogo) { - "/NoLogo", - "/t:Restore" - }; + msbuildArgs.Add("/nologo"); + } + + msbuildArgs.Add("/t:Restore"); msbuildArgs.AddRange(parsedRestore.OptionValuesToBeForwarded()); diff --git a/test/dotnet-msbuild.Tests/GivenDotnetBuildInvocation.cs b/test/dotnet-msbuild.Tests/GivenDotnetBuildInvocation.cs index e0742823f..64b7ecda5 100644 --- a/test/dotnet-msbuild.Tests/GivenDotnetBuildInvocation.cs +++ b/test/dotnet-msbuild.Tests/GivenDotnetBuildInvocation.cs @@ -9,7 +9,7 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests { public class GivenDotnetBuildInvocation { - const string ExpectedPrefix = "exec /m /v:m /clp:Summary"; + const string ExpectedPrefix = "exec /m /v:m"; [Theory] [InlineData(new string[] { }, "/t:Build")] @@ -18,8 +18,6 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests [InlineData(new string[] { "--output", "foo" }, "/t:Build /p:OutputPath=foo")] [InlineData(new string[] { "-o", "foo1 foo2" }, "/t:Build \"/p:OutputPath=foo1 foo2\"")] [InlineData(new string[] { "--no-incremental" }, "/t:Rebuild")] - [InlineData(new string[] { "-f", "tfm" }, "/t:Build /p:TargetFramework=tfm")] - [InlineData(new string[] { "--framework", "tfm" }, "/t:Build /p:TargetFramework=tfm")] [InlineData(new string[] { "-r", "rid" }, "/t:Build /p:RuntimeIdentifier=rid")] [InlineData(new string[] { "--runtime", "rid" }, "/t:Build /p:RuntimeIdentifier=rid")] [InlineData(new string[] { "-c", "config" }, "/t:Build /p:Configuration=config")] @@ -28,14 +26,45 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests [InlineData(new string[] { "--no-dependencies" }, "/t:Build /p:BuildProjectReferences=false")] [InlineData(new string[] { "-v", "diag" }, "/t:Build /verbosity:diag")] [InlineData(new string[] { "--verbosity", "diag" }, "/t:Build /verbosity:diag")] - [InlineData(new string[] { "--no-incremental", "-o", "myoutput", "-r", "myruntime", "-v", "diag" }, "/t:Rebuild /p:OutputPath=myoutput /p:RuntimeIdentifier=myruntime /verbosity:diag")] + [InlineData(new string[] { "--no-incremental", "-o", "myoutput", "-r", "myruntime", "-v", "diag", "/ArbitrarySwitchForMSBuild" }, + "/t:Rebuild /p:OutputPath=myoutput /p:RuntimeIdentifier=myruntime /verbosity:diag /ArbitrarySwitchForMSBuild")] public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalArgs) { expectedAdditionalArgs = (string.IsNullOrEmpty(expectedAdditionalArgs) ? "" : $" {expectedAdditionalArgs}"); var msbuildPath = ""; - BuildCommand.FromArgs(args, msbuildPath) - .GetProcessStartInfo().Arguments.Should().Be($"{ExpectedPrefix}{expectedAdditionalArgs}"); + var command = BuildCommand.FromArgs(args, msbuildPath); + + command.SeparateRestoreCommand.Should().BeNull(); + + command.GetProcessStartInfo() + .Arguments.Should() + .Be($"{ExpectedPrefix} /restore /clp:Summary{expectedAdditionalArgs}"); + } + + [Theory] + [InlineData(new string[] { "-f", "tfm" }, "/t:Restore", "/t:Build /p:TargetFramework=tfm")] + [InlineData(new string[] { "-o", "myoutput", "-f", "tfm", "-v", "diag", "/ArbitrarySwitchForMSBuild" }, + "/t:Restore /p:OutputPath=myoutput /verbosity:diag /ArbitrarySwitchForMSBuild", + "/t:Build /p:OutputPath=myoutput /p:TargetFramework=tfm /verbosity:diag /ArbitrarySwitchForMSBuild")] + public void MsbuildInvocationIsCorrectForSeparateRestore( + string[] args, + string expectedAdditionalArgsForRestore, + string expectedAdditionalArgs) + { + expectedAdditionalArgs = (string.IsNullOrEmpty(expectedAdditionalArgs) ? "" : $" {expectedAdditionalArgs}"); + + var msbuildPath = ""; + var command = BuildCommand.FromArgs(args, msbuildPath); + + command.SeparateRestoreCommand.GetProcessStartInfo() + .Arguments.Should() + .Be($"{ExpectedPrefix} {expectedAdditionalArgsForRestore}"); + + command.GetProcessStartInfo() + .Arguments.Should() + .Be($"{ExpectedPrefix} /nologo /clp:Summary{expectedAdditionalArgs}"); + } } } diff --git a/test/dotnet-msbuild.Tests/GivenDotnetPackInvocation.cs b/test/dotnet-msbuild.Tests/GivenDotnetPackInvocation.cs index aba1b15a6..74559a6fa 100644 --- a/test/dotnet-msbuild.Tests/GivenDotnetPackInvocation.cs +++ b/test/dotnet-msbuild.Tests/GivenDotnetPackInvocation.cs @@ -11,7 +11,7 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests { public class GivenDotnetPackInvocation { - const string ExpectedPrefix = "exec /m /v:m /t:pack"; + const string ExpectedPrefix = "exec /m /v:m /restore /t:pack"; [Theory] [InlineData(new string[] { }, "")] @@ -33,8 +33,10 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests expectedAdditionalArgs = (string.IsNullOrEmpty(expectedAdditionalArgs) ? "" : $" {expectedAdditionalArgs}"); var msbuildPath = ""; - PackCommand.FromArgs(args, msbuildPath) - .GetProcessStartInfo().Arguments.Should().Be($"{ExpectedPrefix}{expectedAdditionalArgs}"); + var command = PackCommand.FromArgs(args, msbuildPath); + + command.SeparateRestoreCommand.Should().BeNull(); + command.GetProcessStartInfo().Arguments.Should().Be($"{ExpectedPrefix}{expectedAdditionalArgs}"); } } } diff --git a/test/dotnet-msbuild.Tests/GivenDotnetPublishInvocation.cs b/test/dotnet-msbuild.Tests/GivenDotnetPublishInvocation.cs index f7cf3838d..4ece567bc 100644 --- a/test/dotnet-msbuild.Tests/GivenDotnetPublishInvocation.cs +++ b/test/dotnet-msbuild.Tests/GivenDotnetPublishInvocation.cs @@ -20,12 +20,10 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests this.output = output; } - const string ExpectedPrefix = "exec /m /v:m /t:Publish"; + const string ExpectedPrefix = "exec /m /v:m"; [Theory] [InlineData(new string[] { }, "")] - [InlineData(new string[] { "-f", "" }, "/p:TargetFramework=")] - [InlineData(new string[] { "--framework", "" }, "/p:TargetFramework=")] [InlineData(new string[] { "-r", "" }, "/p:RuntimeIdentifier=")] [InlineData(new string[] { "--runtime", "" }, "/p:RuntimeIdentifier=")] [InlineData(new string[] { "-o", "" }, "/p:PublishDir=")] @@ -43,10 +41,35 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests expectedAdditionalArgs = (string.IsNullOrEmpty(expectedAdditionalArgs) ? "" : $" {expectedAdditionalArgs}"); var msbuildPath = ""; - PublishCommand.FromArgs(args, msbuildPath) - .GetProcessStartInfo() - .Arguments.Should() - .Be($"{ExpectedPrefix}{expectedAdditionalArgs}"); + var command = PublishCommand.FromArgs(args, msbuildPath); + + command.SeparateRestoreCommand + .Should() + .BeNull(); + + command.GetProcessStartInfo() + .Arguments.Should() + .Be($"{ExpectedPrefix} /restore /t:Publish{expectedAdditionalArgs}"); + } + + [Theory] + [InlineData(new string[] { "-f", "" }, "/p:TargetFramework=")] + [InlineData(new string[] { "--framework", "" }, "/p:TargetFramework=")] + public void MsbuildInvocationIsCorrectForSeparateRestore(string[] args, string expectedAdditionalArgs) + { + expectedAdditionalArgs = (string.IsNullOrEmpty(expectedAdditionalArgs) ? "" : $" {expectedAdditionalArgs}"); + + var msbuildPath = ""; + var command = PublishCommand.FromArgs(args, msbuildPath); + + command.SeparateRestoreCommand + .GetProcessStartInfo() + .Arguments.Should() + .Be($"{ExpectedPrefix} /t:Restore"); + + command.GetProcessStartInfo() + .Arguments.Should() + .Be($"{ExpectedPrefix} /nologo /t:Publish{expectedAdditionalArgs}"); } [Theory] diff --git a/test/dotnet-msbuild.Tests/GivenDotnetRestoreInvocation.cs b/test/dotnet-msbuild.Tests/GivenDotnetRestoreInvocation.cs index 1c1802b2d..eb8ea6163 100644 --- a/test/dotnet-msbuild.Tests/GivenDotnetRestoreInvocation.cs +++ b/test/dotnet-msbuild.Tests/GivenDotnetRestoreInvocation.cs @@ -11,7 +11,7 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests public class GivenDotnetRestoreInvocation { private const string ExpectedPrefix = - "exec /m /v:m /NoLogo /t:Restore"; + "exec /m /v:m /nologo /t:Restore"; [Theory] [InlineData(new string[] { }, "")]