Couple fixes for UseSystemResourceKeys#103463
Conversation
…blish time Fixes dotnet#96539. Fixes dotnet#102303. Two possible approaches were discussed in dotnet#96539 but doing it in illinker feels more straightforward.
ghost
added
the
area-Tools-ILLink
|
Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas |
| <resource name="{StringResourcesName}.resources" action="remove" /> | ||
| <type fullname="System.SR"> | ||
| <method signature="System.Boolean UsingResourceKeys()" body="stub" value="true" /> | ||
| <method signature="System.Boolean GetUsingResourceKeysSwitchValue()" body="stub" value="true" /> |
|
|
||
| <ILLinkResourcesSubstitutionIntermediatePath>$(IntermediateOutputPath)ILLink.Resources.Substitutions.xml</ILLinkResourcesSubstitutionIntermediatePath> | ||
| <GenerateResourcesSubstitutions Condition="'$(GenerateResourcesSubstitutions)' == '' and '$(StringResourcesPath)' != ''">true</GenerateResourcesSubstitutions> | ||
| <GenerateResourcesSubstitutions Condition="'$(GenerateResourcesSubstitutions)' == '' and '$(StringResourcesPath)' != '' and '$(OmitResources)' != 'true'">true</GenerateResourcesSubstitutions> |
There was a problem hiding this comment.
OmitResources is an existing mechanism to prevent generating resx. It was a bug that we'd still generate a substitution file when this was set.
| ' The trimming tools are also capable of replacing the value of this method when the application Is being trimmed. | ||
| Public Shared Function UsingResourceKeys() As Boolean | ||
| Return False | ||
| Return s_usingResourceKeys |
There was a problem hiding this comment.
I synchronized SR.vb with SR.cs that we apparently neglected for years.
| <TargetPlatformIdentifier>$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)'))</TargetPlatformIdentifier> | ||
| <GeneratePlatformNotSupportedAssemblyMessage Condition="'$(TargetPlatformIdentifier)' == ''">SR.DiagnosticsFileVersionInfo_PlatformNotSupported</GeneratePlatformNotSupportedAssemblyMessage> | ||
| <GeneratePlatformNotSupportedAssemblyMessage Condition="'$(TargetPlatformIdentifier)' == ''">SR.DiagnosticsFileVersionInfo_PlatformNotSupported</GeneratePlatformNotSupportedAssemblyMessage> | ||
| <OmitResources Condition="'$(GeneratePlatformNotSupportedAssemblyMessage)' == ''">true</OmitResources> |
There was a problem hiding this comment.
Bugfix for a customer reported issue linked from top post that I ran into myself while working on this (not sure if it's needed in this PR or it could be separated out, but I sure don't want to rebuild libraries to see if I still get a warning).
The problem was that only the PlatformNotSupported flavor of the assembly needs resource strings, but we were generating the SR class/resources/substitution anyway. Customers were getting warnings for this (see the issue for the warning).
sbomer
left a comment
sbomer
left a comment
There was a problem hiding this comment.
Code changes LGTM but I wonder if we actually need this (see my other comment).
| private static readonly bool s_usingResourceKeys = GetUsingResourceKeysSwitchValue(); | ||
|
|
||
| // This method is a target of ILLink substitution. | ||
| private static bool GetUsingResourceKeysSwitchValue() => AppContext.TryGetSwitch("System.Resources.UseSystemResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false; |
There was a problem hiding this comment.
I think this could be changed to use FeatureSwitchDefinition, and then instead of featuredefault, set the defaults in Microsoft.NET.ILLink.targets. Do you see any problems with that?
If possible I'd prefer to move away from the XML for feature switches. If we can do so here, I wonder if we still need #96539 at all.
There was a problem hiding this comment.
I also realized that featuredefault is potentially problematic because it could result in feature switches being substituted even without the corresponding AppContext setting. I'm starting to think we should avoid featuredefault entirely.
There was a problem hiding this comment.
I think this could be changed to use
FeatureSwitchDefinition, and then instead of featuredefault, set the defaults inMicrosoft.NET.ILLink.targets. Do you see any problems with that?
Much simpler. Surprising nobody thought of this in the issue, we already had the facilities. FeatureSwitchDefinition didn't look necessary so I didn't add it - it wasn't clear to me what it would buy here.
There was a problem hiding this comment.
FeatureSwitchDefinition could let us get rid of some XML, but isn't necessary.
sbomer
left a comment
sbomer
left a comment
There was a problem hiding this comment.
LGTM, thank you! (PR title should be updated)
MichalStrehovsky
changed the title
2 participants
Fixes #102303.
@dotnet/illink