Skip to content

Simplify binary logger implementation by using BinaryLoggerParameters directly throughout codebase #50063

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
39 changes: 5 additions & 34 deletions src/BuiltInTools/dotnet-watch/CommandLine/CommandLineOptions.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Immutable;
using System.CommandLine;
using System.CommandLine.Parsing;
using System.Data;
Expand All @@ -17,8 +16,6 @@ internal sealed class CommandLineOptions
{
public const string DefaultCommand = "run";

private static readonly ImmutableArray<string> s_binaryLogOptionNames = ["-bl", "/bl", "-binaryLogger", "--binaryLogger", "/binaryLogger"];

public bool List { get; init; }
public required GlobalOptions GlobalOptions { get; init; }

Expand Down Expand Up @@ -157,17 +154,14 @@ internal sealed class CommandLineOptions
}
}

var commandArguments = GetCommandArguments(parseResult, watchOptions, explicitCommand, out var binLogToken, out var binLogPath);
var commandArguments = GetCommandArguments(parseResult, watchOptions, explicitCommand);

// We assume that forwarded options, if any, are intended for dotnet build.
var buildArguments = buildOptions.Select(option => ((IForwardedOption)option).GetForwardingFunction()(parseResult)).SelectMany(args => args).ToList();

if (binLogToken != null)
{
buildArguments.Add(binLogToken);
}

var targetFrameworkOption = (Option<string>?)buildOptions.SingleOrDefault(option => option.Name == "--framework");
var binaryLoggerOption = (Option<string?>?)buildOptions.SingleOrDefault(option => option.Name == "--binaryLogger");
var binaryLoggerPath = binaryLoggerOption != null ? parseResult.GetValue(binaryLoggerOption) : null;

return new()
{
Expand All @@ -178,7 +172,7 @@ internal sealed class CommandLineOptions
NoHotReload = parseResult.GetValue(noHotReloadOption),
NonInteractive = parseResult.GetValue(NonInteractiveOption),
Verbose = parseResult.GetValue(verboseOption),
BinaryLogPath = ParseBinaryLogFilePath(binLogPath),
BinaryLogPath = ParseBinaryLogFilePath(binaryLoggerPath),
},

CommandArguments = commandArguments,
Expand Down Expand Up @@ -215,13 +209,9 @@ internal sealed class CommandLineOptions
private static IReadOnlyList<string> GetCommandArguments(
ParseResult parseResult,
IReadOnlyList<Option> watchOptions,
Command? explicitCommand,
out string? binLogToken,
out string? binLogPath)
Command? explicitCommand)
{
var arguments = new List<string>();
binLogToken = null;
binLogPath = null;

foreach (var child in parseResult.CommandResult.Children)
{
Expand Down Expand Up @@ -276,25 +266,6 @@ private static IReadOnlyList<string> GetCommandArguments(
seenCommand = true;
continue;
}

// Workaround: commands do not have forwarding option for -bl
// https://github.com/dotnet/sdk/issues/49989
foreach (var name in s_binaryLogOptionNames)
{
if (token.StartsWith(name, StringComparison.OrdinalIgnoreCase))
{
if (token.Length == name.Length)
{
binLogToken = token;
binLogPath = "";
}
else if (token.Length > name.Length + 1 && token[name.Length] == ':')
{
binLogToken = token;
binLogPath = token[(name.Length + 1)..];
}
}
}
}

if (!dashDashInserted && i >= unmatchedTokensBeforeDashDash)
Expand Down
1 change: 1 addition & 0 deletions src/Cli/dotnet/Commands/Build/BuildCommandParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ private static Command ConstructCommand()
command.Options.Add(CommonOptions.ArchitectureOption);
command.Options.Add(CommonOptions.OperatingSystemOption);
command.Options.Add(CommonOptions.DisableBuildServersOption);
command.Options.Add(CommonOptions.BinaryLoggerOption);
command.Options.Add(TargetOption);

command.SetAction(BuildCommand.Run);
Expand Down
1 change: 1 addition & 0 deletions src/Cli/dotnet/Commands/Clean/CleanCommandParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ private static Command ConstructCommand()
command.Options.Add(CommonOptions.ArtifactsPathOption);
command.Options.Add(NoLogoOption);
command.Options.Add(CommonOptions.DisableBuildServersOption);
command.Options.Add(CommonOptions.BinaryLoggerOption);
command.Options.Add(TargetOption);
command.Subcommands.Add(CleanFileBasedAppArtifactsCommandParser.Command);

Expand Down
2 changes: 1 addition & 1 deletion src/Cli/dotnet/Commands/MSBuild/MSBuildForwardingApp.cs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ private static MSBuildArgs ConcatTelemetryLogger(MSBuildArgs msbuildArgs)
/// Mostly intended for quick/one-shot usage - most 'core' SDK commands should do more hands-on parsing.
/// </summary>
public MSBuildForwardingApp(IEnumerable<string> rawMSBuildArgs, string? msbuildPath = null) : this(
MSBuildArgs.AnalyzeMSBuildArguments(rawMSBuildArgs.ToArray(), CommonOptions.PropertiesOption, CommonOptions.RestorePropertiesOption, CommonOptions.MSBuildTargetOption(), CommonOptions.VerbosityOption()),
MSBuildArgs.AnalyzeMSBuildArguments(rawMSBuildArgs.ToArray(), CommonOptions.PropertiesOption, CommonOptions.RestorePropertiesOption, CommonOptions.MSBuildTargetOption(), CommonOptions.VerbosityOption(), CommonOptions.BinaryLoggerOption),
msbuildPath)
{
}
Expand Down
1 change: 1 addition & 0 deletions src/Cli/dotnet/Commands/Pack/PackCommandParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ private static Command ConstructCommand()
command.Options.Add(CommonOptions.VersionSuffixOption);
command.Options.Add(ConfigurationOption);
command.Options.Add(CommonOptions.DisableBuildServersOption);
command.Options.Add(CommonOptions.BinaryLoggerOption);
command.Options.Add(TargetOption);

// Don't include runtime option because we want to include it specifically and allow the short version ("-r") to be used
Expand Down
1 change: 1 addition & 0 deletions src/Cli/dotnet/Commands/Publish/PublishCommandParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ private static Command ConstructCommand()
command.Options.Add(CommonOptions.ArchitectureOption);
command.Options.Add(CommonOptions.OperatingSystemOption);
command.Options.Add(CommonOptions.DisableBuildServersOption);
command.Options.Add(CommonOptions.BinaryLoggerOption);
command.Options.Add(TargetOption);

command.SetAction(PublishCommand.Run);
Expand Down
1 change: 1 addition & 0 deletions src/Cli/dotnet/Commands/Restore/RestoreCommandParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ private static Command ConstructCommand()

command.Arguments.Add(SlnOrProjectOrFileArgument);
command.Options.Add(CommonOptions.DisableBuildServersOption);
command.Options.Add(CommonOptions.BinaryLoggerOption);

command.Options.AddRange(FullRestoreOptions());

Expand Down
1 change: 1 addition & 0 deletions src/Cli/dotnet/Commands/Run/RunCommandParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ private static Command ConstructCommand()
command.Options.Add(CommonOptions.ArchitectureOption);
command.Options.Add(CommonOptions.OperatingSystemOption);
command.Options.Add(CommonOptions.DisableBuildServersOption);
command.Options.Add(CommonOptions.BinaryLoggerOption);
command.Options.Add(CommonOptions.ArtifactsPathOption);
command.Options.Add(CommonOptions.EnvOption);

Expand Down
1 change: 1 addition & 0 deletions src/Cli/dotnet/Commands/Store/StoreCommandParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ private static Command ConstructCommand()
command.Options.Add(CommonOptions.VerbosityOption());
command.Options.Add(CommonOptions.CurrentRuntimeOption(CliCommandStrings.CurrentRuntimeOptionDescription));
command.Options.Add(CommonOptions.DisableBuildServersOption);
command.Options.Add(CommonOptions.BinaryLoggerOption);

command.SetAction(StoreCommand.Run);

Expand Down
1 change: 1 addition & 0 deletions src/Cli/dotnet/Commands/Test/TestCommandParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@ private static Command GetVSTestCliCommand()
command.Options.Add(CommonOptions.ArchitectureOption);
command.Options.Add(CommonOptions.OperatingSystemOption);
command.Options.Add(CommonOptions.DisableBuildServersOption);
command.Options.Add(CommonOptions.BinaryLoggerOption);
command.Options.Add(VsTestTargetOption);
command.SetAction(TestCommand.Run);

Expand Down
48 changes: 48 additions & 0 deletions src/Cli/dotnet/CommonOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,54 @@ internal static class CommonOptions
.ForwardAsMSBuildProperty()
.AllowSingleArgPerToken();

public static Option<string?> BinaryLoggerOption = CreateBinaryLoggerOption();

private static ForwardedOption<string?> CreateBinaryLoggerOption()
{
var option = new ForwardedOption<string?>("--binaryLogger", "-binaryLogger", "/binaryLogger", "-bl", "--bl", "/bl")
{
Description = "Log all build output to a binary log file. Optionally specify a file path and optional parameters.",
HelpName = "PATH",
Arity = ArgumentArity.ZeroOrOne,
Hidden = true
};

option.AllowSingleArgPerToken();

// Set the forwarding function directly to bypass the null value check
Func<ParseResult, IEnumerable<string>> forwardingFunc = (ParseResult parseResult) =>
{
// Find the option result for the binary logger
var optionResult = parseResult.GetResult(option);
if (optionResult != null)
{
// Get the exact token that was used (e.g., "-bl", "/bl", "--binaryLogger", etc.)
var originalToken = optionResult.IdentifierToken?.Value ?? option.Name;

// Check if there are any argument tokens (the value after :)
if (optionResult.Tokens.Count > 0)
{
// If there is a value, forward it with the parameter
var argumentValue = optionResult.Tokens[0].Value;
return [$"{originalToken}:{argumentValue}"];
}
else
{
// If no value is provided, forward just the flag
return [originalToken];
}
}

return [];
};

// Use reflection to set the private ForwardingFunction field
var forwardingFunctionField = typeof(ForwardedOption<string?>).GetField("ForwardingFunction", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
forwardingFunctionField?.SetValue(option, forwardingFunc);

return option;
}

private static ReadOnlyDictionary<string, string>? ParseMSBuildTokensIntoDictionary(ArgumentResult result)
{
if (result.Tokens.Count == 0)
Expand Down
Loading