Support custom culture in RAR (#11000)

* Support custom culture

* Add ChangeWave info

* Warn on unintended culture overwrite

* Fix the warning call

* Add and fix unittests

* Apply PR comments

documentation/wiki/ChangeWaves.md

@@ -26,6 +26,7 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t
 ### 17.14
 - [.SLNX support - use the new parser for .sln and .slnx](https://github.com/dotnet/msbuild/pull/10836)
 - [TreatWarningsAsErrors, WarningsAsMessages, WarningsAsErrors, WarningsNotAsErrors are now supported on the engine side of MSBuild](https://github.com/dotnet/msbuild/pull/10942)
+- [Support custom culture in RAR](https://github.com/dotnet/msbuild/pull/11000)
 
 ### 17.12
 - [Log TaskParameterEvent for scalar parameters](https://github.com/dotnet/msbuild/pull/9908)

src/BuildCheck.UnitTests/EndToEndTests.cs

@@ -64,54 +64,102 @@ public void PropertiesUsageAnalyzerTest(bool buildInOutOfProcessNode)
         Regex.Matches(output, "BC0203 .* Property").Count.ShouldBe(2);
     }
 
-    // <EmbeddedResource Update = "Resource1.cs.resx" />
-
     [Theory]
+    // The culture is not set explicitly, but the extension is a known culture
+    //  - a buildcheck warning will occur, but otherwise works
     [InlineData(
         "cs",
         "cs",
         """<EmbeddedResource Update = "Resource1.cs.resx" />""",
-        "warning BC0105: .* 'Resource1\\.cs\\.resx'")]
-    // Following tests are prepared after the EmbeddedCulture handling fix is merged: https://github.com/dotnet/msbuild/pull/11000
-    ////[InlineData(
-    ////    "xyz",
-    ////    "xyz",
-    ////    """<EmbeddedResource Update = "Resource1.xyz.resx" />""",
-    ////    "warning BC0105: .* 'Resource1\\.xyz\\.resx'")]
-    ////[InlineData(
-    ////    "xyz",
-    ////    "zyx",
-    ////    """<EmbeddedResource Update = "Resource1.zyx.resx" Culture="xyz" />""",
-    ////    "")]
-    public void EmbeddedResourceCheckTest(string culture, string resourceExtension, string resourceElement, string expectedDiagnostic)
+        false,
+        "warning BC0105: .* 'Resource1\\.cs\\.resx'",
+        true)]
+    // The culture is not set explicitly, and is not a known culture
+    //  - a buildcheck warning will occur, and resource is not recognized as culture specific - won't be copied around
+    [InlineData(
+        "xyz",
+        "xyz",
+        """<EmbeddedResource Update = "Resource1.xyz.resx" />""",
+        false,
+        "warning BC0105: .* 'Resource1\\.xyz\\.resx'",
+        false)]
+    // The culture is explicitly set, and it is not a known culture, but $(RespectAlreadyAssignedItemCulture) is set to true
+    //  - no warning will occur, and resource is recognized as culture specific - and copied around
+    [InlineData(
+        "xyz",
+        "xyz",
+        """<EmbeddedResource Update = "Resource1.xyz.resx" Culture="xyz" />""",
+        true,
+        "",
+        true)]
+    // The culture is explicitly set, and it is not a known culture and $(RespectAlreadyAssignedItemCulture) is not set to true
+    //  - so culture is overwritten, and resource is not recognized as culture specific - won't be copied around
+    [InlineData(
+        "xyz",
+        "zyx",
+        """<EmbeddedResource Update = "Resource1.zyx.resx" Culture="xyz" />""",
+        false,
+        "warning MSB3002: Explicitly set culture .* was overwritten",
+        false)]
+    // The culture is explicitly set, and it is not a known culture, but $(RespectAlreadyAssignedItemCulture) is set to true
+    //  - no warning will occur, and resource is recognized as culture specific - and copied around
+    [InlineData(
+        "xyz",
+        "zyx",
+        """<EmbeddedResource Update = "Resource1.zyx.resx" Culture="xyz" />""",
+        true,
+        "",
+        true)]
+    public void EmbeddedResourceCheckTest(
+        string culture,
+        string resourceExtension,
+        string resourceElement,
+        bool respectAssignedCulturePropSet,
+        string expectedDiagnostic,
+        bool resourceExpectedToBeRecognizedAsSatelite)
     {
-        EmbedResourceTestOutput output = RunEmbeddedResourceTest(resourceElement, resourceExtension);
+        EmbedResourceTestOutput output = RunEmbeddedResourceTest(resourceElement, resourceExtension, respectAssignedCulturePropSet);
 
+        int expectedWarningsCount = 0;
         // each finding should be found just once - but reported twice, due to summary
         if (!string.IsNullOrEmpty(expectedDiagnostic))
         {
             Regex.Matches(output.LogOutput, expectedDiagnostic).Count.ShouldBe(2);
+            expectedWarningsCount = 1;
         }
 
-        AssertHasResourceForCulture("en");
-        AssertHasResourceForCulture(culture);
-        output.DepsJsonResources.Count.ShouldBe(2);
+        AssertHasResourceForCulture("en", true);
+        AssertHasResourceForCulture(culture, resourceExpectedToBeRecognizedAsSatelite);
+        output.DepsJsonResources.Count.ShouldBe(resourceExpectedToBeRecognizedAsSatelite ? 2 : 1);
+        GetWarningsCount(output.LogOutput).ShouldBe(expectedWarningsCount);
 
-        void AssertHasResourceForCulture(string culture)
+        void AssertHasResourceForCulture(string culture, bool isResourceExpected)
         {
             KeyValuePair<string, JsonNode?> resource = output.DepsJsonResources.FirstOrDefault(
                 o => o.Value?["locale"]?.ToString().Equals(culture, StringComparison.Ordinal) ?? false);
-            resource.Equals(default(KeyValuePair<string, JsonNode?>)).ShouldBe(false,
-                $"Resource for culture {culture} was not found in deps.json:{Environment.NewLine}{output.DepsJsonResources.ToString()}");
+            // if not found - the KVP will be default
+            resource.Equals(default(KeyValuePair<string, JsonNode?>)).ShouldBe(!isResourceExpected,
+                $"Resource for culture {culture} was {(isResourceExpected ? "not " : "")}found in deps.json:{Environment.NewLine}{output.DepsJsonResources.ToString()}");
 
-            resource.Key.ShouldBeEquivalentTo($"{culture}/ReferencedProject.resources.dll",
-                $"Unexpected resource for culture {culture} was found in deps.json:{Environment.NewLine}{output.DepsJsonResources.ToString()}");
+            if (isResourceExpected)
+            {
+                resource.Key.ShouldBeEquivalentTo($"{culture}/ReferencedProject.resources.dll",
+                    $"Unexpected resource for culture {culture} was found in deps.json:{Environment.NewLine}{output.DepsJsonResources.ToString()}");
+            }
+        }
+
+        int GetWarningsCount(string output)
+        {
+            Regex regex = new Regex(@"(\d+) Warning\(s\)");
+            Match match = regex.Match(output);
+            match.Success.ShouldBeTrue("Expected Warnings section not found in the build output.");
+            return int.Parse(match.Groups[1].Value);
         }
     }
 
     private readonly record struct EmbedResourceTestOutput(String LogOutput, JsonObject DepsJsonResources);
 
-    private EmbedResourceTestOutput RunEmbeddedResourceTest(string resourceXmlToAdd, string resourceExtension)
+    private EmbedResourceTestOutput RunEmbeddedResourceTest(string resourceXmlToAdd, string resourceExtension, bool respectCulture)
     {
         string testAssetsFolderName = "EmbeddedResourceTest";
         const string entryProjectName = "EntryProject";
@@ -128,7 +176,7 @@ private EmbedResourceTestOutput RunEmbeddedResourceTest(string resourceXmlToAdd,
 
         _env.SetCurrentDirectory(Path.Combine(workFolder.Path, entryProjectName));
 
-        string output = RunnerUtilities.ExecBootstrapedMSBuild("-check -restore", out bool success);
+        string output = RunnerUtilities.ExecBootstrapedMSBuild("-check -restore /p:RespectCulture=" + (respectCulture ? "True" : "\"\""), out bool success);
         _env.Output.WriteLine(output);
         _env.Output.WriteLine("=========================");
         success.ShouldBeTrue();

src/BuildCheck.UnitTests/TestAssets/EmbeddedResourceTest/ReferencedProject/ReferencedProject.csproj

@@ -1,7 +1,8 @@
 <Project Sdk="Microsoft.NET.Sdk">
 
   <PropertyGroup>
-    <TargetFramework>net9.0</TargetFramework>
+    <!-- Target net8.0 - as from net9.0 the RespectAlreadyAssignedItemCulture is added by common targets. -->
+    <TargetFramework>net8.0</TargetFramework>
     <ImplicitUsings>enable</ImplicitUsings>
     <Nullable>enable</Nullable>
   </PropertyGroup>
@@ -15,7 +16,7 @@
   </ItemGroup>
 
 	<PropertyGroup>
-		<RespectAlreadyAssignedItemCulture>True</RespectAlreadyAssignedItemCulture>
+		<RespectAlreadyAssignedItemCulture>$(RespectCulture)</RespectAlreadyAssignedItemCulture>
 	</PropertyGroup>
 
   <ItemGroup>

src/Framework/StringUtils.cs

@@ -31,4 +31,24 @@ internal static string GenerateRandomString(int length)
         string randomBase64String = Convert.ToBase64String(randomBytes).Replace('/', '_');
         return randomBase64String.Substring(0, length);
     }
+
+    /// <summary>
+    /// Removes last occurence of <paramref name="substring"/> from <paramref name="fromString"/>, if present.
+    /// </summary>
+    /// <param name="fromString">String to be altered.</param>
+    /// <param name="substring">String to be removed.</param>
+    /// <param name="comparison">The comparison to use for finding.</param>
+    /// <returns>The original string (if no occurrences found) or a new string, with last instance of <paramref name="substring"/> removed.</returns>
+    internal static string RemoveLastInstanceOf(this string fromString, string substring, StringComparison comparison = StringComparison.Ordinal)
+    {
+        int lastOccurrenceIndex = fromString.LastIndexOf(substring, comparison);
+
+        if (lastOccurrenceIndex != -1)
+        {
+            fromString = fromString.Substring(0, lastOccurrenceIndex) +
+                         fromString.Substring(lastOccurrenceIndex + substring.Length);
+        }
+
+        return fromString;
+    }
 }

src/Tasks/AssemblyDependency/ReferenceTable.cs

@@ -971,7 +971,8 @@ private void FindSatellites(
                     // Is there a candidate satellite in that folder?
                     string cultureName = Path.GetFileName(subDirectory);
 
-                    if (CultureInfoCache.IsValidCultureString(cultureName))
+                    // Custom or unknown cultures can be met as well
+                    if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_14) || CultureInfoCache.IsValidCultureString(cultureName))
                     {
                         string satelliteAssembly = Path.Combine(subDirectory, satelliteFilename);
                         if (_fileExists(satelliteAssembly))

src/Tasks/AssignCulture.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using Microsoft.Build.Collections;
 #if DEBUG
 using System.Diagnostics;
 #endif
@@ -158,6 +159,20 @@ public override bool Execute()
                             // https://github.com/dotnet/msbuild/issues/3064
                             ConversionUtilities.ValidBooleanFalse(AssignedFiles[i].GetMetadata(ItemMetadataNames.withCulture)));
 
+                        // The culture was explicitly specified, but not opted in via 'RespectAlreadyAssignedItemCulture' and different will be used
+                        if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_14) &&
+                            !string.IsNullOrEmpty(existingCulture) &&
+                            !MSBuildNameIgnoreCaseComparer.Default.Equals(existingCulture, info.culture))
+                        {
+                            Log.LogWarningWithCodeFromResources("AssignCulture.CultureOverwritten",
+                                existingCulture, AssignedFiles[i].ItemSpec, info.culture);
+                            // Remove the culture if it's not recognized
+                            if (string.IsNullOrEmpty(info.culture))
+                            {
+                                AssignedFiles[i].RemoveMetadata(ItemMetadataNames.culture);
+                            }
+                        }
+
                         if (!string.IsNullOrEmpty(info.culture))
                         {
                             AssignedFiles[i].SetMetadata(ItemMetadataNames.culture, info.culture);

src/Tasks/CreateCSharpManifestResourceName.cs

@@ -101,12 +101,26 @@ internal static string CreateManifestNameImpl(
             }
 
             dependentUponFileName = FileUtilities.FixFilePath(dependentUponFileName);
-            Culture.ItemCultureInfo info = Culture.GetItemCultureInfo(embeddedFileName, dependentUponFileName, treatAsCultureNeutral);
+            Culture.ItemCultureInfo info;
 
-            // If the item has a culture override, respect that.
-            if (!string.IsNullOrEmpty(culture))
+            if (!string.IsNullOrEmpty(culture) && ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_14))
             {
-                info.culture = culture;
+                info = new Culture.ItemCultureInfo()
+                {
+                    culture = culture,
+                    cultureNeutralFilename =
+                        embeddedFileName.RemoveLastInstanceOf("." + culture, StringComparison.OrdinalIgnoreCase)
+                };
+            }
+            else
+            {
+                info = Culture.GetItemCultureInfo(embeddedFileName, dependentUponFileName, treatAsCultureNeutral);
+                // If the item has a culture override, respect that.
+                // We need to recheck here due to changewave in condition above - after Wave17_14 removal, this should be unconditional.
+                if (!string.IsNullOrEmpty(culture))
+                {
+                    info.culture = culture;
+                }
             }
 
             var manifestName = StringBuilderCache.Acquire();

src/Tasks/CreateVisualBasicManifestResourceName.cs

@@ -99,12 +99,27 @@ internal static string CreateManifestNameImpl(
                 embeddedFileName = fileName;
             }
 
-            Culture.ItemCultureInfo info = Culture.GetItemCultureInfo(embeddedFileName, dependentUponFileName, treatAsCultureNeutral);
+            dependentUponFileName = FileUtilities.FixFilePath(dependentUponFileName);
+            Culture.ItemCultureInfo info;
 
-            // If the item has a culture override, respect that.
-            if (!string.IsNullOrEmpty(culture))
+            if (!string.IsNullOrEmpty(culture) && ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_14))
             {
-                info.culture = culture;
+                info = new Culture.ItemCultureInfo()
+                {
+                    culture = culture,
+                    cultureNeutralFilename =
+                        embeddedFileName.RemoveLastInstanceOf("." + culture, StringComparison.OrdinalIgnoreCase)
+                };
+            }
+            else
+            {
+                info = Culture.GetItemCultureInfo(embeddedFileName, dependentUponFileName, treatAsCultureNeutral);
+                // If the item has a culture override, respect that.
+                // We need to recheck here due to changewave in condition above - after Wave17_14 removal, this should be unconditional.
+                if (!string.IsNullOrEmpty(culture))
+                {
+                    info.culture = culture;
+                }
             }
 
             var manifestName = StringBuilderCache.Acquire();

src/Tasks/Resources/Strings.resx

@@ -149,6 +149,13 @@
   <data name="AssignCulture.Comment">
     <value>Culture of "{0}" was assigned to file "{1}".</value>
   </data>
+  <data name="AssignCulture.CultureOverwritten">
+    <value>MSB3002: Explicitly set culture "{0}" for item "{1}" was overwritten with inferred culture "{2}", because 'RespectAlreadyAssignedItemCulture' property was not set.</value>
+    <comment>
+	{StrBegin="MSB3002: "}
+	'RespectAlreadyAssignedItemCulture' should not be translated
+	</comment>
+  </data>
   <!--
         The AxImp message bucket is: MSB3656 - MSB3660.