Skip to content

Conversation

@sbomer
Copy link
Member

@sbomer sbomer commented Jul 28, 2025

This lets the logic from the ILLink targets add AssemblyMetadata("IsTrimmable", "true") to ref projects which have trimming enabled.

Also moves the trimming opt-out into Directory.Build.props when it should be shared between src and corresponding ref projects.

@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

@sbomer sbomer requested review from a team and ViktorHofer July 28, 2025 23:21
@sbomer sbomer marked this pull request as ready for review July 28, 2025 23:22
Copilot AI review requested due to automatic review settings July 28, 2025 23:22
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enables ILLink targets to add AssemblyMetadata("IsTrimmable", "true") to reference (ref) projects that have trimming enabled by importing the illink.targets for ref projects. The main change consolidates trimming opt-out configuration by moving IsTrimmable>false</IsTrimmable> settings from individual source project files to their corresponding Directory.Build.props files, ensuring the setting is shared between source and reference projects.

Key changes:

  • Imports illink.targets for reference assembly projects in addition to source projects
  • Moves trimming opt-out declarations from individual .csproj files to Directory.Build.props files
  • Maintains existing trimming behavior while enabling proper metadata for ref projects

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/libraries/Directory.Build.targets Updates condition to import illink.targets for reference assembly projects
src/libraries/System.Speech/src/System.Speech.csproj Removes IsTrimmable>false</IsTrimmable> declaration
src/libraries/System.Speech/Directory.Build.props Adds IsTrimmable>false</IsTrimmable> declaration
src/libraries/System.Configuration.ConfigurationManager/src/System.Configuration.ConfigurationManager.csproj Removes trimming opt-out and comment
src/libraries/System.Configuration.ConfigurationManager/Directory.Build.props Adds trimming opt-out with comment
src/libraries/System.ComponentModel.Composition/src/System.ComponentModel.Composition.csproj Removes IsTrimmable>false</IsTrimmable> declaration
src/libraries/System.ComponentModel.Composition/Directory.Build.props Adds IsTrimmable>false</IsTrimmable> declaration
src/libraries/System.ComponentModel.Composition.Registration/src/System.ComponentModel.Composition.Registration.csproj Removes IsTrimmable>false</IsTrimmable> declaration
src/libraries/System.ComponentModel.Composition.Registration/Directory.Build.props Adds IsTrimmable>false</IsTrimmable> declaration
src/libraries/System.CodeDom/src/System.CodeDom.csproj Removes IsTrimmable>false</IsTrimmable> declaration
src/libraries/System.CodeDom/Directory.Build.props Adds IsTrimmable>false</IsTrimmable> declaration

@ViktorHofer
Copy link
Member

Can you please provide context on why this is necessary now? Reference assemblies never had that assembly attribute on them. Just for my own education.

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Assuming that the import of illink.targets by ref projects don't trigger any additional work. LGTM.

@sbomer
Copy link
Member Author

sbomer commented Jul 29, 2025

Should've mentioned in the PR description - this is for #117712. We want to have an option for the Roslyn analyzer to warn if there are references to assemblies that aren't marked IsTrimmable.

@sbomer sbomer merged commit f9fb081 into dotnet:main Jul 29, 2025
88 checks passed
@ViktorHofer
Copy link
Member

This implies that the contract (the reference assembly) now governs whether an implementation must be AOT compatible. We should probably document that. cc @ericstj @bartonjs

@sbomer
Copy link
Member Author

sbomer commented Jul 29, 2025

I'm planning to add docs for the trim/aot behavior. Is there another doc that should be updated for ref assemblies in general?

@ericstj
Copy link
Member

ericstj commented Jul 29, 2025

Why do we want to trim the reference assemblies? Seems like that can hide bugs in the linker like @tannergooding recently discovered. We don't have anything in the reference projects that we need the linker to trim, I'm not certain what the motivation for this change is.

@MichalStrehovsky
Copy link
Member

Why do we want to trim the reference assemblies? Seems like that can hide bugs in the linker like @tannergooding recently discovered. We don't have anything in the reference projects that we need the linker to trim, I'm not certain what the motivation for this change is.

We don't want to trim them, just mark them as trimmable (including ILLink.targets generates the attribute based on IsTrimmable property). Context is in the above mentioned #117712.

@sbomer sbomer deleted the isTrimmableRefAsm branch August 7, 2025 17:38
@github-actions github-actions bot locked and limited conversation to collaborators Sep 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants