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.

Sign up for GitHub

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

Annotate framework assemblies as trimmable #48428

Merged
merged 2 commits into from
Feb 19, 2021
Merged

Annotate framework assemblies as trimmable #48428

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9caaa7
Annotate framework assemblies as trimmable
sbomer Feb 17, 2021
d03327e
Add a condition to let projects opt out
sbomer Feb 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions eng/versioning.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
<AssemblyMetadata Include="PreferInbox">
<Value>True</Value>
</AssemblyMetadata>
<AssemblyMetadata Include="IsTrimmable">
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we want to do this blindly? It feels like at least there should be a Condition to allow individual assemblies to turn it off.

Note that today we only analyze the assemblies in the shared fx / runtimepack for ILLink warnings. All OOB assemblies go unanalyzed. Once we have a story for analyzing libraries, we should hook dotnet/runtime up to it.

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 can add a condition. Are there any in particular that you think I should turn off to start with? (Would it maybe make sense to turn it off for OOB assemblies and opt them in individually when we analyze them?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, note that the feature as designed isn't tied to the warnings inherently - the attribute just says "trim me" - whether the assembly is linker-safe or not. That said, maybe it makes sense to opt out of trimming in cases where we don't trim a library during the runtime build because it uses too much reflection, like this:

runtime/src/libraries/System.Private.DataContractSerialization/src/System.Private.DataContractSerialization.csproj

Lines 7 to 8 in 75d5cc3

<!-- Too much private reflection. Do not bother with trimming -->
<ILLinkTrimAssembly>false</ILLinkTrimAssembly>

Copy link
Contributor

Choose a reason for hiding this comment

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

All libraries need to be trimmable in 6.0 but I'd probably exclude netfx build targets

Copy link
Member

@eerhardt eerhardt Feb 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

All libraries need to be trimmable in 6.0

I agree we want to get there. But I think we should do it in a pragmatic way. Turning it on for all assemblies and letting people tell us what breaks doesn't seem like the best approach.

My thinking is that once we are done annotating the shared fx libraries, we start analyzing ILLink warnings for OOB libraries. When each library is "clean" of ILLink warnings, we mark it as "Trimmable".

For the initial change, I was assuming we would analyze System.IO.Pipelines, and assuming it is clean, we mark it as "IsTrimmable".

like this: System.Private.DataContractSerialization.csproj

That one is part of the shared fx, so it gets trimmed today in a user's app 🙃. When we get to that assembly, that comment and line will go away and we will trim it as part of the build like every other shared fx library.

Copy link
Member Author

Choose a reason for hiding this comment

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

Per discussion with @eerhardt, the plan is to opt all assemblies in as suggested by @marek-safar - and there's a new condition that we can use to opt out per project, should we need to.

By following the existing pattern for AssemblyMetadata like "Serviceable" and "PreferInbox", the attribute will be placed in src and ref assemblies, and also in assemblies targeting netstandard/netcoreapp/netfx. We need to include it for netstandard/netcoreapp because plenty of OOB assemblies build for those targets. Including the attributes in netfx assemblies shouldn't cause any issues since there are no common scenarios where an app targeting net6.0 references one of our netfx assemblies. I think it makes sense to be consistent with existing attributes on this, but @marek-safar if you prefer I could also disable it for netfx.

Copy link
Member

Choose a reason for hiding this comment

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

and ref assemblies

Are ref assemblies ever trimmed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to be consistent with existing attributes on this, but @marek-safar if you prefer I could also disable it for netfx.

I'm not against it, just wanted to be more cautious for netfx to not break bad code which could, for example, read only the last AssemblyMetadata but the chance is low.

Copy link
Member

Choose a reason for hiding this comment

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

Are ref assemblies ever trimmed?

No. But they are also never "serviced", and yet we are marking them with "Serviceable" assembly metadata. We should consistently apply these attributes across the assemblies.

Copy link
Member

Choose a reason for hiding this comment

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

No. But they are also never "serviced", and yet we are marking them with "Serviceable" assembly metadata. We should consistently apply these attributes across the assemblies.

FWIW starting .NET 5, we can now service the ref pack, we try to avoid it but we added support to be able to do it in case we need it, but I'm fine with adding IsTrimmable to them for consistency.

<Value>True</Value>
</AssemblyMetadata>
</ItemGroup>

<!-- Adds SupportedOSPlatform attribute for Windows Specific libraries -->
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Private.CoreLib/src/Internal/AssemblyAttributes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,6 @@

[assembly: AssemblyMetadata("Serviceable", "True")]
[assembly: AssemblyMetadata(".NETFrameworkAssembly", "")]
Copy link
Member

Choose a reason for hiding this comment

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

Completely unrelated to this PR, but it caught my eye... is this value still desired? Should the "Framework" piece be removed?
cc: @ericstj

Copy link
Member

Choose a reason for hiding this comment

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

Certain things look for this string. ex.

https://github.com/dotnet/corert/blob/24f1edc938af9954eac9a3385958787ca64956d5/src/ILCompiler.Compiler/src/Compiler/FrameworkStringResourceBlockingPolicy.cs#L58-L87

[assembly: AssemblyMetadata("IsTrimmable", "True")]

[assembly: NeutralResourcesLanguage("en-US")]