-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add IPAddress ISpanFormattable/Parsable implementations #82913
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #82842
|
@lewing, can you help me understand what would cause this failure on wasm?
The cited method is an explicit static interface implementation added in this PR. |
I'll take a look but failures in the EAT (Enable Aggressive Trimming) lane are almost always trimming related |
@lambdageek @vitek-karas @sbomer any idea why class initialization would fail here after the aggressive trimming pass? |
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
Not sure, but there was a recently-fixed issue around static interface trimming that may be relevant: #82197. Though I wouldn't expect this to be a problem if the EAT test is fully trimming the assembly containing the implementation. @jtschuster |
The error looks the same as the failures that #82197 fixed, but since all the assemblies are fully trimmed, I think this might be related to having multiple layers with IPAddress implementing ISpanParsable which implements IParsable. I'll look into it. |
Thanks. In the meantime, can I disable these test suites for these legs, and if so, what's the best way to do so currently? Or should I hold off on this PR until you complete your investigation? |
@lewing, I merged in the latest main and it's now passed the wasm legs. Does that mean this is ok to merge? Did we maybe take in some dependency update that included a fix? |
go ahead and merge, i'll keep an eye out for any problems |
Looks like yes, the failure was using version 8.0.0-preview.2.23112.4 of the linker which didn't have #82197, and now runtime uses version 8.0.0-preview.2.23126.3 with the fix. |
Thanks |
Fixes #82842