-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Adding generation of substitution files for trimming out resources when feature switch is present #38397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding generation of substitution files for trimming out resources when feature switch is present #38397
Changes from all commits
533f5ee
2899fe4
9b493ee
35a4164
9e634e3
576e14d
507fb88
e3f19ac
ccdf0a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| <linker> | ||
| <!-- System.Resources.UseSystemResourceKeys removes resource strings and instead uses the resource key as the exception message --> | ||
| <assembly fullname="{AssemblyName}" feature="System.Resources.UseSystemResourceKeys" featurevalue="true"> | ||
| <!-- System.Resources.UseSystemResourceKeys removes resource strings and instead uses the resource key as the exception message --> | ||
| <resource name="{StringResourcesName}.resources" action="remove" /> | ||
| <type fullname="System.SR"> | ||
| <method signature="System.Boolean UsingResourceKeys()" body="stub" value="true" /> | ||
| </type> | ||
| </assembly> | ||
| </linker> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ internal static partial class SR | |
| // Needed for debugger integration | ||
| internal static string GetResourceString(string resourceKey) | ||
| { | ||
| return GetResourceString(resourceKey, string.Empty); | ||
| return GetResourceString(resourceKey, null); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tarekgh I made this change due to the discussion #37587 (comment) Do you mind taking a look at this?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to get rid of this file and use the one in the common? we can look quickly at the differences. both should be providing the same logic. or if you want we can do that in a separate PR.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file is extension of the common file, we cannot get rid of it
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the discussion on that issue it may be that it there is a problem as some codegen from arcade might be calling this method specifically so removing might not be so simple, although I may have misinterpreted the discussion. I would prefer to scope the change to only this for now so that if Arcade gets broken by removing the method we don't have to revert this whole PR
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I may have misread but I thought what Tarek meant was more to remove this method, in favor of the one in the other overload defined in Common.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if this is the case, the SR.cs that in the common already have the method: internal static string GetResourceString(string resourceKey, string? defaultString = null)so I guess we should delete GetResourceString from here then. In reply to: 445786809 [](ancestors = 445786809)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also the SR.cs in common is just ifdef stuff for corelib which we can get rid of now as the ResourceManager became a part of corelib already. that means we can get rid of SR.cs which is in the corelib. as I mentioned we can make this in other PR if we need to. In reply to: 445792039 [](ancestors = 445792039,445786809)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think such change should be part of this PR |
||
| } | ||
|
|
||
| private static string InternalGetResourceString(string key) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using Microsoft.DotNet.RemoteExecutor; | ||
| using Xunit; | ||
|
|
||
| namespace System.Runtime.Tests | ||
| { | ||
| public class UseResourceKeysTest | ||
| { | ||
| [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] | ||
| public void ReturnsResourceKeyWhenFeatureSwitchIsEnabled() | ||
| { | ||
| RemoteInvokeOptions options = new RemoteInvokeOptions(); | ||
| options.RuntimeConfigurationOptions.Add("System.Resources.UseSystemResourceKeys", true); | ||
|
|
||
| RemoteExecutor.Invoke(() => | ||
| { | ||
| try | ||
| { | ||
| throw new AggregateException(); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Assert.Equal("AggregateException_ctor_DefaultMessage", e.Message); | ||
| } | ||
| }, options).Dispose(); | ||
| } | ||
|
|
||
| [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] | ||
| public void ReturnsResourceWhenFeatureSwitchIsDisabled() | ||
| { | ||
| RemoteInvokeOptions options = new RemoteInvokeOptions(); | ||
| options.RuntimeConfigurationOptions.Add("System.Resources.UseSystemResourceKeys", false); | ||
|
|
||
| RemoteExecutor.Invoke(() => | ||
| { | ||
| try | ||
| { | ||
| throw new ArgumentException(); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Assert.NotEqual("AggregateException_ctor_DefaultMessage", e.Message); | ||
| } | ||
| }, options).Dispose(); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| using System; | ||
|
|
||
| class Program | ||
| { | ||
| static int Main(string[] args) | ||
| { | ||
| try | ||
| { | ||
| throw new AggregateException(); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| // When the Trimmer recieves the feature switch to use resource keys then exception | ||
| // messages shouldn't return the exception message resource, but instead the resource | ||
| // key. This test is passing in the feature switch so we make sure that the resources | ||
| // got trimmed correctly. | ||
| if (e.Message == "AggregateException_ctor_DefaultMessage") | ||
| { | ||
| return 100; | ||
| } | ||
| else | ||
| { | ||
| return -1; | ||
| } | ||
| } | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.