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

Support Windows' pseudo-locales (#3653) #3654

Merged
merged 3 commits into from Aug 28, 2018

Conversation

Projects
None yet
3 participants
@martincostello
Copy link
Contributor

martincostello commented Aug 25, 2018

As described in #3653, MSBuild does not produce satellite resource assemblies for Windows' pseudo-locales (at least on systems running Windows 10, Version 1803 and later).

This is because they are not returned by the OS as supported cultures. From the documentation:

For Windows 10, version 1803, editing the Windows Registry like this has no effect. But you can still call the non-enumerating NLS APIs with the names of the pseudo-locales (see the code examples above) to populate your user interface (UI).

The ability to do this is useful where developers of an application wish to use the built-in Windows pseudo-locales to test their own applications' localisation support.

This PR special-cases the 4 pseudo-locales in CultureInfoCache so that they are considered to be valid cultures. This then allows the AssignCulture task to populate the Culture and WithCulture metadata for any EmbeddedResource item for these cultures so that a satellite resource assembly is compiled for the application to use.

The relevant bits of code this change affects are shown below:

AssignedFiles[i].SetMetadata("Culture", info.culture);
AssignedFiles[i].SetMetadata("WithCulture", "true");

<AssignCulture Files="@(EmbeddedResource)" Condition="'%(Extension)'!='.licx'">
<!-- Create the list of culture resx and embedded resource files -->
<Output TaskParameter="AssignedFilesWithCulture" ItemName="_MixedResourceWithCulture"/>
<!-- Create the list of non-culture resx and embedded resource files -->
<Output TaskParameter="AssignedFilesWithNoCulture" ItemName="_MixedResourceWithNoCulture"/>
</AssignCulture>

<_SatelliteAssemblyResourceInputs Include="@(EmbeddedResource->'%(OutputResource)')" Condition="'%(EmbeddedResource.WithCulture)' == 'true' and '%(EmbeddedResource.Type)' == 'Resx'" />

This PR also updates the values in the HardcodedCultureNames property of CultureInfoCache with the 4 pseudo-locales and an updated list of values generated by my local Windows 10 OS.

The original source of the list has been removed (aspnet/Localization#323) as well as the tool that generated it (aspnet/Localization#130), so I re-generated it using that code locally in a throw-away project. I've only added new entries generated by the list. Any values that weren't generated I've left in place.

Compiling this repro application with the changes made to MSBuild by this PR, the qps-ploc/PseudoLocales.resources.dll satellite resource assembly is correctly compiled and used by the application when run for that culture.

@rainersigwald
Copy link
Contributor

rainersigwald left a comment

I have a few nitpicks, but this looks great, thanks!

@AndyGerlicher What do you think about taking this for 15.9? 1803+ users will only increase, and we do want to encourage loc . . .

{
if (!ValidCultureNames.Contains(pseudoLocale))
{
ValidCultureNames.Add(pseudoLocale);

This comment has been minimized.

@rainersigwald

rainersigwald Aug 27, 2018

Contributor

Since it's a HashSet, could you just do this unconditionally?

Actually, you could probably just do ValidCultureNames.UnionWith(pseudoLocales). Though there's a foreach above so this being implemented the same way is nice.

This comment has been minimized.

@martincostello

martincostello Aug 27, 2018

Contributor

I was just being conservative in case a different (older) version of Windows to the one I'm running did return the pseudo locales and then threw an exception if it was already in there when it was added. Happy to amend if adding when already present would be safe.

Do you want me to retain the foreach or use the UnionWith()?

This comment has been minimized.

@rainersigwald

rainersigwald Aug 27, 2018

Contributor

HashSet<T>.Add can be called for the same item many times. Its return value indicates whether it was actually added or already present, but we don't care in this case, so it's fine to ignore it. I prefer this because it's easier to read and reason about, and reduces indenting.

Let's just keep the foreach. It's consistent and evidently that's all UnionWith does anyway: https://github.com/dotnet/corefx/blob/58a310782b805df19a3939b12544c39296db5ed3/src/System.Collections/src/System/Collections/Generic/HashSet.cs#L507-L518

This comment has been minimized.

@martincostello

martincostello Aug 27, 2018

Contributor

Sorry, not quite sure from the comment. So do you just want me to drop the Contains()?

This comment has been minimized.

@rainersigwald

rainersigwald Aug 27, 2018

Contributor

Correct.

This comment has been minimized.

@martincostello

martincostello Aug 27, 2018

Contributor

Fixed in ce49c38.

// Regenerated using the tool (removed by https://github.com/aspnet/Localization/pull/130)
// * Removed the empty string from the list
// * Any cultures not present when regenerated were retained
// * Added the Windows pseudo-locales

This comment has been minimized.

@rainersigwald

rainersigwald Aug 27, 2018

Contributor

Should these be added to this list when they're going to be added above to handle the dynamic list-of-cultures case?

This comment has been minimized.

@martincostello

martincostello Aug 27, 2018

Contributor

When HardcodedCultureNames is used the method returns early on line 30. I can either rework the #if !FEATURE_CULTUREINFO_GETCULTURES to not return early and remove them from the hard-coded list, or leave it as-is.

This comment has been minimized.

@rainersigwald

rainersigwald Aug 27, 2018

Contributor

Ah, so it is. This is great, then.

@@ -192,6 +192,32 @@ public void Regress283991()
Assert.Equal(0, t.AssignedFilesWithCulture.Length);
Assert.Equal(1, t.AssignedFilesWithNoCulture.Length);
}

/*
* Method: PseudoLocalization

This comment has been minimized.

@rainersigwald

rainersigwald Aug 27, 2018

Contributor

Nit: can you make this a doc comment like the others in this file?

This comment has been minimized.

@martincostello

martincostello Aug 27, 2018

Contributor

On inspection of the file, they're all /* */ format except the one immediately before this new test. Happy to update to /// format if you prefer though.

This comment has been minimized.

@rainersigwald

rainersigwald Aug 27, 2018

Contributor

No, consistency with the majority is the right call. Naturally the one I looked at was the oddball 🤷‍♂️

@rainersigwald
Copy link
Contributor

rainersigwald left a comment

You were right all around. Only question that remains is whether we should rebase this on 15.9 and pull it in there, or leave it targeting master for 16.0.

// Regenerated using the tool (removed by https://github.com/aspnet/Localization/pull/130)
// * Removed the empty string from the list
// * Any cultures not present when regenerated were retained
// * Added the Windows pseudo-locales

This comment has been minimized.

@rainersigwald

rainersigwald Aug 27, 2018

Contributor

Ah, so it is. This is great, then.

{
if (!ValidCultureNames.Contains(pseudoLocale))
{
ValidCultureNames.Add(pseudoLocale);

This comment has been minimized.

@rainersigwald

rainersigwald Aug 27, 2018

Contributor

HashSet<T>.Add can be called for the same item many times. Its return value indicates whether it was actually added or already present, but we don't care in this case, so it's fine to ignore it. I prefer this because it's easier to read and reason about, and reduces indenting.

Let's just keep the foreach. It's consistent and evidently that's all UnionWith does anyway: https://github.com/dotnet/corefx/blob/58a310782b805df19a3939b12544c39296db5ed3/src/System.Collections/src/System/Collections/Generic/HashSet.cs#L507-L518

@@ -192,6 +192,32 @@ public void Regress283991()
Assert.Equal(0, t.AssignedFilesWithCulture.Length);
Assert.Equal(1, t.AssignedFilesWithNoCulture.Length);
}

/*
* Method: PseudoLocalization

This comment has been minimized.

@rainersigwald

rainersigwald Aug 27, 2018

Contributor

No, consistency with the majority is the right call. Naturally the one I looked at was the oddball 🤷‍♂️

@martincostello

This comment has been minimized.

Copy link
Contributor

martincostello commented Aug 27, 2018

Cool, thanks. I wasn't sure what the process for the target branch for PRs was here compared to the .NET Core repos (with master being 3.0 and the separate 2.2 branch and the auto-merging stuff).

Assuming approved, happy to re-target onto 15.9 so the change gets released faster 😃

@rainersigwald

This comment has been minimized.

Copy link
Contributor

rainersigwald commented Aug 27, 2018

At the moment, we have vs15.9 and master is for 16.0 and .NET Core 3.0. We don't have a fancy auto-merger at the moment, but we live without. We'll see what @AndyGerlicher thinks about 15.9 vs 16; I think we still have the option but we might not want to take the risk.

@AndyGerlicher

This comment has been minimized.

Copy link
Member

AndyGerlicher commented Aug 28, 2018

@martincostello This looks great, thanks for the contribution! Can you rebase to vs15.9 branch? That way we can get this in 15.9 / .NET Core 2.2 release.

@martincostello martincostello changed the base branch from master to vs15.9 Aug 28, 2018

@martincostello

This comment has been minimized.

Copy link
Contributor

martincostello commented Aug 28, 2018

I've updated the base branch for the PR to vs15.9, but looks like I need to tidy it up a bit more first.

martincostello added some commits Aug 25, 2018

Support Windows pseudo-locales
Add support for Windows pseudo-locales so that satellite resource assemblies can be compiled for them.
Update HardcodedCultureNames
Update HardcodedCultureNames by regenerating it using the generated since removed from aspnet/Localization (see aspnet/Localization#130).
Remove use of Contains()
Remove use of Contains() as it's safe to add to the HashSet if it's already there.

@martincostello martincostello force-pushed the martincostello:support-windows-pseudo-locales branch from ce49c38 to d059cde Aug 28, 2018

@martincostello

This comment has been minimized.

Copy link
Contributor

martincostello commented Aug 28, 2018

@AndyGerlicher Rebased.

@AndyGerlicher
Copy link
Member

AndyGerlicher left a comment

Thanks for rebasing!

@AndyGerlicher AndyGerlicher merged commit 21fb999 into Microsoft:vs15.9 Aug 28, 2018

7 checks passed

OSX10.13 Build for CoreCLR Build finished.
Details
RHEL7.2 Build for CoreCLR Build finished.
Details
Ubuntu16.04 Build for CoreCLR Build finished.
Details
WIP ready for review
Details
Windows_NT Build for CoreCLR Build finished.
Details
Windows_NT Build for Full Build finished.
Details
license/cla All CLA requirements met.
Details

@martincostello martincostello deleted the martincostello:support-windows-pseudo-locales branch Aug 29, 2018

rainersigwald added a commit to rainersigwald/msbuild that referenced this pull request Sep 5, 2018

Support Windows' pseudo-locales (Microsoft#3654)
* Support Windows pseudo-locales

Add support for Windows pseudo-locales so that satellite resource assemblies can be compiled for them.

* Update HardcodedCultureNames

Update HardcodedCultureNames by regenerating it using the generated since removed from aspnet/Localization (see aspnet/Localization#130).

Closes Microsoft#3653
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment