-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Support multiple binary logs from command line arguments #12706
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
base: main
Are you sure you want to change the base?
Changes from 10 commits
b7d8e1b
4ff3553
3274de9
501181e
0403ab7
9e449d2
0a591a7
8685237
d1e0aae
152cdea
3f5e369
8f9a79e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |||||
| // The .NET Foundation licenses this file to you under the MIT license. | ||||||
|
|
||||||
| using System; | ||||||
| using System.Collections.Generic; | ||||||
| using System.IO; | ||||||
| using System.IO.Compression; | ||||||
| using Microsoft.Build.Experimental.BuildCheck.Infrastructure.EditorConfig; | ||||||
|
|
@@ -296,6 +297,17 @@ private static bool TryParsePathParameter(string parameter, out string filePath) | |||||
|
|
||||||
| internal string FilePath { get; private set; } | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Gets or sets additional output file paths. When set, the binlog will be copied to all these paths | ||||||
| /// after the build completes. The primary FilePath will be used as the temporary write location. | ||||||
| /// </summary> | ||||||
| /// <remarks> | ||||||
| /// This property is intended for internal use by MSBuild command-line processing. | ||||||
| /// It should not be set by external code or logger implementations. | ||||||
| /// Use multiple logger instances with different Parameters instead. | ||||||
| /// </remarks> | ||||||
| public IReadOnlyList<string> AdditionalFilePaths { get; set; } | ||||||
|
|
||||||
| /// <summary> Gets or sets the verbosity level.</summary> | ||||||
| /// <remarks> | ||||||
| /// The binary logger Verbosity is always maximum (Diagnostic). It tries to capture as much | ||||||
|
|
@@ -505,6 +517,15 @@ public void Shutdown() | |||||
| } | ||||||
|
|
||||||
|
|
||||||
| // Log additional file paths before closing stream (so they're recorded in the binlog) | ||||||
| if (AdditionalFilePaths != null && AdditionalFilePaths.Count > 0 && stream != null) | ||||||
| { | ||||||
| foreach (var additionalPath in AdditionalFilePaths) | ||||||
| { | ||||||
| LogMessage("BinLogCopyDestination=" + additionalPath); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| if (stream != null) | ||||||
| { | ||||||
| // It's hard to determine whether we're at the end of decoding GZipStream | ||||||
|
|
@@ -514,6 +535,37 @@ public void Shutdown() | |||||
| stream.Dispose(); | ||||||
| stream = null; | ||||||
| } | ||||||
|
|
||||||
| // Copy the binlog file to additional destinations if specified | ||||||
| if (AdditionalFilePaths != null && AdditionalFilePaths.Count > 0) | ||||||
| { | ||||||
| foreach (var additionalPath in AdditionalFilePaths) | ||||||
| { | ||||||
| try | ||||||
| { | ||||||
| string directory = Path.GetDirectoryName(additionalPath); | ||||||
| if (!string.IsNullOrEmpty(directory)) | ||||||
| { | ||||||
| Directory.CreateDirectory(directory); | ||||||
| } | ||||||
| File.Copy(FilePath, additionalPath, overwrite: true); | ||||||
| } | ||||||
| catch (Exception ex) | ||||||
| { | ||||||
| // Log the error but don't fail the build | ||||||
| // Note: We can't use LogMessage here since the stream is already closed | ||||||
| string message = ResourceUtilities.FormatResourceStringStripCodeAndKeyword( | ||||||
| out string errorCode, | ||||||
| out string helpKeyword, | ||||||
| "ErrorCopyingBinaryLog", | ||||||
| FilePath, | ||||||
| additionalPath, | ||||||
| ex.Message); | ||||||
|
|
||||||
| throw new LoggerException(message, ex, errorCode, helpKeyword); | ||||||
|
||||||
| throw new LoggerException(message, ex, errorCode, helpKeyword); | |
| Console.Error.WriteLine(message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does a single logger failing break the build? How does the loggingservice handle this?
YuliiaKovalova marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
YuliiaKovalova marked this conversation as resolved.
Show resolved
Hide resolved
YuliiaKovalova marked this conversation as resolved.
Show resolved
Hide resolved
YuliiaKovalova marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test coverage for the new public static methods ProcessParameters, ExtractFilePathFromParameters, and ExtractNonPathParameters in BinaryLogger. These are now part of the public API but have no direct unit tests in the BinaryLogger test suite.
While these methods are tested indirectly through the XMake integration tests, they should have dedicated unit tests to verify edge cases such as:
- Empty or null parameter arrays
- Parameters with wildcards
- Parameters with various configurations (ProjectImports, OmitInitialInfo, etc.)
- Invalid path formats
- Mixed valid and invalid parameters
This would provide better isolation and faster feedback when these methods have bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot add the tests suggested by your colleague
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added dedicated unit tests for all three public static methods: ExtractFilePathFromParameters, ExtractNonPathParameters, and ProcessParameters. Tests cover edge cases including null/empty inputs, various parameter formats, duplicate path handling (case-insensitive), and different configuration scenarios. (3f5e369)
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rainersigwald I am not sure how to handle it better - I some places I see
Console.WriteLineusage, but it's not clear when we can use it?