-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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/8.0] [wasm] Endian fix for Webcil #92495
Conversation
'dotnet new blazorwasm' command failed on s390x and was throwing a not implemented exception The issue was with with the WebCil writer and reader, specific endianness conversions relating to the webcil payload were not implemented for big endian machines. We considered fixing the generic implementation, but there were only two structures in use: WebcilHeader and WebcilSectionHeader, so it was easier to handle them explicitly.
Tagging subscribers to this area: @dotnet/ncl Issue DetailsBackport of #92274 to release/8.0 Customer ImpactTestingRiskIMPORTANT: If this backport is for a servicing release, please verify that:
|
Tagging subscribers to 'arch-wasm': @lewing Issue DetailsBackport of #92274 to release/8.0 Customer ImpactCustomers using the browser-wasm workload on s390x and other big-endian platforms are able to produce working wasm webcil modules. TestingManual testing RiskLow/Medium. There is additional code on non-s390x platforms in the webcil assembly, but it is all gated by an endianness check. In .NET Framework builds of the wasm builder MSBuild task there is a new dependency on the System.Memory.dll out of band assembly - so if we got the version dependencies wrong this may break the build for customers building browser-wasm projects using Visual Studio on Windows.
|
@lambdageek is it intentional to target release/8.0, or were you intending to target release/8.0-rc2? If the latter, then please edit the PR title and choose the target branch from the dropdown. Just please make sure to confirm that the commits are the ones you want, because sometimes additional unwanted commits get added. |
I don't see an email requesting Tactics approval, and we're close to EOD so this will have to go to runtime/8.0 directly (RTM). |
@carlossanlop RTM is ok. I wasn't sure there was runway left for RC2. |
@lambdageek don't forget to send an email requesting Tactics approval. I can't find one with this PR's title. |
The System.Memory reference matches the one MSBuild guarantees (and we use ourselves) in supported-for-.NET-8.0 MSBuild/VS versions (17.7+), so I think that risk here is low. |
Approved by Tactics but needs a code review sign-off from another area owner. @lewing @steveisok @radical? Who can volunteer as tribute? |
@lambdageek @lewing @steveisok @radical can we get a sign-off please? |
Backport of #92274 to release/8.0
/cc @lambdageek @saitama951
Customer Impact
Customers using the browser-wasm workload on s390x and other big-endian platforms are able to produce working wasm webcil modules.
Testing
Manual testing
Risk
Low~~/Medium~~. There is additional code on non-s390x platforms in the webcil assembly, but it is all gated by an endianness check.
In .NET Framework builds of the wasm builder MSBuild task there is a new dependency on the System.Memory.dll out of band assembly - so if we got the version dependencies wrong this may break the build for customers building browser-wasm projects using Visual Studio on Windows.(See #92495 (comment))