-
Notifications
You must be signed in to change notification settings - Fork 5k
[release/9.0-staging] [wasm] Read messages from binlog if process output is missing build finished message #114676
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
[release/9.0-staging] [wasm] Read messages from binlog if process output is missing build finished message #114676
Conversation
Tagging subscribers to this area: @akoeplinger, @matouskozak |
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 reviewed 1 out of 3 changed files in this pull request and generated no comments.
Files not reviewed (2)
- eng/Versions.props: Language not supported
- src/mono/wasm/Wasm.Build.Tests/Wasm.Build.Tests.csproj: Language not supported
Comments suppressed due to low confidence (2)
src/mono/wasm/Wasm.Build.Tests/BuildTestBase.cs:180
- Consider adding test cases to cover scenarios where the process output is replaced with binlog messages to ensure this new logic behaves as expected.
// Ensure we got all output.
src/mono/wasm/Wasm.Build.Tests/BuildTestBase.cs:188
- [nitpick] Consider renaming 'buildRoot' to 'binlogRoot' for enhanced clarity that this variable represents the parsed binary log.
var buildRoot = BinaryLog.ReadBuild(logFilePath);
<!-- <SdkVersionForWorkloadTesting>$(MicrosoftDotNetApiCompatTaskVersion)</SdkVersionForWorkloadTesting> --> | ||
<SdkVersionForWorkloadTesting>9.0.105</SdkVersionForWorkloadTesting> |
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.
@akoeplinger Should I merge this in to avoid non-existent 9.0.106
or do we handle it separately?
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.
yeah we're doing it as part of #114700 too. not sure when that PR will go in.
/ba-g CI failures are unrelated, passed on previous run |
Manual backport of #107280.
Mitigates #106160.
Infrastructure only change.