-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add support for aliased cultures #6003
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/Tasks/CultureInfoCache.cs
Outdated
// See https://docs.microsoft.com/en-us/dotnet/api/System.Globalization.CultureInfo.LCID#remarks | ||
const int LOCALE_CUSTOM_UNSPECIFIED = 0x1000; | ||
if (culture.LCID == LOCALE_CUSTOM_UNSPECIFIED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please expand on this a bit? It's not immediately obvious to me from the link why non-system locales should be fully rejected here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this could be further discussed. What do we want? Accept any valid culture or accept known (by the operating system) cultures? If we accept any valid culture then we can get rid of this check altogether.
|
||
Assert.Single(t.AssignedFiles); | ||
Assert.Single(t.CultureNeutralAssignedFiles); | ||
Assert.Equal(String.Empty, t.AssignedFiles[0].GetMetadata("Culture")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the critical difference between this test and the above, right? Can you call it out with a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that the Culture
metadata is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I just mean "Please call out the parts of this test that are different". Here I guess it's not just that Culture is empty but also that the culture-neutral part doesn't get assigned.
bb3815d
to
ee1b9a3
Compare
I believe we should get rid of the whole cache and just use CultureInfo.GetCultureInfo API. are we not using this API for any perf reason? Note that on Windows 10 (on on Linux using ICU), all cultures with the well formed name is considered a valid culture even if the system doesn't have a data for. why we are trying to create a cache at all? CultureInfo.GetCultureInfo already maintaining internal cache for all previously created cultures. |
The .NET Core version of this was introduced in #213, but that was a patch over - try
- {
- // Construct a CultureInfo just to see if its possible.
- new CultureInfo(cultureName);
- validCulture = true;
- }
- catch (ArgumentException)
- {
- // Just eat the exception, but now we know the culture isn't valid.
- }
+ validCulture = CultureStringUtilities.IsValidCultureString(cultureName); So maybe there was a perf issue 18 years ago? I certainly like the simplicity of your proposal. |
@rainersigwald we should give it a try. I think the perf hit would be in invalid cases only which I think is not common. Also, on Windows 10, most of the time will not encounter any invalid cases too. |
I am not familiar with this issue. I will pass it to @tarekgh :) |
Ok. @0xced would you be willing to make that change? |
Sure. I force-pushed and simplified to the maximum! (+77 / -901) The internal static bool IsValidCultureString(string name)
{
try
{
_ = CultureInfo.GetCultureInfo(name);
return true;
}
catch (CultureNotFoundException)
{
return false;
}
} |
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
…rity Co-authored-by: Rainer Sigwald <raines@microsoft.com>
Remove anything "smart" and only rely on `CultureInfo.GetCultureInfo` not throwing `CultureNotFoundException` to deem a culture name valid. Since CultureInfo.GetCultureInfo() returns a cached CultureInfo there should be no performance issue.
19c9e11
to
52f3cbb
Compare
@0xced thanks a lot for your help here. the changes looks good. could you please try to clean the usage of @rainersigwald do we have any perf test can be used to ensure we are not regressing the perf with that? |
This was needed in the .NET Core 1.0 era but CultureInfo.GetCultures and CultureInfo.GetCultureInfo are now available in .NET Core (since version 2.0).
cc @ladipro and @Forgind but not really. We have VS RPS tests but they exercise very small projects and I don't know if they touch this codepath at all. |
I did some cleanup on When doing this cleanup I noticed this comment:
Is it still true that MSBuild needs to target .NET Standard 1.3? |
Nope! Please feel free to remove that :) |
It looks like at some point they decided to throw an error when it hadn't been initialized properly? Not sure why: msbuild/src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs Lines 934 to 982 in c70d520
It goes back to the original commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the idea behind the change is to stop dynamically generating valid cultures and instead use the CultureInfo API (which results in supporting aliased cultures)? This change generally LGTM.
src/Tasks/CultureInfoCache.cs
Outdated
return ValidCultureNames.Contains(name); | ||
try | ||
{ | ||
_ = CultureInfo.GetCultureInfo(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're going from a hashset lookup to try-catching a function call. This will run for every resource in a project, what's the perf impact here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it does a TryGetValue
on a dictionary behind the scenes. https://source.dot.net/#System.Private.CoreLib/CultureInfo.cs,fec605b3b773ab26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't notice the discussion before, it looks like we're okay with this change so long as RPS doesn't catch this.
I also didn't realize that the previous valid cultures are calculated on each run. This seems fine to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try itself is fast as long as it doesn't hit the catch (StackOverflow), and I imagine the cache is small enough that the dictionary lookup will be faster than looking through a list of valid cultures. My biggest worry would be if calling into the runtime causes us to load an assembly we otherwise didn't repeatedly. Not sure about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My biggest worry would be if calling into the runtime causes us to load an assembly we otherwise didn't repeatedly. Not sure about that.
The Globalization code residing inside the core library. so this change wouldn't make any difference. Or are you referring to something else?
I'm a bit lost as to why the Can someone with more MSBuild experience help me to diagnose this issue? |
I wonder if the edit: I'm constantly running into an issue with eng\cibuild_bootstrapped_msbuild.sh not running the second build because of a permission issue so I can't get a local repro. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldly errors read better, we should switch to those while we're modifying these functions.
string result = expander.ExpandIntoStringLeaveEscaped(@"$([System.Globalization.CultureInfo]::GetCultureInfo(`en-US`).ToString())", ExpanderOptions.ExpandProperties, MockElementLocation.Instance); | ||
#else | ||
string result = expander.ExpandIntoStringLeaveEscaped(@"$([System.Globalization.CultureInfo]::new(`en-US`).ToString())", ExpanderOptions.ExpandProperties, MockElementLocation.Instance); | ||
#endif | ||
|
||
Assert.Equal(new CultureInfo("en-US").ToString(), result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're here
Assert.Equal(new CultureInfo("en-US").ToString(), result); | |
result.ShouldBe(new CultureInfo("en-US").ToString()); |
@@ -216,6 +256,30 @@ public void PseudoLocalization(string culture) | |||
Assert.Equal($"MyResource.{culture}.resx", t.AssignedFiles[0].ItemSpec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This applies to the other asserts as well, we should shouldly-ify functions we touch on.
Assert.Equal($"MyResource.{culture}.resx", t.AssignedFiles[0].ItemSpec); | |
t.AssignedFiles[0].ItemSpec.ShouldBe($"MyResource.{culture}.resx"); |
t.Files = new ITaskItem[] { i }; | ||
t.Execute(); | ||
|
||
Assert.Single(t.AssignedFiles); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assert -> Shouldly function calls.
Here's why the tests were failing: MSBuild itself uses MSBuild itself is a good example as to why we should be conservative and accept only aliased cultures in order not to break projects using I have improved this in 4bdca75 to only deem aliased cultures as valid, not all cultures. |
Thinking more about it, maybe we hit another bug because if |
On .NET Framework, ThreeLetterISOLanguageName has two letters for unknown cultures such as "xx".
So, I think I finally found a way to identify predefined cultures, under both ICU and NLS. To deem a culture valid it must either:
A better solution would be to use I also tweaked the test so that they pass on all supported platforms. Supported cultures vary a lot across different platforms! |
Please don't use this. Windows is not promising this will work. That is why we have exposed the other GetCultureInfo overload.
If you use this, please check LOCALE_CUSTOM_DEFAULT too.
Also to clarify, checking LOCALE_CUSTOM_UNSPECIFIED would exclude some legitimate Windows cultures. I didn't follow closely here but would be nice if someone clarify why we care filtering out these cultures? |
Hmmm, but
Do you have an example of such a culture so that I can add it to the tests and make sure it's considered a valid culture?
Just to be conservative and to not accidentally break too much existing projects. MSBuild itself breaks when using |
I assume we are talking when running on Windows. If so, you may call the OS directly for that. something like the call https://source.dot.net/#System.Private.CoreLib/CultureInfo.Nls.cs,14
please run the following code and it will give you such list. foreach (CultureInfo ci in CultureInfo.GetCultures(CultureTypes.AllCultures))
{
if (ci.LCID == 0x1000)
{
Console.WriteLine(ci.Name);
}
}
Is there any other way to include the shared resources without considering it as a culture? I mean can the resource file be renamed to something else (like String_shared.resx for instance)? or we don't have control over that? |
Any recent updates on this? |
Team triage: closing for inactivity. Happy to reopen if we can work through its issues. |
Fixes #3897