Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 6, 2025

Fixes #12256

Context

dotnet-watch needs to parse the file path from the /bl command line option without duplicating MSBuild's parsing logic. This PR adds a public API to the BinaryLogger that exposes the parameter parsing functionality.

Changes Made

New Public API:

  • Added BinaryLoggerParameters class to hold parsed logger parameters with properties:
    • LogFilePath - The parsed file path (null for wildcards or when unspecified)
    • ProjectImportsCollectionMode - Import collection mode (None/Embed/ZipFile)
    • OmitInitialInfo - Whether to omit initial info
    • HasProjectImportsParameter (internal) - Tracks if ProjectImports was explicitly specified
  • Added BinaryLogger.ParseParameters(string) static method that parses semicolon-delimited parameter strings

Implementation Changes:

  • Refactored internal ProcessParameters() to use the new public API
  • Extracted parameter string literals to private constants (LogFileParameterPrefix, BinlogFileExtension, OmitInitialInfoParameter, ProjectImportsNoneParameter, ProjectImportsEmbedParameter, ProjectImportsZipFileParameter)
  • Added helper methods TryParseProjectImports() and TrySetProjectImportsMode() for cleaner parsing logic
  • Simplified TryParseProjectImports() to use || operator for more concise code

Bug Fix:

  • Fixed issue where manually setting CollectProjectImports before calling Initialize() would be overridden by parameter parsing

Code Quality Improvements:

  • Using early returns with continue statements instead of nested if-else blocks
  • Extracted duplicate code into helper methods
  • Improved readability with || operator for short-circuit evaluation

Testing

  • ✅ Added 19 comprehensive unit tests for the new ParseParameters API
    • Tests for valid parameters (file paths, imports modes, OmitInitialInfo)
    • Tests for wildcard paths (returns null as per design)
    • Tests for null and invalid parameters (throws LoggerException)
  • ✅ All new tests pass
  • ✅ Fixed 2 tests broken by refactoring (ZipFile tests)
  • ✅ Full build succeeds
  • ⚠️ 3 pre-existing TestBinaryLoggerRoundtrip failures (not caused by this PR)

Notes

Usage Example:

var parsed = BinaryLogger.ParseParameters("LogFile=build.binlog;ProjectImports=None");
Console.WriteLine(parsed.LogFilePath); // "build.binlog"
Console.WriteLine(parsed.ProjectImportsCollectionMode); // None

Important: Wildcard paths ({}) return null for LogFilePath as expansion requires runtime context (timestamp, process ID) only available during logger initialization.

Original prompt

This section details on the original issue you should resolve

<issue_title>Add public API to BinaryLogger that parses its parameter string</issue_title>
<issue_description>The string is parsed in

private bool TryInterpretPathParameter(string parameter, out string filePath)

We need to parse the file path out of the /bl command line option in dotnet-watch and need to replicate the parsing logic.
</issue_description>

Comments on the Issue (you are @copilot in this section)

@MichalPavlik @baronfel / @rainersigwald We could make the method static—as it should be—but I’m leaning toward making it internal and introducing a new friend assembly. I’m hesitant to expand the public API surface. What are your thoughts? :) @baronfel The binary logger itself is already public - I think that there should be a matching typed structure for the parameters that all loggers accept, and parsing methods for those structures should be part of the public API for those loggers. This is the only sane way to keep parity across the CLI and API usage of these loggers.
Fixes #12256

💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Copilot AI changed the title [WIP] Add public API to BinaryLogger that parses its parameter string Add public API to BinaryLogger for parsing parameter strings Oct 6, 2025
Copilot AI requested a review from YuliiaKovalova October 6, 2025 09:03
Copilot finished work on behalf of YuliiaKovalova October 6, 2025 09:03
@YuliiaKovalova
Copy link
Member

@copilot fix the tests you broke

The original implementation of ProcessParameters() only SET the CollectProjectImports property if it was explicitly specified in the parameters string. My refactored version was unconditionally setting it to the parsed value (which defaulted to Embed), thus overwriting any manually-set property value.

Fixed by adding a HasProjectImportsParameter flag to track whether the parameter was explicitly specified, and only setting CollectProjectImports when the flag is true.

This fixes the BinaryLoggerShouldEmbedFilesViaTaskOutput and BinaryLoggerShouldEmbedSymlinkFilesViaTaskOutput tests that were broken by the refactoring.

Note: The TestBinaryLoggerRoundtrip test failures (3 tests) are pre-existing and were failing before this PR.

Co-authored-by: YuliiaKovalova <[email protected]>
Copy link
Contributor Author

Copilot AI commented Oct 6, 2025

@copilot fix the tests you broke

Fixed in commit e9c8db7. The issue was that my refactored parameter parsing was unconditionally setting CollectProjectImports from the parsed parameters, overwriting any manually-set property value. Added a HasProjectImportsParameter flag to only set it when explicitly specified in the parameters string.

The BinaryLoggerShouldEmbedFilesViaTaskOutput and BinaryLoggerShouldEmbedSymlinkFilesViaTaskOutput tests now pass.

Note: The 3 TestBinaryLoggerRoundtrip test failures are pre-existing (they also fail on the base commit 338e820) and were not caused by my changes.

Copilot finished work on behalf of YuliiaKovalova October 6, 2025 17:04
Refactored ParseParameters() to use early returns with 'continue' statements instead of nested if-else blocks. Extracted ProjectImports parsing logic into a separate TryParseProjectImports() helper method for better code organization and readability.

Changes:
- Replaced if-else chain with if-continue pattern for cleaner flow
- Extracted ProjectImports parameter parsing into TryParseProjectImports() method
- Added XML documentation for the new helper method
- All tests still pass (19 ParseParameters tests, 44 BinaryLogger tests total)

Co-authored-by: YuliiaKovalova <[email protected]>
Copilot AI requested a review from YuliiaKovalova October 14, 2025 14:04
Copilot finished work on behalf of YuliiaKovalova October 14, 2025 14:04
Extracted all parameter-related string literals into private constants to improve code maintainability and reduce duplication. This includes:
- LogFileParameterPrefix = "LogFile="
- BinlogFileExtension = ".binlog"
- OmitInitialInfoParameter = "OmitInitialInfo"
- ProjectImportsNoneParameter = "ProjectImports=None"
- ProjectImportsEmbedParameter = "ProjectImports=Embed"
- ProjectImportsZipFileParameter = "ProjectImports=ZipFile"

All tests continue to pass (19 ParseParameters tests, 44 total BinaryLogger tests).

Co-authored-by: YuliiaKovalova <[email protected]>
Copilot AI requested a review from YuliiaKovalova October 27, 2025 10:25
Copilot finished work on behalf of YuliiaKovalova October 27, 2025 10:25
Copilot finished work on behalf of YuliiaKovalova October 27, 2025 10:27
Refactored TryParseProjectImports to eliminate code duplication by extracting the common pattern into a new TrySetProjectImportsMode helper method. This reduces repetition and makes the code more maintainable.

The helper method takes the parameter, expected parameter string, mode value, and result object as parameters, encapsulating the comparison and assignment logic in one place.

All tests continue to pass (19 ParseParameters tests, 44 total BinaryLogger tests).

Co-authored-by: YuliiaKovalova <[email protected]>
Copilot AI requested a review from YuliiaKovalova October 27, 2025 10:55
@YuliiaKovalova YuliiaKovalova marked this pull request as ready for review October 27, 2025 12:00
Copilot AI review requested due to automatic review settings October 27, 2025 12:00
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a public API for parsing BinaryLogger parameter strings to address the need from dotnet-watch to parse /bl command line options without duplicating MSBuild's parsing logic.

Key Changes:

  • Introduced BinaryLoggerParameters class to encapsulate parsed logger parameters
  • Added public BinaryLogger.ParseParameters() static method for parsing parameter strings
  • Refactored internal parameter processing to use the new public API
  • Fixed a bug where manually setting CollectProjectImports before Initialize() would be overridden

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.

File Description
src/Build/Logging/BinaryLogger/BinaryLogger.cs Added public API (BinaryLoggerParameters class and ParseParameters method), refactored internal ProcessParameters to use the new API, and introduced helper methods for parsing
src/Build.UnitTests/BinaryLogger_Tests.cs Added comprehensive tests for ParseParameters method covering valid parameters, wildcard paths, and error cases

@YuliiaKovalova YuliiaKovalova force-pushed the copilot/fix-cf2c81d9-0675-4bf2-808c-0ae67b264ded branch from 2da594e to 7484e5c Compare November 20, 2025 10:35
Refactored TryParseProjectImports to use a single return statement with || operator instead of multiple if statements. This is more concise and takes advantage of short-circuit evaluation, stopping at the first successful match.

All tests continue to pass (19 ParseParameters tests, 44 total BinaryLogger tests).

Co-authored-by: YuliiaKovalova <[email protected]>
Copy link
Contributor Author

Copilot AI commented Nov 20, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • rcxvsblobprodcus328.vsblob.vsassets.io
    • Triggering command: /home/REDACTED/work/msbuild/msbuild/.dotnet/dotnet build src/Build/Microsoft.Build.csproj -v quiet (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot finished work on behalf of YuliiaKovalova November 20, 2025 11:39
Copilot AI requested a review from YuliiaKovalova November 20, 2025 11:39
@YuliiaKovalova YuliiaKovalova merged commit 7085356 into main Nov 20, 2025
10 checks passed
@YuliiaKovalova YuliiaKovalova deleted the copilot/fix-cf2c81d9-0675-4bf2-808c-0ae67b264ded branch November 20, 2025 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add public API to BinaryLogger that parses its parameter string

3 participants