Uh oh!
There was an error while loading. Please reload this page.
Stop using LocalizationResources within framework - #86430
Conversation
Fixesdotnet#81459. I moved the messages to `EventAttribute.Message` which I believe should be equivalent, but I would appreciate if someone could double-check. I also removed all the now-unused resource strings. I believe they all need to be prefixed by one of well-known prefixes. I would appreciate if someone could double-check.
ghost
commented
May 18, 2023
Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas Issue DetailsFixes #81459. I moved the messages to Cc @noahfalk@brianrob@eerhardt @dotnet/ilc-contrib
|
| Name = "System.Threading.Tasks.TplEventSource", | ||
| Guid = "2e5dba47-a3d2-4d16-8ee0-6671ffdcd7b5", | ||
| LocalizationResources = | ||
| #if CORECLR |
There was a problem hiding this comment.
This was set up in dotnet/coreclr#22120. I don't think we really want to keep it this way, so I fixed that by adding Message unconditionally.
MichalStrehovsky
commented
May 18, 2023
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| } | ||
| } | ||
| } | ||
| // End of workaround for https://github.com/dotnet/runtime/issues/81459 |
There was a problem hiding this comment.
We don't still want this for cases where a 3rd-party uses LocalizationResources?
There was a problem hiding this comment.
We don't remove resources in user assemblies so this should not be needed for those unless the user does the same gestures to get them removed.
brianrob
commented
May 18, 2023
@MichalStrehovsky, if I understand correctly, the motivation for this change is that NativeAOT strips localized resources from framework assemblies, and so these strings aren't usable anyway within NativeAOT. Do I have that right? If so, the non-localized messages seem fine for NativeAOT, but AOT is also affected here. How should I think about this change in the context of non-AOT? |
MichalStrehovsky
commented
May 18, 2023
This also affects non-aot - we have a documented non-aot trimming option to strip framework resources when trimming. The assumption for that was that resource strings are only used for exception messages, so we replace them with resource keys. Aot can sometimes do this more aggressively so I found this in aot testing, but this problem is reachable with normal trimming too. The issue has more context. |
brianrob
commented
May 18, 2023
Thanks @MichalStrehovsky for pointing me to the issue. It turns out that I already had the exact same thoughts and we discussed them already. The change looks good to me. From your notes at the beginning of the PR, yes, Message should be equivalent, it just won't be localized: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs#L4805-L4826. |
Fixes#81459.
I moved the messages to
EventAttribute.Messagewhich I believe should be equivalent, but I would appreciate if someone could double-check. I also removed all the now-unused resource strings. I believe they all need to be prefixed by one of well-known prefixes. I would appreciate if someone could double-check.Cc @noahfalk@brianrob@eerhardt @dotnet/ilc-contrib