-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Bucket failing Xml trimming test apps #102449
base: main
Are you sure you want to change the base?
Conversation
<NativeAotIncompatible>true</NativeAotIncompatible> | ||
</TestConsoleAppSourceFiles> | ||
<TestConsoleAppSourceFiles Include="XmlSerializer.Deserialize.cs"> | ||
<NativeAotIncompatible>true</NativeAotIncompatible> |
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.
Should we list a reason for XmlSerializer.Deserialize.cs
and XmlSerializer.Serialize.cs
?
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.
I struggled with what to put there and still struggle. I don't understand the reason we have this test in the first place. If we add a new optimization to IL Link that will make this test break same as it's broken with native AOT, would we consider that "illegal" optimization? What is the purpose of trimming tests for something that is marked as trim unsafe?
(My idea always was that we write trimming test if something is annotated as trim safe, but there's a warning suppression in place.)
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.
I don't understand the reason we have this test in the first place.
It is for Xamarin / Blazor WASM style trimming. See #41389. Even though the API is marked as "unsafe" - we wanted to ensure it works if the developer does the work to preserve/mark their own types. (Sort of like how DiagnosticSource and EventSource work.) It is ensuring that we have our internals annotated correctly.
I struggled with what to put there and still struggle.
What's the current problem we run into on native AOT? If we annotated in the test more, would that help?
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.
What's the current problem we run into on native AOT? If we annotated in the test more, would that help?
There are two problems. I'm fixing one now in this PR, but the other problem requires #102482 (I put it into a separate PR so expect failures here for now).
</TestConsoleAppSourceFiles> | ||
<TestConsoleAppSourceFiles Include="XmlSerializer.Deserialize.cs"> | ||
<NativeAotIncompatible>true</NativeAotIncompatible> | ||
</TestConsoleAppSourceFiles> | ||
<TestConsoleAppSourceFiles Include="XmlSerializer.Deserialize.SealerOpt.cs" | ||
ExtraTrimmerArgs="--enable-opt sealer" /> |
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.
The ExtraTrimmerArgs is kind of odd in the context of native AOT. Should this test be disabled too?
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.
Sealing is an optimization that native AOT does automatically so it doesn't seem too irrelevant. Native AOT's version should not really be observable.
Resolves #101228.