Skip to content

Commit 480b58f

Browse files
baronfeljjonescz
baronfel
and
jjonescz
authored
Improve MSBuild argument parsing and tracking, and use that to set RestoreProperties that optimize Restore for large projects (#49526)
Co-authored-by: Jan Jones <[email protected]>
1 parent ea86351 commit 480b58f

File tree

78 files changed

+1384
-813
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

78 files changed

+1384
-813
lines changed

‎src/BuiltInTools/dotnet-watch/CommandLine/CommandLineOptions.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ private static IReadOnlyList<string> GetCommandArguments(
195195
// Some options _may_ be computed or have defaults, so not all may have an IdentifierToken.
196196
// For those that do not, use the Option's Name instead.
197197
var optionNameToForward = optionResult.IdentifierToken?.Value ?? optionResult.Option.Name;
198-
if (optionResult.Tokens.Count == 0)
198+
if (optionResult.Tokens.Count == 0 && !optionResult.Implicit)
199199
{
200200
arguments.Add(optionNameToForward);
201201
}

‎src/Cli/Microsoft.DotNet.Cli.Utils/Constants.cs

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,12 @@ public static class Constants
1717
public static readonly string ObjDirectoryName = "obj";
1818
public static readonly string GitDirectoryName = ".git";
1919

20-
public static readonly string MSBUILD_EXE_PATH = "MSBUILD_EXE_PATH";
21-
public static readonly string MSBuildExtensionsPath = "MSBuildExtensionsPath";
22-
public static readonly string EnableDefaultItems = "EnableDefaultItems";
20+
public static readonly string MSBUILD_EXE_PATH = nameof(MSBUILD_EXE_PATH);
21+
public static readonly string MSBuildExtensionsPath = nameof(MSBuildExtensionsPath);
22+
public static readonly string EnableDefaultItems = nameof(EnableDefaultItems);
23+
public static readonly string EnableDefaultCompileItems = nameof(EnableDefaultCompileItems);
24+
public static readonly string EnableDefaultEmbeddedResourceItems = nameof(EnableDefaultEmbeddedResourceItems);
25+
public static readonly string EnableDefaultNoneItems = nameof(EnableDefaultNoneItems);
2326

2427
public static readonly string ProjectArgumentName = "<PROJECT>";
2528
public static readonly string SolutionArgumentName = "<SLN_FILE>";

‎src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildArgs.cs

Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System.Collections.ObjectModel;
5+
using System.CommandLine;
6+
using System.CommandLine.Parsing;
7+
8+
namespace Microsoft.DotNet.Cli.Utils;
9+
10+
/// <summary>
11+
/// Represents all of the parsed and forwarded arguments that the SDK should pass to MSBuild.
12+
/// </summary>
13+
public sealed class MSBuildArgs
14+
{
15+
private MSBuildArgs(ReadOnlyDictionary<string, string>? properties, ReadOnlyDictionary<string, string>? restoreProperties, string[]? targets, string[]? otherMSBuildArgs)
16+
{
17+
GlobalProperties = properties;
18+
RestoreGlobalProperties = restoreProperties;
19+
RequestedTargets = targets;
20+
OtherMSBuildArgs = otherMSBuildArgs is not null
21+
? [.. otherMSBuildArgs]
22+
: new List<string>();
23+
}
24+
25+
/// <summary>
26+
/// The set of <c>-p</c> flags that should be passed to MSBuild.
27+
/// </summary>
28+
public ReadOnlyDictionary<string, string>? GlobalProperties { get; }
29+
/// <summary>
30+
/// The set of <c>-rp</c> flags that should be passed to MSBuild for restore operations only.
31+
/// If this is non-empty, all <see cref="GlobalProperties"/> flags should be passed as <c>-rp</c> as well.
32+
/// </summary>
33+
public ReadOnlyDictionary<string, string>? RestoreGlobalProperties { get; private set; }
34+
35+
/// <summary>
36+
/// The ordered list of targets that should be passed to MSBuild.
37+
/// </summary>
38+
public string[]? RequestedTargets { get; }
39+
40+
/// <summary>
41+
/// All non <c>-p</c> and <c>-rp</c> arguments that should be passed to MSBuild.
42+
/// </summary>
43+
public List<string> OtherMSBuildArgs { get; }
44+
45+
/// <summary>
46+
/// Takes all of the unstructured properties and arguments that have been accrued from the command line
47+
/// processing of the SDK and returns a structured set of MSBuild arguments grouped by purpose.
48+
/// </summary>
49+
/// <param name="forwardedAndUserFacingArgs">the complete set of forwarded MSBuild arguments and un-parsed, potentially MSBuild-relevant arguments</param>
50+
/// <returns></returns>
51+
public static MSBuildArgs AnalyzeMSBuildArguments(IEnumerable<string> forwardedAndUserFacingArgs, params Option[] options)
52+
{
53+
var fakeCommand = new System.CommandLine.Command("dotnet");
54+
foreach (var option in options)
55+
{
56+
fakeCommand.Options.Add(option);
57+
}
58+
59+
var propertyParsingConfiguration = new CommandLineConfiguration(fakeCommand)
60+
{
61+
EnablePosixBundling = false
62+
};
63+
var parseResult = propertyParsingConfiguration.Parse([..forwardedAndUserFacingArgs]);
64+
var globalProperties = parseResult.GetResult("--property") is OptionResult propResult ? propResult.GetValueOrDefault<ReadOnlyDictionary<string, string>?>() : null;
65+
var restoreProperties = parseResult.GetResult("--restoreProperty") is OptionResult restoreResult ? restoreResult.GetValueOrDefault<ReadOnlyDictionary<string, string>?>() : null;
66+
var requestedTargets = parseResult.GetResult("--target") is OptionResult targetResult ? targetResult.GetValueOrDefault<string[]?>() : null;
67+
var otherMSBuildArgs = parseResult.UnmatchedTokens.ToArray();
68+
return new MSBuildArgs(
69+
properties: globalProperties,
70+
restoreProperties: restoreProperties,
71+
targets: requestedTargets,
72+
otherMSBuildArgs: otherMSBuildArgs);
73+
}
74+
75+
76+
public static MSBuildArgs FromProperties(ReadOnlyDictionary<string, string>? properties)
77+
{
78+
return new MSBuildArgs(properties, null, null, null);
79+
}
80+
81+
public static MSBuildArgs FromOtherArgs(params ReadOnlySpan<string> args)
82+
{
83+
return new MSBuildArgs(null, null, null, args.ToArray());
84+
}
85+
86+
public static readonly MSBuildArgs ForHelp = new(null, null, null, ["--help"]);
87+
88+
/// <summary>
89+
/// Completely replaces the MSBuild arguments with the provided <paramref name="newArgs"/>.
90+
/// </summary>
91+
public MSBuildArgs CloneWithExplicitArgs(string[] newArgs)
92+
{
93+
return new MSBuildArgs(
94+
properties: GlobalProperties,
95+
restoreProperties: RestoreGlobalProperties,
96+
targets: RequestedTargets,
97+
otherMSBuildArgs: newArgs);
98+
}
99+
100+
/// <summary>
101+
/// Adds new arbitrary MSBuild flags to the end of the existing MSBuild arguments.
102+
/// </summary>
103+
public MSBuildArgs CloneWithAdditionalArgs(params string[] additionalArgs)
104+
{
105+
if (additionalArgs is null || additionalArgs.Length == 0)
106+
{
107+
// If there are no additional args, we can just return the current instance.
108+
return new MSBuildArgs(GlobalProperties, RestoreGlobalProperties, RequestedTargets, OtherMSBuildArgs.ToArray());
109+
}
110+
111+
return new MSBuildArgs(GlobalProperties, RestoreGlobalProperties, RequestedTargets, [.. OtherMSBuildArgs, .. additionalArgs]);
112+
}
113+
114+
public MSBuildArgs CloneWithAdditionalRestoreProperties(ReadOnlyDictionary<string, string>? additionalRestoreProperties)
115+
{
116+
if (additionalRestoreProperties is null || additionalRestoreProperties.Count == 0)
117+
{
118+
// If there are no additional restore properties, we can just return the current instance.
119+
return new MSBuildArgs(GlobalProperties, RestoreGlobalProperties, RequestedTargets, OtherMSBuildArgs.ToArray());
120+
}
121+
if (RestoreGlobalProperties is null)
122+
{
123+
return new MSBuildArgs(GlobalProperties, additionalRestoreProperties, RequestedTargets, OtherMSBuildArgs.ToArray());
124+
}
125+
126+
var newRestoreProperties = new Dictionary<string, string>(RestoreGlobalProperties, StringComparer.OrdinalIgnoreCase);
127+
foreach (var kvp in additionalRestoreProperties)
128+
{
129+
newRestoreProperties[kvp.Key] = kvp.Value;
130+
}
131+
return new MSBuildArgs(GlobalProperties, new(newRestoreProperties), RequestedTargets, OtherMSBuildArgs.ToArray());
132+
}
133+
134+
public MSBuildArgs CloneWithAdditionalProperties(ReadOnlyDictionary<string, string>? additionalProperties)
135+
{
136+
if (additionalProperties is null || additionalProperties.Count == 0)
137+
{
138+
// If there are no additional properties, we can just return the current instance.
139+
return new MSBuildArgs(GlobalProperties, RestoreGlobalProperties, RequestedTargets, OtherMSBuildArgs.ToArray());
140+
}
141+
if (GlobalProperties is null)
142+
{
143+
return new MSBuildArgs(additionalProperties, RestoreGlobalProperties, RequestedTargets, OtherMSBuildArgs.ToArray());
144+
}
145+
146+
var newProperties = new Dictionary<string, string>(GlobalProperties, StringComparer.OrdinalIgnoreCase);
147+
foreach (var kvp in additionalProperties)
148+
{
149+
newProperties[kvp.Key] = kvp.Value;
150+
}
151+
return new MSBuildArgs(new(newProperties), RestoreGlobalProperties, RequestedTargets, OtherMSBuildArgs.ToArray());
152+
}
153+
154+
public MSBuildArgs CloneWithAdditionalTarget(string additionalTarget)
155+
{
156+
string[] newTargets = RequestedTargets is not null
157+
? [.. RequestedTargets, additionalTarget]
158+
: [ additionalTarget ];
159+
return new MSBuildArgs(GlobalProperties, RestoreGlobalProperties, newTargets, OtherMSBuildArgs.ToArray());
160+
}
161+
162+
public void ApplyPropertiesToRestore()
163+
{
164+
if (RestoreGlobalProperties is null)
165+
{
166+
RestoreGlobalProperties = GlobalProperties;
167+
return;
168+
}
169+
else if (GlobalProperties is not null && GlobalProperties.Count > 0)
170+
{
171+
// If we have restore properties, we need to merge the global properties into them.
172+
// We ensure the restore properties overwrite the properties to align with expected MSBuild semantics.
173+
var newdict = new Dictionary<string, string>(GlobalProperties, StringComparer.OrdinalIgnoreCase);
174+
foreach (var restoreKvp in RestoreGlobalProperties)
175+
{
176+
newdict[restoreKvp.Key] = restoreKvp.Value;
177+
}
178+
RestoreGlobalProperties = new(newdict);
179+
}
180+
}
181+
}

‎src/Cli/Microsoft.DotNet.Cli.Utils/MSBuildForwardingAppWithoutLogging.cs

Lines changed: 39 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
using System.Diagnostics;
77
using Microsoft.DotNet.Cli.Utils.Extensions;
8+
using Microsoft.DotNet.Cli;
89

910
namespace Microsoft.DotNet.Cli.Utils;
1011

@@ -25,11 +26,13 @@ public static string MSBuildVersion
2526
// Null if we're running MSBuild in-proc.
2627
private ForwardingAppImplementation? _forwardingApp;
2728

28-
// Command line arguments we're asked to forward to MSBuild.
29-
private readonly IEnumerable<string> _argsToForward;
30-
3129
internal static string? MSBuildExtensionsPathTestHook = null;
3230

31+
/// <summary>
32+
/// Structure describing the parsed and forwarded MSBuild arguments for this command.
33+
/// </summary>
34+
private MSBuildArgs _msbuildArgs;
35+
3336
// Path to the MSBuild binary to use.
3437
public string MSBuildPath { get; }
3538

@@ -40,11 +43,17 @@ public static string MSBuildVersion
4043

4144
private readonly List<string> _msbuildRequiredParameters = [ "-maxcpucount", "-verbosity:m" ];
4245

43-
public MSBuildForwardingAppWithoutLogging(IEnumerable<string> argsToForward, string? msbuildPath = null, bool includeLogo = false)
46+
public MSBuildForwardingAppWithoutLogging(MSBuildArgs msbuildArgs, string? msbuildPath = null, bool includeLogo = false, bool isRestoring = true)
4447
{
4548
string defaultMSBuildPath = GetMSBuildExePath();
46-
47-
_argsToForward = includeLogo ? argsToForward : ["-nologo", ..argsToForward];
49+
_msbuildArgs = msbuildArgs;
50+
if (!includeLogo && !msbuildArgs.OtherMSBuildArgs.Contains("-nologo", StringComparer.OrdinalIgnoreCase))
51+
{
52+
// If the user didn't explicitly ask for -nologo, we add it to avoid the MSBuild logo.
53+
// This is useful for scenarios like restore where we don't want to print the logo.
54+
// Note that this is different from the default behavior of MSBuild, which prints the logo.
55+
msbuildArgs.OtherMSBuildArgs.Add("-nologo");
56+
}
4857
string? tlpDefault = TerminalLoggerDefault;
4958
// new for .NET 9 - default TL to auto (aka enable in non-CI scenarios)
5059
if (string.IsNullOrWhiteSpace(tlpDefault))
@@ -84,7 +93,22 @@ public ProcessStartInfo GetProcessStartInfo()
8493

8594
public string[] GetAllArguments()
8695
{
87-
return _msbuildRequiredParameters.Concat(_argsToForward.Select(Escape)).ToArray();
96+
return [.. _msbuildRequiredParameters, ..EmitMSBuildArgs(_msbuildArgs) ];
97+
}
98+
99+
private string[] EmitMSBuildArgs(MSBuildArgs msbuildArgs) => [
100+
.. msbuildArgs.GlobalProperties?.Select(kvp => EmitProperty(kvp)) ?? [],
101+
.. msbuildArgs.RestoreGlobalProperties?.Select(kvp => EmitProperty(kvp, "restoreProperty")) ?? [],
102+
.. msbuildArgs.RequestedTargets?.Select(target => $"--target:{target}") ?? [],
103+
.. msbuildArgs.OtherMSBuildArgs
104+
];
105+
106+
private static string EmitProperty(KeyValuePair<string, string> property, string label = "property")
107+
{
108+
// Escape RestoreSources to avoid issues with semicolons in the value.
109+
return IsRestoreSources(property.Key)
110+
? $"--{label}:{property.Key}={Escape(property.Value)}"
111+
: $"--{label}:{property.Key}={property.Value}";
88112
}
89113

90114
public void EnvironmentVariable(string name, string value)
@@ -160,9 +184,12 @@ public int ExecuteInProc(string[] arguments)
160184
}
161185
}
162186

163-
private static string Escape(string arg) =>
164-
// this is a workaround for https://github.com/Microsoft/msbuild/issues/1622
165-
IsRestoreSources(arg) ? arg.Replace(";", "%3B").Replace("://", ":%2F%2F") : arg;
187+
/// <summary>
188+
/// This is a workaround for https://github.com/Microsoft/msbuild/issues/1622.
189+
/// Only used historically for RestoreSources property only.
190+
/// </summary>
191+
private static string Escape(string propertyValue) =>
192+
propertyValue.Replace(";", "%3B").Replace("://", ":%2F%2F");
166193

167194
private static string GetMSBuildExePath()
168195
{
@@ -194,19 +221,13 @@ internal static Dictionary<string, string> GetMSBuildRequiredEnvironmentVariable
194221
{
195222
return new()
196223
{
197-
{ "MSBuildExtensionsPath", MSBuildExtensionsPathTestHook ?? AppContext.BaseDirectory },
224+
{ "MSBuildExtensionsPath", MSBuildExtensionsPathTestHook ?? Environment.GetEnvironmentVariable("MSBuildExtensionsPath") ?? AppContext.BaseDirectory },
198225
{ "MSBuildSDKsPath", GetMSBuildSDKsPath() },
199226
{ "DOTNET_HOST_PATH", GetDotnetPath() },
200227
};
201228
}
202229

203-
private static bool IsRestoreSources(string arg)
204-
{
205-
return arg.StartsWith("/p:RestoreSources=", StringComparison.OrdinalIgnoreCase) ||
206-
arg.StartsWith("/property:RestoreSources=", StringComparison.OrdinalIgnoreCase) ||
207-
arg.StartsWith("-p:RestoreSources=", StringComparison.OrdinalIgnoreCase) ||
208-
arg.StartsWith("-property:RestoreSources=", StringComparison.OrdinalIgnoreCase);
209-
}
230+
private static bool IsRestoreSources(string arg) => arg.Equals("RestoreSources", StringComparison.OrdinalIgnoreCase);
210231
}
211232

212233
#endif

‎src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,11 @@
2222
<EmbeddedResource Update="**\*.resx" GenerateSource="true" />
2323
</ItemGroup>
2424

25+
<!-- Versions that have to be pinned because they are locked to VS/MSBuild compatibility -->
26+
<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'">
27+
<PackageReference Include="System.Collections.Immutable" VersionOverride="$(SystemCollectionsImmutableToolsetPackageVersion)" />
28+
</ItemGroup>
29+
2530
<Target Name="VerifyMSBuildDependency" BeforeTargets="ResolveAssemblyReferences" Condition="'$([MSBuild]::GetTargetFrameworkIdentifier($(TargetFramework)))' == '.NETCoreApp'">
2631
<!-- We explicitly reference an older version of MSBuild here to support VS
2732
for Mac and other VS scenarios. During source-build, we only have access to

‎src/Cli/dotnet/CliSchema.cs

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -179,20 +179,31 @@ private static RootCommandDetails CreateRootCommandDetails(Command command)
179179
option.HelpName,
180180
option.ValueType.ToCliTypeString(),
181181
option.HasDefaultValue,
182-
option.HasDefaultValue ? option.GetDefaultValue() : null,
182+
option.HasDefaultValue ? HumanizeValue(option.GetDefaultValue()) : null,
183183
CreateArityDetails(option.Arity),
184184
option.Required,
185185
option.Recursive
186186
);
187187

188+
/// <summary>
189+
/// Maps some types that don't serialize well to more human-readable strings.
190+
/// For example, <see cref="VerbosityOptions"/> is serialized as a string instead of an integer.
191+
/// </summary>
192+
private static object? HumanizeValue(object? v) => v switch
193+
{
194+
VerbosityOptions o => Enum.GetName(o),
195+
null => null,
196+
_ => v // For other types, return as is
197+
};
198+
188199
private static ArgumentDetails CreateArgumentDetails(int index, Argument argument) => new ArgumentDetails(
189200
argument.Description?.ReplaceLineEndings("\n"),
190201
index,
191202
argument.Hidden,
192203
argument.HelpName,
193204
argument.ValueType.ToCliTypeString(),
194205
argument.HasDefaultValue,
195-
argument.HasDefaultValue ? argument.GetDefaultValue() : null,
206+
argument.HasDefaultValue ? HumanizeValue(argument.GetDefaultValue()) : null,
196207
CreateArityDetails(argument.Arity)
197208
);
198209

0 commit comments

Comments
 (0)