Adding Infrastructure and post build step to run the linker on our OOB assemblies #52133
Conversation
|
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
added
the
linkable-framework
|
Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer Issue Detailscc: @eerhardt @sbomer @krwq @vitek-karas @ViktorHofer fixes #48488 This is mostly ready for review except that I'm still hitting dotnet/linker#1416 and trying to find the best way to suppress these warnings which for now are causing the build to fail. Once I figure that out I'll remove the 'Draft' from this PR, but sending it out as a draft for now to get initial feedback on the infrastructure
|
joperezr
force-pushed
the
AddOOBTrimmingInfrastructure
branch
from
bc68a3a
to
760489c
Compare
| <MSBuild Targets="Restore" | ||
| Projects="$(MSBuildThisFileDirectory)trimming\trimming.csproj" | ||
| Properties="$(TraversalGlobalProperties);MSBuildRestoreSessionId=$([System.Guid]::NewGuid())" /> |
There was a problem hiding this comment.
Please avoid this extra restore invocation. We worked really hard in the past months / year to reduce extra invocations. If you put the trimming app under a "src" sub-directory it will be restored automatically. Another option would be to add a P2P in pretest.proj trimming.csproj.
There was a problem hiding this comment.
I don't think adding a P2P is the right thing to do in this case, specially since it is a Console application project and not a library, but I'll see how I can avoid this manual restore call.
There was a problem hiding this comment.
That doesn't prevent the P2P. We do have a lot P2Ps to executables i.e. the test runners. Just make sure to set ReferenceOutputAssembly to false.
| @@ -138,5 +138,20 @@ | |||
| RootAttributes="@(FrameworkListRootAttribute)" /> | |||
| </Target> | |||
|
|
|||
| <Target Name="ILLinkTrimOOBLibraries" | |||
There was a problem hiding this comment.
What's the reason that you aren't doing this in src.proj where we also trim the inbox ones?
There was a problem hiding this comment.
Is it that you need a ready to use runtime pack?
There was a problem hiding this comment.
We do the shared framework on src.proj because we are manually invoking the linker and editing the command that we run, while for the OOBs we are instead following the steps we are asking third party library devs to follow but for that to work, we need to have the RuntimeList.xml file already generated in order to be able to build trimming.csproj against the produced shared framework, and that only happens after pretest.proj.
There was a problem hiding this comment.
Is it that you need a ready to use runtime pack?
Yup, exactly
There was a problem hiding this comment.
Hmm does that mean we can't restore before the runtime pack is available? If yes, did we consider treating this as a test instead of the product build? I.e. sourcebuild likely doesn't care about this logic.
| @@ -0,0 +1,17 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <linker> | |||
| <assembly fullname="System.Private.DataContractSerialization, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a"> | |||
There was a problem hiding this comment.
I don't understand this. I thought you fixed this assembly in #50619. Why are we getting these now?
There was a problem hiding this comment.
I think this is because the wrong configuration of System.Private.DataContractSerialization is getting picked. One more reason why not to go with this approach I guess.
| @@ -0,0 +1,161 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <linker> | |||
| <assembly fullname="System.Configuration.ConfigurationManager, Version=6.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51"> | |||
There was a problem hiding this comment.
ConfigurationManager is not trimmable. See #49085. We probably don't need to waste time analyzing this assembly until we try to make it trimmable (which isn't in the near future).
| @@ -0,0 +1,17 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <linker> | |||
| <assembly fullname="System.ComponentModel.TypeConverter, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a"> | |||
There was a problem hiding this comment.
This shouldn't be needed. We have a LibraryBuild.xml file, and should be respecting that
There was a problem hiding this comment.
Actually, in a perfect world, we shouldn't be analyzing the System.ComponentModel.TypeConverter library at all during the OOB trim analysis run.
|
There are a few assemblies here that I think we should set
|
|
Closing this as this has been superseded by #52272 |
ghost
locked as resolved and limited conversation to collaborators
cc: @eerhardt @sbomer @krwq @vitek-karas @ViktorHofer
fixes #48488
This is mostly ready for review except that I'm still hitting dotnet/linker#1416 and trying to find the best way to suppress these warnings which for now are causing the build to fail. That means this PR isn't ready to go in, but sending it out for now to get initial feedback on the infrastructure.
This approach is mostly following the advise we are giving library devs here: https://github.com/dotnet/docs/blob/main/docs/core/deploying/prepare-libraries-for-trimming.md. We are only adding a few additional targets that are accounting for the fact that we have a slightly complex build system, but other than that this is mostly following the guidance on those docs provided by @sbomer