-
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
Disable STJ reflection by default on AOT publish #88609
Disable STJ reflection by default on AOT publish #88609
Conversation
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsFollowing up on dotnet/sdk#33757 (comment) this updates the AOT targets so that the
|
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.
This LGTM. It would be good to get a review from @MichalStrehovsky before merging.
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.
This LGTM. It would be good to get a review from @MichalStrehovsky before merging.
I don't think this is necessary. Native AOT without trimming doesn't make sense. Neither does a potential future Mono AOT using Native AOT-specific targets (that this PR is changing). It's likely the RequiresDynamicCode-annotated code doesn't even pose a problem for Mono-based AOT in its current form.
See feedback from @vitek-karas here. @vitek-karas - thoughts? |
I saw that - my comment is that I don't see how the ILCompiler targets (that are edited here) could be relevant for a possible PublishAot-use in Mono and that we haven't even decided how RDC annotations could apply to Mono (Mono doesn't have a matching mode that would be able to take advantage of RDC restrictions so as things stand now, enabling the RDC analyzer on Mono would annoy people with warnings, most of which will not actually be a problem because Mono has both an interpreter, and universal shared code). |
I will happily follow Michal's guidance on this. I was thinking about it on the abstract level - feature which is incompatible with AOT (due to RequiresDynamicCode) should be disabled for AOT. On the other hand, NativeAOT currently doesn't support the configuration without trimming and mono might have different needs (and thus different feature switch defaults). |
It is good to think about PublishAot/PublishTrimmed as two separate things, but since we don't have plans to support PublishAot without PublishTrimmed in ILCompiler (we error out if someone unsets PublishTrimmed), it doesn't feel necessary to put it into ILCompiler .targets file. It might be useful in some Mono AOT .targets file at some point in the future. Such .targets file might not even exist right now thought. I would close this - there's no spot where we could proactively add this right now. |
Following up on dotnet/sdk#33757 (comment) this updates the AOT targets so that the
JsonSerializerIsReflectionEnabledByDefault
feature switch is defaulted to false.