Remove AssemblyMetadata(".NETFrameworkAssembly", "") attributes
#89490
Conversation
This is likely a .NET Native leftover that is currently (ironically) breaking .NET Native. This attribute indicates to the .NET Native compiler that the assembly can be trimmed. We're currently setting this on assemblies that no longer carry RD.XML to make this safe. Without this attribute, the assembly will be treated like any other and not trimmed. Fixes dotnet#44697.
|
Tagging subscribers to this area: @dotnet/area-meta Issue DetailsThis is likely a .NET Native leftover that is currently breaking .NET Native. This attribute indicates to the .NET Native compiler that the assembly can be trimmed. We're currently setting this on assemblies that no longer carry RD.XML to make this safe. Without this attribute, the .NET Native compiler will root the assembly and we should no longer see trimming issues with it. Cc @eerhardt @tarekgh @ericstj Fixes #44697.
|
|
@tarekgh can you please weigh in on this since it looks like you initially added it. Do you recall who needed it and if we should let them know about removing it? |
|
Oh, here's the usage: https://github.com/dotnet/coreclr/blob/1b04187394cd1de5b316c40a3ad9d9189f4cb5b0/src/System.Private.CoreLib/shared/System/Resources/ResourceManager.Uap.cs#L94 I also see some other usage from tools. |
|
Yes, there are number of ways the core framework assemblies are special cased by .NET Native for UWP toolchain, most of it has to do with trimming. The idea behind this PR is that it is better to make the OOB assemblies look like ordinary user assemblies for .NET Native for UWP toolchain since we do not test that the special handling of OOB assemblies works correctly, and it does not actually work correctly. |
|
Just curious (I might very well be wrong here): is this PR removing the attribute from all OOB packages and not just from that |
|
Yes, this PR is removing this attribute from all OOB assemblies build in this repo. |
|
I see. I guess I can make get some size diffs once this is merged and the next daily packages are out. I'm especially curious to see whether System.Text.Json 8.0 will regress much, as that in particular is showing some amazing size improvements on 8.0 🤔 |
It will improve compatibility and regress size with default rd.xml. It should not affect the size much with custom .rd.xml that I assume you are on. |
Got it. Yeah we're using a custom .rd.xml (I mean it's just empty, minus a couple targeted fixes for a handful of types). Should we take extra care to validate that there's no regressions for us (as in, things crashing at runtime) when we upgrade to the 8.0 packages then, due to these changes, or does the fact we already had global trimming enabled for us (as in, no |
Correct. |
|
Sweet, thank you for the additional info Jan! 🙂 |
|
So it looks like this attribute had a few purposes - it's clear the rd.xml case is no longer relevant since all the rd.xml's were removed. What about the resource loader one? IIRC this was to allow the framework assemblies to still go through the embedded .NET resource loader instead of the WinRT loader which read the pri file in the application. I think since this special casing was only present for the UAP corelib, and the only way UAP will see these assemblies is through nuget packages brought in with the app - which should go through MSBuild pri file conversion - it should be OK to remove this. Does that sound right @tarekgh? |
UAP testing was nuked 4 years ago in dotnet/corefx#41759. We don't test any of this or we'd have found out the Microsoft.Extensions.* assemblies don't work. UAP gets netstandard2.0 assemblies and crosses fingers that it works (we do a better job of validating these for .NET Framework). This attribute is used within the .NET Native compiler for many purposes. One of them is for example framework exception message resource stripping, the protocol for which was this attribute and the shape of the SR class. I'm pretty sure we broke the shape of the SR class that this was relying on. We've also deleted RD.XML files that shipped as part of the framework (#59201) because we stopped embedding them without anyone noticing, and they all bitrotted anyway (comes down to the nuked testing again - all of this became unmaintainable when the testing was deleted and we should have deleted all .NET Native special casing at that point and have .NET Native treat these as any other assembly). |
|
I'm in agreement with you @MichalStrehovsky - just trying to do due-diligence on this before removing since it is somewhat of a generic attribute that could be used by a lot of tools. You had one reason to remove it and could say it was the right thing to do in that case - to help unblock a UAP scenario. I found another case and it also seemed the right thing to do for UAP - so it's supporting the case of removal. I do see some other random usage of this attribute on github -- probably folks using it because they noticed it was there rather than documentation telling them to use or what it meant. Also the DevDiv codebase has a few hits on this and related APIs exposed from CCI libraries. Most looked related to .NETNative. It might help to give these a look and make sure that nothing stands out to you as concerning. |
ghost
locked as resolved and limited conversation to collaborators
This is likely a .NET Native leftover that is currently breaking .NET Native. This attribute indicates to the .NET Native compiler that the assembly can be trimmed.
We're currently setting this on assemblies that no longer carry RD.XML to make this safe. Without this attribute, the .NET Native compiler will root the assembly and we should no longer see trimming issues with it.
Cc @eerhardt @tarekgh @ericstj
Fixes #44697.