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.
dotnet-issue-labeler
bot
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" /> |
There was a problem hiding this comment.
This is #95948 as a proof that it works.
| @@ -37,7 +37,7 @@ | |||
| <ILLinkRewritePDBs Condition="'$(ILLinkRewritePDBs)' == ''">true</ILLinkRewritePDBs> | |||
|
|
|||
| <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.
| 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.
| @@ -9,7 +9,8 @@ | |||
| <!-- DesignTimeBuild requires all the TargetFramework Derived Properties to not be present in the first property group. --> | |||
| <PropertyGroup> | |||
| <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).
| 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.
MichalStrehovsky
changed the title
Fixes #102303.
@dotnet/illink