Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Call New GetCultureInfo API When Validating Cultures #7853

Merged
merged 15 commits into from
Aug 11, 2022
Merged

Call New GetCultureInfo API When Validating Cultures #7853

merged 15 commits into from
Aug 11, 2022

Conversation

benvillalobos
Copy link
Member

@benvillalobos benvillalobos commented Jul 27, 2022 •
edited
Loading

Fixes #3897

Context

Changes Made

Testing

Notes

Haven't tested this locally.

@benvillalobos benvillalobos marked this pull request as draft July 27, 2022 23:54
@benvillalobos
Copy link
Member Author

Talked with Tarek, turns out we should be using a different API altogether: https://docs.microsoft.com/en-us/dotnet/api/system.globalization.cultureinfo.getcultureinfo?view=net-6.0#system-globalization-cultureinfo-getcultureinfo(system-string-system-boolean)

Forgind
Forgind approved these changes Aug 1, 2022
View reviewed changes
src/Framework/Traits.cs Outdated
/// <summary>
/// https://github.com/dotnet/msbuild/issues/3897
/// </summary>
public readonly bool EnableHardcodedCultureNames = Environment.GetEnvironmentVariable("MSBUILDENABLEHARDCODEDCULTURENAMES") == "1";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were thinking opt out? But I probably misremembered.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this will turn into a change wave instead of a trait

@benvillalobos benvillalobos changed the title Work around culture API difference on windows/unix machines Call New GetCultureInfo API When Validating Cultures Aug 2, 2022
@benvillalobos benvillalobos marked this pull request as ready for review August 2, 2022 17:19
Forgind
Forgind reviewed Aug 2, 2022
View reviewed changes
src/Tasks/CultureInfoCache.cs
@@ -56,6 +57,21 @@ static HashSet<string> InitializeValidCultureNames()
/// <returns>True if the culture is determined to be valid.</returns>
internal static bool IsValidCultureString(string name)
{
#if NET5_0_OR_GREATER
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer more descriptive ifdefs.

src/Tasks/CultureInfoCache.cs Outdated
try
{
// GetCultureInfo throws if the culture doesn't exist
CultureInfo.GetCultureInfo(name, true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This essentially says "skip the hardcoded list on net6," right? Why is that correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardcoded list has always been a workaround to the fact that we didn't have an API that would specifically tell us when a culture was valid or not (CultureInfo.GetCultures(AllCultures) wasn't made for verification purposes). It's not ideal to use the list at all, and this avoids it because starting in net5.0 this GetCultureInfo(name, true) was introduced for specifically this purpose.

The only valid case for us to use the list moving foward would be in net472 for backwards compat when some culture or alias isn't supported. But net472 happens to work because on Windows the API we're currently using happens to contain the cultures that aren't included on linux.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the back compat issue not apply to net6 as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does, NET5_0_OR_GREATER is defined in the SDK and would apply to net5.0+. It happens around here:

https://github.com/dotnet/sdk/blob/dd33b56b1c8ab92cef9fd4173585b9915003e0b8/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets#L207

rainersigwald
rainersigwald reviewed Aug 3, 2022
View reviewed changes
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a test for explicit zh-CN or whatever the most notable problematic alias was?

src/Tasks/CultureInfoCache.cs Outdated Show resolved Hide resolved
src/Tasks/CultureInfoCache.cs Outdated Show resolved Hide resolved
@benvillalobos
Copy link
Member Author

@tarekgh I tried out a few test cases for aliases in codingdinosaur's writeup, and I'm seeing interesting results:

image

The good news is they both see zh-TW as a valid culture, which is strictly improved from before 😄 any thoughts on the failing cases? are those aliases expected not to be detected?

@tarekgh
Copy link
Member

tarekgh commented Aug 4, 2022

@benvillalobos the failing cases is because the culture name using _ and not -.

rainersigwald
rainersigwald approved these changes Aug 4, 2022
View reviewed changes
src/Tasks.UnitTests/AssignCulture_Tests.cs Outdated
@@ -218,5 +218,26 @@ public void PseudoLocalization(string culture)
Assert.Equal($"MyResource.{culture}.resx", t.AssignedFiles[0].ItemSpec);
Assert.Equal("MyResource.resx", t.CultureNeutralAssignedFiles[0].ItemSpec);
}

/// <summary>
/// Testing that certain aliases are considered valid cultures.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Testing that certain aliases are considered valid cultures.
/// Testing that certain aliases are considered valid cultures. Regression test for https://github.com/dotnet/msbuild/issues/3897.

@benvillalobos
Copy link
Member Author

the failing cases is because the culture name using _ and not -.

thanks!

@benvillalobos
Copy link
Member Author

This is the first I've seen CanShutdownServerProcess fail. This might be a flaky test, but it's too early to tell. It looks very unrelated to what this change is about.

@Forgind
Copy link
Member

Forgind commented Aug 8, 2022

@benvillalobos, I told it to rerun. I suspect flakiness.

@rokonec, see BenVillalobos's comment—second instance today.

@benvillalobos benvillalobos added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Aug 8, 2022
@rokonec rokonec merged commit 8b24a1b into dotnet:main Aug 11, 2022
@tarekgh tarekgh mentioned this pull request Aug 31, 2022
Closed
@benvillalobos benvillalobos mentioned this pull request Sep 28, 2022
Merged
@rainersigwald rainersigwald mentioned this pull request May 3, 2023
Open
@maltalex maltalex mentioned this pull request Sep 8, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Forgind Forgind Forgind approved these changes

@rainersigwald rainersigwald rainersigwald approved these changes

Assignees
No one assigned
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cultures aliased by ICU cannot be used for resource localization on non-Windows environments
5 participants
@benvillalobos @tarekgh @Forgind @rainersigwald @rokonec