dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.17k stars 1.34k forks source link

Satellite assemblies are not produced for registered custom cultures when using .NET Core msbuild #1454

Open nguerrera opened 7 years ago

nguerrera commented 7 years ago

Moving from https://github.com/dotnet/sdk/issues/387 (originally https://github.com/dotnet/cli/issues/4050) on behalf of @ryanbrandenburg.


Steps to reproduce

  1. Register a custom culture using something like sysglobl.dll and:
var car1 = new CultureAndRegionInfoBuilder("ru-US", CultureAndRegionModifiers.None);
car1.LoadDataFromCultureInfo(CultureInfo.CreateSpecificCulture("ru-RU"));
car1.LoadDataFromRegionInfo(new RegionInfo("en-US"));

car1.CultureEnglishName = "Russion (United States)";
car1.CultureNativeName = "русский (США)";
car1.CurrencyNativeName = "Доллар (США)";
car1.RegionNativeName = "США";

// Register the culture.
try
{
    car1.Register();
}
catch (InvalidOperationException)
{
    // Swallow the exception: the culture already is registered.
}
  1. Create a new ASP.NET Core Web Application (.NET Core)
  2. Create two resources, one named Resource.en-US.resx and one named Resource.ru-US.resx
  3. Build the project.
  4. Open bin/Debug/net451
  5. Observe that a en-US folder is present and has a dll and that ru-US is missing

    Expected behavior

Any culture which will work with new CultureInfo("culture-NAME") (which custom cultures do once registered as above) should create a folder and dll which can be used in localization.

Actual behavior

Satellite assemblies are only created for original cultures, not for custom cultures.

Environment data

dotnet --info output: .NET Command Line Tools (1.0.0-preview2-003121)

Product Information: Version: 1.0.0-preview2-003121 Commit SHA-1 hash: 1e9d529bc5

Runtime Environment: OS Name: Windows OS Version: 10.0.10586 OS Platform: Windows RID: win10-x64

nguerrera commented 7 years ago

(Moving https://github.com/dotnet/sdk/issues/387#issuecomment-265608133)

This will work fine if you build using desktop MSBuild / Visual Studio, but .NET Core MSBuild has this.

This can be worked around with a custom target that lets any culture through:

  <Target Name="AssignCustomCultures" AfterTargets="SplitResourcesByCulture">
    <ItemGroup>
      <EmbeddedResource Condition="$([System.IO.Path]::HasExtension(%(Filename)))">
        <Culture>$([System.IO.Path]::GetExtension(%(Filename)).TrimStart('.'))</Culture>
        <WithCulture>true</WithCulture>
      </EmbeddedResource>
    </ItemGroup>
  </Target>

This approach has the advantage that it is not dependent on what is registered on the build machine.

nguerrera commented 7 years ago

@rainersigwald Thoughts? Should msbuild be more lenient about culture names to allow for custom cultures? I'm not too keen on relying on the build machine having registered a custom culture, but that does at least work with desktop msbuild. However, I don't know if .NET Core BCL provides enough to replicate that.

rainersigwald commented 7 years ago

I remember talking about this when @cdmihai implemented the current mechanism but I don't know enough about the problem space to have a good opinion off the bat.

Speaking from the hip, it seems like we should either always allow arbitrary cultures or stick to some sort of list. I agree that registering the culture by changing OS state on the build machine is kind of an icky way to enable this.

cdmihai commented 7 years ago

On Full Framework MSBuild populates a list of all valid culture names via CultureInfo.GetCultures. That method does not exist on .Net Core, so we just use the hardcoded list FF gave us.

We hit this in MSBuild's own build because we have a resource named Strings.Shared.Resx. This caused a runtime crash with missing resources on .Net Core because the resource was assigned the "Shared" culture. It works on FF because FF successfully rejects "Shared" as a culture.

Here's one solution: accept everything on .net core, and introduce a ThisIsNotACulture metadata for EmbeddedResource which tells the AssignCulture task to interpret it as neutral.

cdmihai commented 7 years ago

P.S.: We went with the hardcoded list to mimic the FF behaviour as close as possible. If universal strings failed for our build, then it could fail for others too. Maybe we'll get the API back with ns2.0

nguerrera commented 7 years ago

How about making the list a property or items that the user can amend.

nguerrera commented 7 years ago

Btw, the first thing I tried was to explicitly give WithCulture=true, Culture=ru-US to a static item but AssignCulture wacks that, which is why my workaround evolved in to a target.

Another approach would be to respect explicit WithCulture metadata as implying that no check is required.

cdmihai commented 7 years ago

Yup, sounds like we have to implement something that respects current metadata to ignore / enforce file name based culture inference. Or as you said, look into extending the hardcoded list of cultures.

In terms of planning, when should this change go in? How often is it that users create their own custom cultures?

nguerrera commented 7 years ago

I don't know about the usage but I suspect it's low. Given that there is a workaround of setting metadata in a custom rather, I'd think we could get away with vNext.

adsurg commented 7 years ago

Is there a bug here? GenerateSatelliteAssemblies only runs where MSBuildRuntimeType != Core but ComputeIntermediateSatelliteAssemblies which is dependent on it runs in all cases.

I've tried following the above bit am still hitting the same issue.

adsurg commented 7 years ago

@nguerrera, ping in case you're not still following this.

adsurg commented 7 years ago

It's an evil, hacky workaround, but dropping the line <Target Name="ComputeIntermediateSatelliteAssemblies"></Target> into the problem project file seems to get past this for now.

nguerrera commented 7 years ago

@adsurg It's not clear what you are working around. Can you open a new issue with repro steps and the precise failure that you're seeing. It is not clear that this is the same as the root issue, which is about custom cultures. There are targets that are core runtime only for satellite only because core msbuild doesn't support building satellites with its own targets, but anything that does not run on core has an alternate on full framework msbuild that does run.

alberto-riggi commented 6 years ago

@nguerrera Hi Nick, I tried with vs build and it doesn't generate the custom culture dll. I tried the workaround you proposed. Adding that target on the xproj and it gives me "Invalid static method invocation syntax: "[System.IO.Path]::HasExtension()". Method 'System.IO.Path.HasExtension' not found. "

I think I am missing something. Do you have any idea?

tarekgh commented 5 years ago

This issue is not limited to the custom cultures but also to the cultures that can be created by the framework but not enumerated with CultureInfo.GetCultures. for example, on Windows 10, you can create any culture even if the OS doesn't have data for (something like yy-YY). also on Linux if using one of the aliased cultures.

marcoregueira commented 4 years ago

Hi

This issue is still happening.

Satellite assemblies for custom cultures are still not generated.

Using the provided workaround generates the satellite assemblies for the referenced projects, but they are not copied to the main executable project.

To make this more misleading, if you have a webapi project with custom culture resources in the main project, you can access those resources without any problem. In this case they're embedded resources and not satellite assemblies. But if you have a referenced assembly, it generates satellite assemblies only for known cultures, but not custom cultures.

(Moving dotnet/sdk#387 (comment))

This will work fine if you build using desktop MSBuild / Visual Studio, but .NET Core MSBuild has this.

This can be worked around with a custom target that lets any culture through:

  <Target Name="AssignCustomCultures" AfterTargets="SplitResourcesByCulture">
    <ItemGroup>
      <EmbeddedResource Condition="$([System.IO.Path]::HasExtension(%(Filename)))">
        <Culture>$([System.IO.Path]::GetExtension(%(Filename)).TrimStart('.'))</Culture>
        <WithCulture>true</WithCulture>
      </EmbeddedResource>
    </ItemGroup>
  </Target>
Forgind commented 4 years ago

Can you use the Copy task to move them right after they're generated?

tarekgh commented 4 years ago

to fix this, msbuild has to change the way depending on culture enumerated list and instead try to validate the culture by trying to create it. CultureInfo.GetCultureInfo can be used as it caches the previously created culture anyway.

keab42 commented 4 years ago

Given that the issue has been present for 3 1/2 years, how likely is a fix at this point?

marcoregueira commented 4 years ago

Can you use the Copy task to move them right after they're generated?

Not really well. I can make it to work partially. But as far as I have seen, I need to add the task from nguerrera above, to every project that has those resources, then add the copy command to all projects in cascade, and a command for each dependency using project paths.

I made the test for a couple of projects in a solution and for some reason it worked for one of them but not for the other with identical configurations. No idea why, but the process seems so time and work consuming and prone to failure that doesn't seem a good idea to go ahead putting it to work.

Maybe I missed something, and is easier than it looks...

DamianEdwards commented 2 years ago

Came across another case of folks running into this issue: https://docs.microsoft.com/en-us/answers/questions/611607/how-to-add-custom-culture-ex-en-mxenglish-mexico-i.html

I'd really like to see us do a focused push on tackling the issues folks are having in this area (localization in .NET Core) as it's definitely a recurring topic every release.

DamianEdwards commented 2 years ago

I've posted an example repro ASP.NET Core project that demonstrates this issue at https://github.com/DamianEdwards/CustomCultureExample

The repro site is running at https://dedward-customcultureissue.azurewebsites.net/ on Linux and it repros there too.

BartoszCichecki commented 1 year ago

Any news on this one?

rainersigwald commented 1 year ago

There are some conflicting constraints that will keep us from getting to a perfect solution, but we can do better than requiring the workaround above.

The ideal solution would have no project-file impact, and a target would determine the culture for each .resx file.

The general pattern is ResourceFilename.culture.resx. But it's perfectly acceptable to have neutral resources in ResourceFilename.resx, and it's also ok for the name to include multiple segments/namespaces/whatever.

So if you have A.B.C.resx, it's ambiguous between:

  1. Resources for A.B.C, with no culture, and
  2. Resources for A.B in culture C.

The existing AssignCulture task uses a heuristic to disambiguate: if C is a valid culture, assume it's a culture. That's great unless you have resources with C# code in them in a Foo.cs.resx which looks like a valid alias for cs-CZ, so you can explicitly specify WithCulture="false" as of #5824 to explicitly say "don't detect a culture here, treat as neutral".

But there's no means to do the opposite and specify that "C is not a culture known to the system at build time, but it will be a culture registered and usable at runtime for this app".

I can see two options for that, not mutually exclusive:

  1. Extend AssignCulture to respect a manually-specified culture, and require custom culture resource files to manually specify that culture, for example
diff --git a/WebApplication116/CustomCultureWebApp.csproj b/WebApplication116/CustomCultureWebApp.csproj
index e0b8c8f..b99c807 100644
--- a/WebApplication116/CustomCultureWebApp.csproj
+++ b/WebApplication116/CustomCultureWebApp.csproj
@@ -18,4 +18,10 @@
     </AssemblyAttribute>
   </ItemGroup>
   </Target>
+
+  <ItemGroup>
+    <EmbeddedResource Update="Pages\Index.en-MX.resx">
+      <Culture>en-MX</Culture>
+    </EmbeddedResource>
+  </ItemGroup>
 </Project>
  1. Define a list of known custom cultures, and respect it in AssignCulture, for example
diff --git a/WebApplication116/CustomCultureWebApp.csproj b/WebApplication116/CustomCultureWebApp.csproj
index e0b8c8f..e1e7a6c 100644
--- a/WebApplication116/CustomCultureWebApp.csproj
+++ b/WebApplication116/CustomCultureWebApp.csproj
@@ -4,6 +4,7 @@
     <TargetFramework>net6.0</TargetFramework>
     <Nullable>enable</Nullable>
     <ImplicitUsings>enable</ImplicitUsings>
+    <KnownCustomCultures>en-MX;ja-MX</KnownCustomCultures>
   </PropertyGroup>

I think doing both would have the best UX. The normal project diff could look like the latter there but enable more flexibility.

DamianEdwards commented 1 year ago

@rainersigwald these proposals look good. Curious though, what would be problematic about just checking whether the C in your example above (i.e. that last dot-separated segment before the .resx extension) is a semantically valid culture identifier, and if so, assume it's a culture, with the same workaround (WithCulture="false") for cases where it isn't?

blanchardglen commented 2 months ago

Would you accept a PR for this? I could nearly match @rainersigwald example except in option 1 for backwards compatibility you would need to specify <WithCulture>true</WithCulture>

  <ItemGroup>
    <EmbeddedResource Update="Pages\Index.en-MX.resx">
      <Culture>en-MX</Culture>
+     <WithCulture>true</WithCulture>
    </EmbeddedResource>
  </ItemGroup>

By doing that the existing unit tests pass without changes and custom cultures are supported

<KnownCustomCultures> works great as well

DaleCam commented 2 months ago

@danroth27 You mentioned I could ping you if somethings an issue for my organization that developing heavily with Blazor. This one is a blocking issue for a non profit application my organization built helping thousands of users in smaller cultures. It would help us immensely if this ticket could get some love. Thanks Dan!

rainersigwald commented 2 months ago

@f-alizada this will be addressed by https://github.com/dotnet/msbuild/pull/10095 right?

f-alizada commented 2 months ago

Recent PR that got merged: https://github.com/dotnet/msbuild/pull/10026 introduces the possibility to specify the property for the Task RespectAlreadyAssignedItemCulture. When set to true the metadata Culture recognized by the MSBuild. however the default value of this property is false. Currently work in progress to enable this by default for SDK.

FatTigerWang commented 1 week ago

(Moving dotnet/sdk#387 (comment))

This will work fine if you build using desktop MSBuild / Visual Studio, but .NET Core MSBuild has this.

This can be worked around with a custom target that lets any culture through:

  <Target Name="AssignCustomCultures" AfterTargets="SplitResourcesByCulture">
    <ItemGroup>
      <EmbeddedResource Condition="$([System.IO.Path]::HasExtension(%(Filename)))">
        <Culture>$([System.IO.Path]::GetExtension(%(Filename)).TrimStart('.'))</Culture>
        <WithCulture>true</WithCulture>
      </EmbeddedResource>
    </ItemGroup>
  </Target>

This approach has the advantage that it is not dependent on what is registered on the build machine.

Using this method will cause ManifestResourceName to generate unexpected names in .NET 8 and .NET Framework, for example:

Controllers.WeatherForecastController.en-CN.resx => Controllers.WeatherForecastController.en-CN.en-CN.resx

It is speculated that this is because MSBUILD cannot distinguish whether en-CN is ordinary characters or Culture.

Therefore, you can use <LogicalName> to re-specify ManifestResourceName

<Target Name="AssignCustomCultures" AfterTargets="SplitResourcesByCulture">
  <ItemGroup>
    <EmbeddedResource Condition="'%(Filename)%(Extension)' == 'Controllers.WeatherForecastController.en-CN.resx'">
      <Culture>$([System.IO.Path]::GetExtension(%(Filename)).TrimStart('.'))</Culture>
      <WithCulture>true</WithCulture>
      <LogicalName>$(AssemblyName).Resources.Controllers.WeatherForecastController.$([System.IO.Path]::GetExtension(%(Filename)).TrimStart('.')).resources</LogicalName>
    </EmbeddedResource>
  </ItemGroup>
</Target>