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

Allow trimming FeatureGuard and FeatureSwitchDefinition attributes #100263

Merged
merged 3 commits into from
Mar 28, 2024

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Mar 25, 2024

Under AggressiveAttributeTrimming setting.

Fixes #100256. AggressiveAttributeTrimming was attempting to remove RequiresDynamicCode attributes, but the type was still referenced by FeatureGuardAttribute:

[FeatureGuard(typeof(RequiresDynamicCodeAttribute))]
public static bool IsDynamicCodeCompiled

Adding FeatureGuardAttribute to the set of attributes that get removed with AggressiveAttributeTrimming fixes this. Also adding FeatureSwitchDefinitionAttribute because that one can be removed as well.

Under AggressiveAttributeTrimming setting.
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 25, 2024
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Mar 25, 2024
Copy link
Contributor

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

@eerhardt
Copy link
Member

eerhardt commented Mar 25, 2024

Can we add a test? Since it seems to have broken a user (android).

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@jtschuster jtschuster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have trimmer cases in illink.sln that use AggressiveAttributeTrimming? I would have expected this to be caught by the ILVerify validation in the linker tests if we do.

@sbomer
Copy link
Member Author

sbomer commented Mar 26, 2024

AggressiveAttributeTrimming is a runtime-defined feature setting, not something built-in to ILLink. ILLink does have tests for attribute removal based on feature settings in general.

@vitek-karas
Copy link
Member

AggressiveAttributeTrimming is a runtime-defined feature setting, not something built-in to ILLink. ILLink does have tests for attribute removal based on feature settings in general.

I think the idea is to add a simple executable test (just like we do for other parts of libraries with regard to trimming) which would turn on aggressive attribute trimming and validate that the attributes are in fact removed.

@sbomer sbomer closed this Mar 28, 2024
@sbomer sbomer reopened this Mar 28, 2024
@sbomer
Copy link
Member Author

sbomer commented Mar 28, 2024

Right - I've added such a test. I think @jtschuster was asking specifically why this wasn't tested in the illink tests.

@sbomer
Copy link
Member Author

sbomer commented Mar 28, 2024

The failure is known, and the job that looks like it's still running seems to be a timeout, also hit in other PRs. I created a known issue to track this: #100424.

@sbomer sbomer merged commit e753712 into dotnet:main Mar 28, 2024
147 of 150 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 28, 2024
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 needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[android] trimmer warning for System.Private.CoreLib.dll
4 participants