Command-line log replay is no longer too aggressive about versioning#13608
Conversation
AlesProkop
force-pushed
the
command-line-log-replay-too-agressive-versioning
branch
from
843ddaf to
426ce6b
Compare
AlesProkop
changed the title
AlesProkop
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates MSBuild’s command-line binary log replay to be forward-compatible when the binlog’s minimum required reader version is still supported, allowing replay to proceed while warning that some data may be missing (fixes #12254).
Changes:
- Enable forward-compatible binlog replay from the CLI and emit a post-replay warning when the binlog format is newer than the current reader.
- Extend
BinaryLogReplayEventSourceto open readers with forward-compat enabled and expose a version-mismatch warning message. - Add unit tests covering forward-compat reader behavior.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/MSBuild/XMake.cs | Enables forward-compatible replay for CLI binlog replay and prints a warning after replay. |
| src/Build/Logging/BinaryLogger/BinaryLogReplayEventSource.cs | Adds a version-mismatch warning property and wires AllowForwardCompatibility into reader opening. |
| src/Build.UnitTests/BinaryLogger_Tests.cs | Adds unit tests for forward-compatible binlog reader behavior and warning behavior for current version. |
| src/Build/Resources/Strings.resx | Adds a new localized string for the version mismatch warning. |
| src/Build/Resources/xlf/Strings.zh-Hant.xlf | Adds localized entry for the new version mismatch warning string. |
| src/Build/Resources/xlf/Strings.zh-Hans.xlf | Adds localized entry for the new version mismatch warning string. |
| src/Build/Resources/xlf/Strings.tr.xlf | Adds localized entry for the new version mismatch warning string. |
| src/Build/Resources/xlf/Strings.ru.xlf | Adds localized entry for the new version mismatch warning string. |
| src/Build/Resources/xlf/Strings.pt-BR.xlf | Adds localized entry for the new version mismatch warning string. |
| src/Build/Resources/xlf/Strings.pl.xlf | Adds localized entry for the new version mismatch warning string. |
| src/Build/Resources/xlf/Strings.ko.xlf | Adds localized entry for the new version mismatch warning string. |
| src/Build/Resources/xlf/Strings.ja.xlf | Adds localized entry for the new version mismatch warning string. |
| src/Build/Resources/xlf/Strings.it.xlf | Adds localized entry for the new version mismatch warning string. |
| src/Build/Resources/xlf/Strings.fr.xlf | Adds localized entry for the new version mismatch warning string. |
| src/Build/Resources/xlf/Strings.es.xlf | Adds localized entry for the new version mismatch warning string. |
| src/Build/Resources/xlf/Strings.de.xlf | Adds localized entry for the new version mismatch warning string. |
| src/Build/Resources/xlf/Strings.cs.xlf | Adds localized entry for the new version mismatch warning string. |
|
/review |
|
✅ Expert Code Review (command) completed successfully! Expert review of PR #13608 completed. The subagent posted inline comments and a summary review via safe-output tools. Key findings: (1) the AllowForwardCompatibility bug fix is correct, (2) missing test for non-null FormatVersionMismatchWarning, (3) minor doc gap on pre-replay null behavior. No further action needed from the orchestrating agent. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Bug Fix ✅
The Replay(string, CancellationToken) fix is correct and necessary. The old code path called the static OpenBuildEventsReader(string) which silently ignored AllowForwardCompatibility. The other Replay overloads (Stream, BinaryReader) were already correct.
New FormatVersionMismatchWarning Property
The implementation is clean and the semantics are correct — returns a human-readable warning when the binlog format version exceeds the current reader's version, null otherwise. The XMake.cs consumer (already merged separately) uses it appropriately with Console.WriteLine.
Key Findings
| # | Finding | Severity |
|---|---|---|
| 1 | Missing test for positive FormatVersionMismatchWarning — only the null/current-version case is tested. There's no test asserting the property returns a non-null warning when the binlog version is newer. The existing OpenBuildEventsReader_SucceedsForForwardCompatibleVersion pattern could be extended to cover this. |
Medium |
| 2 | Pre-replay behavior difference — FileFormatVersion and MinimumReaderVersion throw InvalidOperationException before replay, but FormatVersionMismatchWarning returns null. This is arguably correct but should be documented in the XML doc. |
Low |
| 3 | Property recomputes on each access — each getter call invokes FormatResourceStringStripCodeAndKeyword (resource lookup + string.Format). Fine for typical single-access patterns but worth noting. |
Info |
Resource String (MSB4281)
The new BinlogFormatVersionMismatch resource string is well-formed. Using an MSBxxxx code for a property-returned string (not an automatically-emitted diagnostic) is appropriate — it gives the message a traceable identifier without risk of breaking /WarnAsError builds since it's not emitted through the build warning pipeline.
What Looks Good
- Test coverage for
OpenBuildEventsReaderforward-compatibility edge cases (incompatible, compatible, opt-out) - Localization files properly updated for all 12 languages
- The fix is minimal and targeted
Note
🔒 Integrity filter blocked 2 items
The following items were blocked because they don't meet the GitHub integrity level.
- #13608
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - #13608
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | noneGenerated by Expert Code Review (command) for issue #13608 · ● 3M
Fixes #12254
Context
Currently, when replaying a log from an older MSBuild, the replay is not done at all and a message indicating this is emitted. It makes sense to replay the log, skip the new entries and still emit this warning message.
Changes Made
XMake.cs
Added a forward compatibility flag. Also emitting the message after the log is replayed to notify the customer that not all of the messages in the log can be present in the replay.
BinaryLogReplayEventSource.cs
Added a version mismatch to emit the warning.
Testing
BinaryLogger_Tests.cs
Added unit tests that test this behaviour.