Add support for MSBUILD_LOGGING_ARGS #12993
Conversation
Document the process for enabling binary logging in CI/CD pipelines using environment variables, including supported arguments, argument processing order, and implementation flow. Fixes #12804
| /// <summary> | ||
| /// Name of environment variable for logging level (warning or message). | ||
| /// </summary> | ||
| public const string MSBuildLoggingArgsLevelEnvVarName = "MSBUILD_LOGGING_ARGS_LEVEL"; |
There was a problem hiding this comment.
When we document MSBUILD_LOGGING_ARGS_LEVEL we need to be very clear on its use - this flag only controls the level of the diagnostic messages emitted when processing these envvar log arguments. That wasn't immediately clear to me just based on reading.
| return IsBinaryLoggerSwitch(switchName) || string.Equals(switchName, "check", StringComparison.OrdinalIgnoreCase); | ||
| } | ||
|
|
||
| private static bool IsBinaryLoggerSwitch(string switchName) => string.Equals(switchName, "bl", StringComparison.OrdinalIgnoreCase) || string.Equals(switchName, "binarylogger", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
nit - instead of the string constants here we should be able to use the constants from the binlog command line switch definition itself?
| int colonIndex = switchPart.IndexOf(':'); | ||
| string switchName = colonIndex >= 0 ? switchPart.Substring(0, colonIndex) : switchPart; | ||
|
|
||
| return IsBinaryLoggerSwitch(switchName) || string.Equals(switchName, "check", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
and same for the 'check' switch.
| <!-- **** MSBUILD_LOGGING_ARGS strings **** --> | ||
| <data name="LoggingArgsEnvVarUsing" xml:space="preserve"> | ||
| <value>Using arguments from MSBUILD_LOGGING_ARGS environment variable: {0}</value> | ||
| <comment>LOCALIZATION: "MSBUILD_LOGGING_ARGS" should not be localized. {0} is the arguments from the environment variable.</comment> |
There was a problem hiding this comment.
| <comment>LOCALIZATION: "MSBUILD_LOGGING_ARGS" should not be localized. {0} is the arguments from the environment variable.</comment> | |
| <comment>LOCALIZATION: "MSBUILD_LOGGING_ARGS" should not be localized. {0} is a string with the command-line arguments from the environment variable.</comment> |
| /// <summary> | ||
| /// Value of the MSBUILD_LOGGING_ARGS_LEVEL environment variable. | ||
| /// </summary> | ||
| public static string? MSBuildLoggingArgsLevel => Environment.GetEnvironmentVariable(MSBuildLoggingArgsLevelEnvVarName); |
There was a problem hiding this comment.
I'd rather this return an enum (or even a bool) and put the string->logical-meaning logic here rather than at the consumer (xmake).
| string switchPart = arg.Substring(GetLengthOfSwitchIndicator(arg)); | ||
|
|
||
| // Extract switch name (before any ':' parameter indicator) | ||
| int colonIndex = switchPart.IndexOf(':'); | ||
| string switchName = colonIndex >= 0 ? switchPart.Substring(0, colonIndex) : switchPart; | ||
|
|
||
| return IsBinaryLoggerSwitch(switchName) || string.Equals(switchName, "check", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
change this to avoid allocating the substrings, by using Span<char> instead.
| } | ||
| catch (Exception ex) | ||
| { | ||
| LogLoggingArgsMessage(ResourceUtilities.FormatResourceStringStripCodeAndKeyword("LoggingArgsEnvVarError", ex.Message), emitAsMessage); |
There was a problem hiding this comment.
ex.Message is a bit of a smell--it drops stack info.
Do we know of anything that could happen in "normal operation" that would cause this? If so, it might be a better UX to have a terse error message. But if this is a last-chance no-idea-how-it-could-happen thing, we should probably log the stack so if it ever happens we can figure it out.
| <comment>{StrBegin="MSBUILD : warning MSB1070: "}LOCALIZATION: "MSBUILD_LOGGING_ARGS" should not be localized. {0} is the unsupported argument.</comment> | ||
| </data> | ||
| <data name="LoggingArgsEnvVarError" xml:space="preserve"> | ||
| <value>MSBUILD : error MSB1071: Error processing MSBUILD_LOGGING_ARGS environment variable: {0}</value> |
There was a problem hiding this comment.
do these codes actually get emitted in the warning case? I'm confused by the stripping call at the place they get logged.
|
Worth noting that the branch protections preventing merge aren't active on this branch due to the naming - if we're going to use the |
Fixes #12804
Summary
This PR implements the MSBUILD_LOGGING_ARGS environment variable feature as described in the design spec (#12805). This allows enabling binary logging collection in CI/CD pipelines without modifying project files or build scripts.
Motivation
In CI/CD environments, it's often desirable to enable diagnostic logging (binary logs) for all builds without:
Modifying project files or .rsp files on disk
Changing build scripts
Affecting local developer builds
This feature enables centralized build diagnostics configuration through environment variables.
Testing
Add comprehensive UT coverage .
connected to #12706