Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MichalStrehovsky
Copy link
Member

Resolves #101228.

<NativeAotIncompatible>true</NativeAotIncompatible>
</TestConsoleAppSourceFiles>
<TestConsoleAppSourceFiles Include="XmlSerializer.Deserialize.cs">
<NativeAotIncompatible>true</NativeAotIncompatible>
Copy link
Member

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?

Copy link
Member Author

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.)

Copy link
Member

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?

Copy link
Member Author

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" />
Copy link
Member

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?

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Go over trimming tests we disabled for AOT
2 participants