Simplify binary logger implementation by using BinaryLoggerParameters directly throughout codebase

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

@@ -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;
@@ -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; }
 
@@ -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<BinaryLoggerOptions?>?)buildOptions.SingleOrDefault(option => option.Name == "--binaryLogger");
+        var binaryLoggerOptions = binaryLoggerOption != null ? parseResult.GetValue(binaryLoggerOption) : null;
 
         return new()
         {
@@ -178,7 +172,7 @@ internal sealed class CommandLineOptions
                 NoHotReload = parseResult.GetValue(noHotReloadOption),
                 NonInteractive = parseResult.GetValue(NonInteractiveOption),
                 Verbose = parseResult.GetValue(verboseOption),
-                BinaryLogPath = ParseBinaryLogFilePath(binLogPath),
+                BinaryLogPath = GetBinaryLogFilePath(binaryLoggerOptions),
             },
 
             CommandArguments = commandArguments,
@@ -193,35 +187,32 @@ internal sealed class CommandLineOptions
     }
 
     /// <summary>
-    /// Parses the value of msbuild option `-binaryLogger[:[LogFile=]output.binlog[;ProjectImports={None,Embed,ZipFile}]]`.
-    /// Emulates https://github.com/dotnet/msbuild/blob/7f69ea906c29f2478cc05423484ad185de66e124/src/Build/Logging/BinaryLogger/BinaryLogger.cs#L481.
-    /// See https://github.com/dotnet/msbuild/issues/12256
+    /// Gets the binary log file path from the binary logger options.
     /// </summary>
-    internal static string? ParseBinaryLogFilePath(string? value)
-        => value switch
+    internal static string? GetBinaryLogFilePath(BinaryLoggerOptions? options)
+    {
+        if (options == null)
         {
-            null => null,
-            _ => (from parameter in value.Split(';', StringSplitOptions.RemoveEmptyEntries)
-                  where !string.Equals(parameter, "ProjectImports=None", StringComparison.OrdinalIgnoreCase) &&
-                        !string.Equals(parameter, "ProjectImports=Embed", StringComparison.OrdinalIgnoreCase) &&
-                        !string.Equals(parameter, "ProjectImports=ZipFile", StringComparison.OrdinalIgnoreCase) &&
-                        !string.Equals(parameter, "OmitInitialInfo", StringComparison.OrdinalIgnoreCase)
-                  let path = (parameter.StartsWith("LogFile=", StringComparison.OrdinalIgnoreCase) ? parameter["LogFile=".Length..] : parameter).Trim('"')
-                  let pathWithExtension = path.EndsWith(".binlog", StringComparison.OrdinalIgnoreCase) ? path : $"{path}.binlog"
-                  select pathWithExtension)
-                 .LastOrDefault("msbuild.binlog")
-        };
+            return null;
+        }
+
+        var parsedParams = options.ParseParameters();
+        if (!string.IsNullOrEmpty(parsedParams.LogFile))
+        {
+            return parsedParams.LogFile.EndsWith(".binlog", StringComparison.OrdinalIgnoreCase) 
+                ? parsedParams.LogFile 
+                : $"{parsedParams.LogFile}.binlog";
+        }
+
+        return "msbuild.binlog";
+    }
 
     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)
         {
@@ -276,25 +267,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)

src/Cli/dotnet/Commands/Build/BuildCommandParser.cs

@@ -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);

src/Cli/dotnet/Commands/Clean/CleanCommandParser.cs

@@ -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);
 

src/Cli/dotnet/Commands/MSBuild/MSBuildForwardingApp.cs

@@ -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)
     {
     }

src/Cli/dotnet/Commands/Pack/PackCommandParser.cs

@@ -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

src/Cli/dotnet/Commands/Publish/PublishCommandParser.cs

@@ -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);

src/Cli/dotnet/Commands/Restore/RestoreCommandParser.cs

@@ -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());
 

src/Cli/dotnet/Commands/Run/Api/RunApiCommand.cs

@@ -108,7 +108,8 @@ public override RunApiOutput Execute()
                 applicationArgs: [],
                 readCodeFromStdin: false,
                 environmentVariables: ReadOnlyDictionary<string, string>.Empty,
-                msbuildRestoreProperties: ReadOnlyDictionary<string, string>.Empty);
+                msbuildRestoreProperties: ReadOnlyDictionary<string, string>.Empty,
+                binaryLoggerOptions: null);
 
             runCommand.TryGetLaunchProfileSettingsIfNeeded(out var launchSettings);
             var targetCommand = (Utils.Command)runCommand.GetTargetCommand(buildCommand.CreateProjectInstance);

src/Cli/dotnet/Commands/Run/RunCommand.cs

@@ -46,6 +46,11 @@ public class RunCommand
 
     public ReadOnlyDictionary<string, string>? RestoreProperties { get; }
 
+    /// <summary>
+    /// Binary logger options, if specified on the command line.
+    /// </summary>
+    public BinaryLoggerOptions? BinaryLoggerOptions { get; }
+
     /// <summary>
     /// unparsed/arbitrary CLI tokens to be passed to the running application
     /// </summary>
@@ -89,7 +94,8 @@ public RunCommand(
         string[] applicationArgs,
         bool readCodeFromStdin,
         IReadOnlyDictionary<string, string> environmentVariables,
-        ReadOnlyDictionary<string, string>? msbuildRestoreProperties)
+        ReadOnlyDictionary<string, string>? msbuildRestoreProperties,
+        BinaryLoggerOptions? binaryLoggerOptions)
     {
         Debug.Assert(projectFileFullPath is null ^ entryPointFileFullPath is null);
         Debug.Assert(!readCodeFromStdin || entryPointFileFullPath is not null);
@@ -108,6 +114,7 @@ public RunCommand(
         MSBuildArgs = SetupSilentBuildArgs(msbuildArgs);
         EnvironmentVariables = environmentVariables;
         RestoreProperties = msbuildRestoreProperties;
+        BinaryLoggerOptions = binaryLoggerOptions;
     }
 
     public int Execute()
@@ -356,7 +363,7 @@ internal ICommand GetTargetCommand(Func<ProjectCollection, ProjectInstance>? pro
             return CreateCommandForCscBuiltProgram(EntryPointFileFullPath);
         }
 
-        FacadeLogger? logger = LoggerUtility.DetermineBinlogger([..MSBuildArgs.OtherMSBuildArgs], "dotnet-run");
+        FacadeLogger? logger = CreateBinaryLogger(BinaryLoggerOptions, "dotnet-run");
         var project = EvaluateProject(ProjectFileFullPath, projectFactory, MSBuildArgs, logger);
         ValidatePreconditions(project);
         InvokeRunArgumentsTarget(project, NoBuild, logger, MSBuildArgs);
@@ -465,6 +472,38 @@ static void InvokeRunArgumentsTarget(ProjectInstance project, bool noBuild, Faca
         }
     }
 
+    private static FacadeLogger? CreateBinaryLogger(BinaryLoggerOptions? options, string verb)
+    {
+        if (options == null)
+        {
+            return null;
+        }
+
+        List<BinaryLogger> binaryLoggers = [];
+        var parsedParams = options.ParseParameters();
+
+        string filename;
+        if (!string.IsNullOrEmpty(parsedParams.LogFile))
+        {
+            filename = parsedParams.LogFile;
+            if (filename.EndsWith(".binlog"))
+            {
+                filename = filename.Substring(0, filename.Length - ".binlog".Length);
+                filename = $"{filename}-{verb}.binlog";
+            }
+        }
+        else
+        {
+            // Default filename when no specific file is provided
+            filename = $"msbuild-{verb}.binlog";
+        }
+
+        binaryLoggers.Add(new BinaryLogger { Parameters = filename });
+
+        // Create the facade logger
+        return LoggerUtility.CreateFacadeLogger(binaryLoggers);
+    }
+
     static readonly string ComputeRunArgumentsTarget = "ComputeRunArguments";
 
     private static LoggerVerbosity ToLoggerVerbosity(VerbosityOptions? verbosity)
@@ -596,23 +635,14 @@ public static RunCommand FromParseResult(ParseResult parseResult)
             parseResult = ModifyParseResultForShorthandProjectOption(parseResult);
         }
 
-        // if the application arguments contain any binlog args then we need to remove them from the application arguments and apply
-        // them to the restore args.
-        // this is because we can't model the binlog command structure in MSbuild in the System.CommandLine parser, but we need
-        // bl information to synchronize the restore and build logger configurations
+        // Get application arguments (no need to separate binary log args since they're now handled as a ForwardedOption)
         var applicationArguments = parseResult.GetValue(RunCommandParser.ApplicationArguments)?.ToList();
 
-        LoggerUtility.SeparateBinLogArguments(applicationArguments, out var binLogArgs, out var nonBinLogArgs);
-
         var msbuildProperties = parseResult.OptionValuesToBeForwarded(RunCommandParser.GetCommand()).ToList();
-        if (binLogArgs.Count > 0)
-        {
-            msbuildProperties.AddRange(binLogArgs);
-        }
 
         // Only consider `-` to mean "read code from stdin" if it is before double dash `--`
         // (otherwise it should be forwarded to the target application as its command-line argument).
-        bool readCodeFromStdin = nonBinLogArgs is ["-", ..] &&
+        bool readCodeFromStdin = applicationArguments is ["-", ..] &&
             parseResult.Tokens.TakeWhile(static t => t.Type != TokenType.DoubleDash)
                 .Any(static t => t is { Type: TokenType.Argument, Value: "-" });
 
@@ -624,7 +654,7 @@ public static RunCommand FromParseResult(ParseResult parseResult)
             throw new GracefulException(CliCommandStrings.CannotCombineOptions, RunCommandParser.ProjectOption.Name, RunCommandParser.FileOption.Name);
         }
 
-        string[] args = [.. nonBinLogArgs];
+        string[] args = [.. applicationArguments ?? []];
         string? projectFilePath = DiscoverProjectFilePath(
             filePath: fileOption,
             projectFileOrDirectoryPath: projectOption,
@@ -661,8 +691,11 @@ public static RunCommand FromParseResult(ParseResult parseResult)
                 stdinStream.CopyTo(fileStream);
             }
 
-            Debug.Assert(nonBinLogArgs[0] == "-");
-            nonBinLogArgs[0] = entryPointFilePath;
+            Debug.Assert(applicationArguments?[0] == "-");
+            if (applicationArguments != null)
+            {
+                applicationArguments[0] = entryPointFilePath;
+            }
         }
 
         var msbuildArgs = MSBuildArgs.AnalyzeMSBuildArguments(msbuildProperties, CommonOptions.PropertiesOption, CommonOptions.RestorePropertiesOption, CommonOptions.MSBuildTargetOption(), RunCommandParser.VerbosityOption);
@@ -681,7 +714,8 @@ public static RunCommand FromParseResult(ParseResult parseResult)
             applicationArgs: args,
             readCodeFromStdin: readCodeFromStdin,
             environmentVariables: parseResult.GetValue(CommonOptions.EnvOption) ?? ImmutableDictionary<string, string>.Empty,
-            msbuildRestoreProperties: parseResult.GetValue(CommonOptions.RestorePropertiesOption)
+            msbuildRestoreProperties: parseResult.GetValue(CommonOptions.RestorePropertiesOption),
+            binaryLoggerOptions: parseResult.GetValue(CommonOptions.BinaryLoggerOption)
         );
 
         return command;

src/Cli/dotnet/Commands/Run/RunCommandParser.cs

@@ -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);
 

src/Cli/dotnet/Commands/Store/StoreCommandParser.cs

@@ -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);
 

src/Cli/dotnet/Commands/Test/TestCommandParser.cs

@@ -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);