Add documentation for enabling binlog collection via env var #12805
Conversation
Document the process for enabling binary logging in CI/CD pipelines using environment variables, including supported arguments, argument processing order, and implementation flow.
|
|
||
| ## Warning Messages | ||
|
|
||
| All issues are logged as **warnings**, never errors. Builds must not fail due to environment variable processing. |
There was a problem hiding this comment.
Would these be Messages logged through the configured Loggers, or raw CLI output? If the latter, we should strongly consider writing these to stderr - in general all 'verbose', non-Logger-provided output should go to that channel.
There was a problem hiding this comment.
I agree in principal but I'm not sure that this specific scenario is the best place to start using stderr--I suspect many build scripts don't capture it well.
There was a problem hiding this comment.
We've started breaking that seal in the dotnet CLI in 10.0.1xx, and busted it wide open in 10.0.200 already. It's a bigger unknown for msbuild.exe for sure though.
| @@ -0,0 +1,113 @@ | |||
| ## Purpose | |||
|
|
|||
| Enable binary logging in CI/CD pipelines when `-noAutoResponse` blocks response files, without modifying build scripts or project files. | |||
There was a problem hiding this comment.
Think that this is a problem even without -noAutoResponse. Using an RSP file would necessitate writing artifacts to disk or modifying an existing RSP file in place. The goal I was thinking of is how can you enable collection without touching the artifacts on disk at all.
There was a problem hiding this comment.
without touching the artifacts on disk at all
Strong agree.
| - `-bl` / `/bl` / `-binarylogger` / `/binarylogger` (with optional parameters) | ||
| - `-check` / `/check` (with optional parameters - requires code modification to support `deferred` mode) | ||
|
|
||
| **All other switches are blocked** as security risks. These two switches are diagnostic/monitoring tools that don't affect build behavior or enable code execution. |
There was a problem hiding this comment.
What are the security risks that this poses? My mental model is that once you're able to inject environment variables into the build then you can change a lot of operations.
Not disputing this angle, just trying to get my head into the right place here.
There was a problem hiding this comment.
environment variables are assumed to be trusted in our threat model, you can specify loading of dlls or proj/targets/props files that will be executed as a part of the build
There was a problem hiding this comment.
IMO this restriction is not a security measure, it's more about diagnosability. It is already very annoying to discover that stray environment variables have acted as build inputs (Platform is an old favorite env var that horks builds). The reason I prefer a restricted logging-only env var is to avoid being able to do so in a new, even harder-to-understand way.
There was a problem hiding this comment.
We currently log the entire command line in the binlog, but we do not 'classify' where arguments of that command line came from. Same is true for various trait flag. It might be an interesting future enhancement to add this provenance information to the binlog.
| 1. **MSBuild.rsp** (next to MSBuild.exe) - skipped if `-noAutoResponse` present | ||
| 2. **Directory.Build.rsp** (next to project) - skipped if `-noAutoResponse` present | ||
| 3. **MSBUILD_LOGGING_ARGS** - always processed, regardless of `-noAutoResponse` | ||
| 4. **Command-line arguments** - highest precedence |
There was a problem hiding this comment.
Given that you limited the set of arguments that could occur in MSBUILD_LOGGING_ARGS is the precedence between 3 and 4 relevant? Both -bl and -check add information (new location for binary logger, new check to run) not replace so seems like this shouldn't have impact.
This is another case where I'm just trying to get my head in the right place. Essentially is this significant for 3 and 4 or not?
There was a problem hiding this comment.
I agree--precedence for this env var is the highest possible level, but because it's only allowed to add log outputs it can't override other argument-passing mechanisms so precedence isn't super interesting.
|
|
||
| **Implementation Note**: MSBuild's existing argument parser handles merging logic. The environment variable arguments are prepended to the command line before parsing, ensuring natural precedence. | ||
|
|
||
| ## Implementation Flow |
There was a problem hiding this comment.
Where all would this end up impacting the builds? Certainly expect that dotnet build and msbuild will process these args but what else? Guessing that say the builds done in a Roslyn msbuild workspace would not emit logs (or would they)?
There was a problem hiding this comment.
Correct--only things that go through MSBuild's Main(), so dotnet build/dotnet msbuild/MSBuild.exe and not MSBuildWorkspace or VS or MyFancyCustomBuildDriverViaAPI.exe.
The only thing that makes me super worried there is devenv.exe /build which is definitely used in official-build scenarios in the wild (though is rare inside Microsoft).
We do have the ability to inject a binlog into even API-driven builds (MSBUILDDEBUGENGINE, which we use for debugging VS builds). We could use that instead of new functionality. I think this spec should discuss and address that.
There was a problem hiding this comment.
Pull request overview
This PR adds documentation for a proposed feature that would enable binary logging in CI/CD pipelines through the MSBUILD_LOGGING_ARGS environment variable. The feature is designed to work even when -noAutoResponse blocks response files, providing an alternative mechanism for enabling diagnostic logging.
Key changes:
- Documents the
MSBUILD_LOGGING_ARGSenvironment variable and its purpose - Specifies supported arguments (
-bland-check) and argument processing order - Describes parameter merging behavior and implementation flow
| @@ -0,0 +1,113 @@ | |||
| ## Purpose | |||
There was a problem hiding this comment.
The file should have a .md extension to be consistent with other specification documents in this directory. All other spec files in documentation/specs/ use the .md extension (e.g., low-priority-switch.md, event-source.md, test-target.md).
| ## Build Check (-check) Handling | ||
|
|
||
| ### Deferred Analysis Mode | ||
|
|
||
| `-check:deferred` enables binlog replay analysis without build-time overhead: | ||
|
|
||
| - **During build**: Flag recorded in binlog, BuildCheck NOT activated | ||
| - **During replay**: Binlog reader activates BuildCheck for analysis | ||
|
|
||
| **Rationale**: BuildCheck analysis can be expensive. Environment variable is for diagnostics that can be analyzed later, allowing teams to record data without performance impact. | ||
|
|
||
| ### Example Workflow | ||
|
|
||
| ```bash | ||
| # 1. Configure environment | ||
| set MSBUILD_LOGGING_ARGS=-bl:build.binlog -check:deferred | ||
|
|
||
| # 2. Run build (normal speed, no BuildCheck overhead) | ||
| msbuild solution.sln | ||
|
|
||
| # 3. Later: Replay binlog (BuildCheck analyzes recorded events) | ||
| msbuild build.binlog | ||
| ``` |
There was a problem hiding this comment.
The documentation describes a deferred mode for the -check argument that is not documented in the existing BuildCheck specification files. This feature should either:
- Have corresponding implementation or specification documentation in
documentation/specs/BuildCheck/ - Be marked as a future feature or proposal
- Include a reference to where this feature is documented
Without supporting documentation, readers cannot verify this functionality exists or understand its complete behavior.
| @@ -0,0 +1,113 @@ | |||
| ## Purpose | |||
There was a problem hiding this comment.
Missing document title. The file should start with a top-level heading (e.g., # Enable Binary Log Collection via Environment Variable) consistent with other specification documents like low-priority-switch.md, event-source.md, and test-target.md.
| 3. Process response files (if no `-noAutoResponse`) | ||
| 4. Read MSBUILD_LOGGING_ARGS environment variable | ||
| 5. Validate and filter arguments | ||
| 6. Prepend valid arguments to command line |
There was a problem hiding this comment.
[nitpick] The implementation flow description states "Prepend valid arguments to command line" (line 57), but this is an implementation detail that cannot be verified without seeing the actual code. If this feature is not yet implemented, consider using language like "will prepend" or "should prepend" to indicate this is a design specification rather than describing existing behavior.
| 6. Prepend valid arguments to command line | |
| 6. Will prepend valid arguments to command line |
| ## Supported Arguments | ||
|
|
||
| - `-bl` / `/bl` / `-binarylogger` / `/binarylogger` (with optional parameters) | ||
| - `-check` / `/check` (with optional parameters - requires code modification to support `deferred` mode) |
There was a problem hiding this comment.
The parenthetical comment "(requires code modification to support deferred mode)" is unclear. If code modification is required for this feature to work, this should be explained in more detail. Specifically:
- What code needs to be modified?
- Is this referring to MSBuild code or user code?
- If this is a future feature, it should be clearly marked as such
This unclear statement may confuse readers about the current state of the feature.
| - `-check` / `/check` (with optional parameters - requires code modification to support `deferred` mode) | |
| - `-check` / `/check` (with optional parameters; Note: `deferred` mode is not currently supported—enabling this feature requires changes to the MSBuild codebase and is planned for a future release. See section "Build Check (-check) Handling" below.) |
### Context Docs-only PRs fail the Code Coverage CI job because tests are skipped (no coverage artifacts generated), but the job unconditionally attempts to download and publish those artifacts. ### Changes Made Modified `.vsts-dotnet-ci.yml` CodeCoverage job to respect the `onlyDocChanged` flag: - Added dependency on `IfOnlyDocumentionChanged` job to access the flag - Added `onlyDocChanged` variable to job variables - Conditioned all artifact downloads on `eq(variables.onlyDocChanged, 0)` - Conditioned coverage processing and publishing on `eq(variables.onlyDocChanged, 0)` This aligns with how other jobs (BootstrapMSBuildOnFullFrameworkWindows, CoreBootstrappedOnLinux, etc.) already handle docs-only PRs. ### Testing Pipeline configuration change - will be validated on next PR run. When `onlyDocChanged = 1`, all coverage tasks skip; when `onlyDocChanged = 0`, coverage runs normally. ### Notes Pre-existing typos in job/variable names (`Documention`, `Varibale`) intentionally preserved for consistency across the pipeline. <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>docs only PRs fail code coverage CI check</issue_title> > <issue_description>#12805 > #12857 > > ``` > Code Coverage failed > [3 errors / 0 warnings](https://dev.azure.com/dnceng-public/cbb18261-c48f-4abb-8651-8cdcb5474649/_build/results?buildId=1217301) > > Annotations > [Check failure on line 9 in Build log](https://github.com/dotnet/msbuild/pull/12857/files#annotation_42398343450) > > @azure-pipelines > azure-pipelines > / msbuild-pr (Code Coverage) > Build log #L9 > > Publishing build artifacts failed with an error: Not found PathtoPublish: D:\a\1\s\artifacts\CoverageResults\merged.coverage > [Check failure on line 11 in Build log](https://github.com/dotnet/msbuild/pull/12857/files#annotation_42398343487) > > @azure-pipelines > azure-pipelines > / msbuild-pr (Code Coverage) > Build log #L11 > > Artifact LinuxCoreCoverage was not found for build 1217301. > [Check failure on line 9 in Build log](https://github.com/dotnet/msbuild/pull/12857/files#annotation_42398343497) > > @azure-pipelines > azure-pipelines > / msbuild-pr (Code Coverage) > Build log #L9 > > Publishing build artifacts failed with an error: Not found PathtoPublish: D:\a\1\s\artifacts\CoverageResults\merged.cobertura.xml > ``` > > This is probably because testing is skipped on docs only PRs, in that case code coverage should also be skipped</issue_description> > > <agent_instructions>fix this issue, note that the CI pipeline definition is called .vsts-dotnet-ci.yml</agent_instructions> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > </comments> > </details> - Fixes #12878 <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/dotnet/msbuild/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: JanProvaznik <[email protected]>
| @@ -0,0 +1,113 @@ | |||
| ## Purpose | |||
|
|
|||
| Enable binary logging in CI/CD pipelines when `-noAutoResponse` blocks response files, without modifying build scripts or project files. | |||
There was a problem hiding this comment.
without touching the artifacts on disk at all
Strong agree.
| - `-bl` / `/bl` / `-binarylogger` / `/binarylogger` (with optional parameters) | ||
| - `-check` / `/check` (with optional parameters - requires code modification to support `deferred` mode) | ||
|
|
||
| **All other switches are blocked** as security risks. These two switches are diagnostic/monitoring tools that don't affect build behavior or enable code execution. |
There was a problem hiding this comment.
IMO this restriction is not a security measure, it's more about diagnosability. It is already very annoying to discover that stray environment variables have acted as build inputs (Platform is an old favorite env var that horks builds). The reason I prefer a restricted logging-only env var is to avoid being able to do so in a new, even harder-to-understand way.
| 1. **MSBuild.rsp** (next to MSBuild.exe) - skipped if `-noAutoResponse` present | ||
| 2. **Directory.Build.rsp** (next to project) - skipped if `-noAutoResponse` present | ||
| 3. **MSBUILD_LOGGING_ARGS** - always processed, regardless of `-noAutoResponse` | ||
| 4. **Command-line arguments** - highest precedence |
There was a problem hiding this comment.
I agree--precedence for this env var is the highest possible level, but because it's only allowed to add log outputs it can't override other argument-passing mechanisms so precedence isn't super interesting.
| 3. **MSBUILD_LOGGING_ARGS** - always processed, regardless of `-noAutoResponse` | ||
| 4. **Command-line arguments** - highest precedence | ||
|
|
||
| The environment variable is processed **after** checking for `-noAutoResponse`, making it a reliable fallback when response files are disabled. |
There was a problem hiding this comment.
I don't see how this matters, since it's additive-only.
| **Binary Logger (-bl)** | ||
| - Multiple `-bl` arguments are **allowed and cumulative** | ||
| - Each creates a separate binlog file | ||
| - Example: `MSBUILD_LOGGING_ARGS=-bl:env.binlog` + command line `-bl:cmd.binlog` → both files created |
There was a problem hiding this comment.
This is already handled by the existing CLI logic, right? Or will be after #12706?
|
|
||
| ```bash | ||
| # Environment provides base logging | ||
| set MSBUILD_LOGGING_ARGS=-bl:base.binlog -check:deferred |
There was a problem hiding this comment.
| set MSBUILD_LOGGING_ARGS=-bl:base.binlog -check:deferred | |
| set MSBUILD_LOGGING_ARGS=-bl:base{}.binlog -check:deferred |
|
|
||
| ## Supported Arguments | ||
|
|
||
| - `-bl` / `/bl` / `-binarylogger` / `/binarylogger` (with optional parameters) |
There was a problem hiding this comment.
Should we be even more specific than this? I think for the use case we're thinking of the only bl arguments that make sense have a path and a {} placeholder (to allow multiple build invocations without overwriting logs).
|
|
||
| ## Warning Messages | ||
|
|
||
| All issues are logged as **warnings**, never errors. Builds must not fail due to environment variable processing. |
There was a problem hiding this comment.
Note that new warnings can easily be errors in a customer build. I think our position is that "if you opt into new functionality by setting this environment variable, you are also opting into those warnings", right?
|
|
||
| - **Informational**: "Using arguments from MSBUILD_LOGGING_ARGS environment variable: {0}" | ||
| - **Rejected Argument**: "MSBUILD_LOGGING_ARGS: Ignoring unsupported argument '{0}'. Only -bl and -check arguments are allowed." | ||
| - **Processing Error**: "Error processing MSBUILD_LOGGING_ARGS environment variable: {0}" |
There was a problem hiding this comment.
In this last error case, what should MSBuild do concretely? What's the difference between "ignoring an argument" and "error"?
|
|
||
| ### Deferred Analysis Mode | ||
|
|
||
| `-check:deferred` enables binlog replay analysis without build-time overhead: |
There was a problem hiding this comment.
| `-check:deferred` enables binlog replay analysis without build-time overhead: | |
| `-check:deferred` enables binlog replay analysis with reduced build-time overhead: |
Right? We still need to track the additional data that checks will need on replay?
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