Don't trim PNSE assemblies and enable trimming for inbox assets on all .NETCoreApp versions #58345
Conversation
Don't trim platform not supported assemblies, since even in "library" trimming mode, the trimmer might remove some non-public members if they are not called within the assembly. In the case of PNSE assemblies this is most likely always the case.
|
Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer Issue DetailsDon't trim platform not supported assemblies, since even in "library" trimming mode, the trimmer might remove some non-public members if they are not called within the assembly. In the case of PNSE assemblies this is most likely always the case.
|
ViktorHofer
changed the title
|
Why are we doing this? If the issue is that implementations of public interfaces are getting trimmed as part of the library build, we should fix the linker / linker configuration to not do that. I don't think we should disable trimming entirely for these PNSE assemblies just because of that. If there are other reasons to disable the trimming, I'd like to understand them. |
|
A PNSE assembly has the same compilation input as a reference assembly, the reference source (but transformed to throw PNSE). If we don't leverage the linker for one (ref), why would we want to use it for the other (PNSE) when the compilation input is nearly identical? A PNSE assembly throws at runtime regardless, so why is it important if it's "library" linked or not? From a size perspective, |
|
If we think that the size reduction makes sense then I'm happy to close this in favor of fixing the "library" linker mode. Note the discussion in #58343 which revealed that many shipping .NETCoreApp assets aren't trimmed. |
|
Actually, let me close this right now. Thanks for raising your concern @stephentoub, I agree with you on this. Fortunately this revealed a bigger issue which we will follow-up on as part of #58343. |
ghost
locked as resolved and limited conversation to collaborators
|
/azp run runtime |
1 similar comment
|
/azp run runtime |
|
After quite a long discussion in, dotnet/linker#2238 we came to the conclusion that trimming PNSE assemblies is not worth the cost (extra linker invocations). The failing linker tests are known and already handled by #60836 and others. |
| '$(GeneratePlatformNotSupportedAssembly)' != 'true' and | ||
| '$(GeneratePlatformNotSupportedAssemblyMessage)' == ''">true</ILLinkTrimAssembly> |
There was a problem hiding this comment.
Why do we need to check both GeneratePlatformNotSupportedAssembly and GeneratePlatformNotSupportedAssemblyMessage? Why isn't '$(GeneratePlatformNotSupportedAssembly)' != 'true' enough?
There was a problem hiding this comment.
Because the corresponding condition in GenFacades is an OR: https://github.com/dotnet/arcade/blob/f08de5892cab40c9a0e94dcc54b4537763420494/src/Microsoft.DotNet.GenFacades/build/Microsoft.DotNet.GenFacades.targets#L10-L11
Don't trim platform not supported assemblies, since even in "library" trimming mode, the trimmer might remove some non-public members if they are not called within the assembly. In the case of PNSE assemblies this is most likely always the case.
Contributes to #58343
More information in dotnet/linker#2238