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

Adding Infrastructure to run linker on OOB Assemblies by manually calling the Linker #52272

Merged
merged 7 commits into from
May 7, 2021

Conversation

joperezr
Copy link
Member

@joperezr joperezr commented May 4, 2021

This is an alternative to #52133 which instead of dogfooding the instructions we give to third party library devs, we manually call the linker very similar to the way we do it for the shared framework trimming. Either this or #52133 will be merged, but not both. The reason why we try this approach as well is because the problem with #52133 is that it has issues with our custom P2P TFM resolving logic in dotnet/runtime which for some cases it is not resolving the best configuration to build. This PR on the other hand, takes the advantage that we already have a flat list folder where all OOB assemblies with the best configuration are binplaced, so we use that instead and don't need to calculate configurations.

Best reviewed commit-by-commit:

  1. First commit adds the new target that will trim the OOB assemblies and the suppression xmls.
  2. Second commit does some refactoring so that illink-oob.targets and illink-sharedframework.targets can share some common code.
  3. Third commit fixes a couple of build issues in the all configurations and the non-Windows legs, and adds an OOB Exclude list based on comment Adding Infrastructure and post build step to run the linker on our OOB assemblies #52133 (comment) by @eerhardt where he suggested to exclude some OOBs that we don't currently plan to annotate as Trimmable.

cc: @eerhardt @sbomer @krwq @vitek-karas @ViktorHofer

fixes #48488

@dotnet-issue-labeler
Copy link

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

@joperezr joperezr added the linkable-framework Issues associated with delivering a linker friendly framework label May 4, 2021
@ghost
Copy link

ghost commented May 4, 2021

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

Issue Details

This is an alternative to #52133 which instead of dogfooding the instructions we give to third party library devs, we manually call the linker very similar to the way we do it for the shared framework trimming. Either this or #52133 will be merged, but not both. The reason why we try this approach as well is because the problem with #52133 is that it has issues with our custom P2P TFM resolving logic in dotnet/runtime which for some cases it is not resolving the best configuration to build. This PR on the other hand, takes the advantage that we already have a flat list folder where all OOB assemblies with the best configuration are binplaced, so we use that instead and don't need to calculate configurations.

Best reviewed commit-by-commit:

  1. First commit adds the new target that will trim the OOB assemblies and the suppression xmls.
  2. Second commit does some refactoring so that illink-oob.targets and illink-sharedframework.targets can share some common code.

cc: @eerhardt @sbomer @krwq @vitek-karas @ViktorHofer

fixes #48488

Author: joperezr
Assignees: -
Labels:

linkable-framework

Milestone: -

@joperezr joperezr requested a review from eerhardt May 5, 2021 16:32
@eerhardt
Copy link
Member

eerhardt commented May 5, 2021

@sbomer @vitek-karas @marek-safar - do any of you have an issue with taking this approach? It sounds like this is the approach we are going to go down for the OOB libraries.

@sbomer
Copy link
Member

sbomer commented May 6, 2021

No fundamental objection from me, though I would have preferred the other approach if it were possible to get it working (I find the MSBuild code in that one easier to understand since it's less spread out).

<_OOBsToIgnore Include="System.Composition.Runtime" />
<_OOBsToIgnore Include="System.Composition.TypedParts" />
<_OOBsToIgnore Include="System.Configuration.ConfigurationManager" />
<_OOBsToIgnore Include="System.Speech" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd include System.DirectoryServices* and System.Management libraries too

Copy link
Member

Choose a reason for hiding this comment

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

@marek-safar - can you provide the reasoning? I can imagine a self-contained, trimmed Windows app that would use those libraries.

Note: I don't plan on addressing all the ILLink warnings in the OOB libraries that are being analyzed. My current thinking is we address the warnings in the following libraries for 6.0:

  • Microsoft.Extensions.* (used by ASP.NET and Maui)
  • System.Drawing.Common (since it is used by WinForms/Wpf)
  • System.Memory.Data (since it is new, and should be pretty easy)

The rest of the OOBs that we are analyzing will get their warnings baselined, and at least we can ensure no new warnings will be introduced. As we get feedback, we can address the warnings in those libraries post-6.0.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

can you provide the reasoning?

I think they will have very low usage in .NET6 apps

Baselining should be ok but why make the effort if we don't plan to fix them.

Copy link
Member

Choose a reason for hiding this comment

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

I think they will have very low usage in .NET6 apps

Looks like System.DirectoryServices.dll is in the WinForms/WPF shared framework. Looking on NuGet, the 5.0 versions of these libraries have almost 2M downloads each in about half a year.

Baselining should be ok but why make the effort if we don't plan to fix them.

It's not that we don't ever plan on fixing them. Just not in the next 2 months.

Also, it provides value because if someone adds new features/APIs to those libraries that are not trim compatible, they will get a build error.

@ViktorHofer
Copy link
Member

Thanks @joperezr for yet another approach to solve this. I prefer this one over the other as it doesn't create a dependency on the runtime pack and force us to restore it during the product build.

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.

This looks awesome, @joperezr! Thanks for the good work here.

@ghost
Copy link

ghost commented May 7, 2021

Hello @joperezr!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@eerhardt
Copy link
Member

eerhardt commented May 7, 2021

runtime (Libraries Test Run checked coreclr windows x64 Debug) failure is #52464

@joperezr
Copy link
Member Author

joperezr commented May 7, 2021

Merging since the failure is a known issue.

@joperezr joperezr merged commit d3a72e5 into dotnet:main May 7, 2021
@joperezr joperezr deleted the ManuallyLinkOOBAssemblies branch May 7, 2021 20:43
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 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
Development

Successfully merging this pull request may close these issues.

Enable ILLink warning analysis for OOB libraries
6 participants