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

Move ILLink.LinkAttributes.xml to a shared location #48217

Closed
MichalStrehovsky opened this issue Feb 12, 2021 · 9 comments · Fixed by #56475
Closed

Move ILLink.LinkAttributes.xml to a shared location #48217

MichalStrehovsky opened this issue Feb 12, 2021 · 9 comments · Fixed by #56475
Assignees
Labels
linkable-framework Issues associated with delivering a linker friendly framework
Milestone

Comments

@MichalStrehovsky
Copy link
Member

We should move src/mono/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.xml to a shared location and make various attribute removal conditional on feature switches that SDKs (or end users) can opt into.

The list currently has things in it that are known to cause behavioral differences in XmlSerializer (ObsoleteAttribute) or Entity Framework (the nullable annotations cruft). While this is likely fine for WASM, broken Entity Framework will likely be unacceptable for Xamarin.

Without feature switches, the workarounds for someone wanting to keep the stripped attributes are pretty laborious.

@MichalStrehovsky MichalStrehovsky added the linkable-framework Issues associated with delivering a linker friendly framework label Feb 12, 2021
@MichalStrehovsky MichalStrehovsky added this to the 6.0.0 milestone Feb 12, 2021
@ghost
Copy link

ghost commented Feb 12, 2021

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @tannergooding, @sbomer
See info in area-owners.md if you want to be subscribed.

Issue Details

We should move src/mono/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.xml to a shared location and make various attribute removal conditional on feature switches that SDKs (or end users) can opt into.

The list currently has things in it that are known to cause behavioral differences in XmlSerializer (ObsoleteAttribute) or Entity Framework (the nullable annotations cruft). While this is likely fine for WASM, broken Entity Framework will likely be unacceptable for Xamarin.

Without feature switches, the workarounds for someone wanting to keep the stripped attributes are pretty laborious.

Author: MichalStrehovsky
Assignees: -
Labels:

linkable-framework

Milestone: 6.0.0

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Feb 12, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@eerhardt
Copy link
Member

@MichalStrehovsky - do you have an idea for what the feature switches would be named? How granular do you think the switches should be? A switch per attribute feels too granular, and a switch for the whole thing feels too big.

@MichalStrehovsky
Copy link
Member Author

One natural grouping would be all the nullable junk. That covers like half of the list.

I would maybe create categories for those that have reasonable payoff and the rest can be WASM only, no-feature-switch, where ~50 bytes is a meaningful saving (e.g. it doesn't feel like stripping ObsoleteAttribute could produce meaningful savings).

@marek-safar
Copy link
Contributor

As a note, Xamarin is way more aggressive than Blazor today with attributes removal. Leaving attributes like Obsolete out the list is very likely not going to work because they have hundreds/thousands of them used and we'd need to mitigate that differently.

@MichalStrehovsky
Copy link
Member Author

In that case it should probably go into one of those that should be under a feature switch.

In either case whether the stripping should happen should probably be decided by the SDK and not by CoreLib as it is done right now. It doesn't feel right to have e.g. XmlSerializer behave differently in the same MAUI app depending on whether you target Android (that uses Mono's corelib) vs Windows desktop (that would use CoreCLR's corelib).

@marek-safar marek-safar removed the untriaged New issue has not been triaged by the area owner label Feb 13, 2021
@eerhardt
Copy link
Member

@MichalStrehovsky @marek-safar - do you feel this is a "must have" for 6.0? Or can it be moved to Future?

@MichalStrehovsky
Copy link
Member Author

If we don't put them in a shared location where it can be enabled/disabled by the SDK, we're going to have differences in trimmed apps depending on whether the runtime is Mono or CoreCLR.

This is around attribute trimming which is a sketchy "no warnings if we broke your app" territory. I think we would want MAUI apps to behave the same irrespective of what runtime is used.

@marek-safar
Copy link
Contributor

@eerhardt it should be done for net6 release

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 28, 2021
@eerhardt eerhardt assigned eerhardt and unassigned joperezr Jul 30, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 30, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
4 participants