Fix NETSDK1187: Replace OS-dependent CultureInfo locale normalization with platform-independent BCP 47 algorithm#53817
Fix NETSDK1187: Replace OS-dependent CultureInfo locale normalization with platform-independent BCP 47 algorithm#53817
Conversation
|
@baronfel doing a local review, copilot suggested we simplify the logic to fix the ckb to ku issue as that's the critical bug but this would leave in the qps-ploc casing warning. I'm kind of inclined to go with the recommendation that fixes the blocking issue but leaves the warning in for the specific case as it's way simpler and easier to maintain. I do see the ploc has a fair bit of usage across a quick GH search. Thoughts on that compromise? I just saw the solution here and it looked like it was going to be overly complex. |
|
@marcpopMSFT I think I like your take a bit more for sure. In general this build warning is a heuristic - the actual behavior of loading the resources is going to rely on the locale information of the host that the .NET Runtime will read content from, so we can loosen the check in the way you describe fairly reasonably! |
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent NETSDK1187 from producing OS/runtime-dependent locale “normalization” results by avoiding CultureInfo remapping behaviors (e.g., ckb → ku) and adding coverage for the ckb scenario.
Changes:
- Updates
ResolvePackageAssetsto only applyCultureInfonormalization when the result differs by casing only. - Adds a new unit test ensuring
ckbdoes not trigger NETSDK1187.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs |
Adjusts locale normalization logic to skip changes that are more than casing differences. |
test/Microsoft.NET.Build.Tasks.Tests/GivenAResolvePackageAssetsTask.cs |
Adds a regression test for the ckb locale not producing NETSDK1187. |
| var normalizedLocale = System.Globalization.CultureInfo.GetCultureInfo(locale).Name; | ||
| if (normalizedLocale != locale) | ||
|
|
||
| // Only apply CultureInfo's normalization when it is a pure casing change. | ||
| // CultureInfo can remap locale codes to entirely different locales | ||
| // (e.g. 'ckb' -> 'ku') depending on the OS/ICU version, so we skip | ||
| // normalization when the result differs beyond casing. | ||
| if (string.Equals(normalizedLocale, locale, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
The PR description/title says locale normalization is now done via a platform-independent BCP 47 casing algorithm, but this code still derives normalizedLocale from CultureInfo.GetCultureInfo(locale).Name. That keeps NETSDK1187’s suggested “correct” casing dependent on the OS/ICU/runtime (e.g., qps-ploc can still normalize differently across environments). Consider using the deterministic BCP 47 casing normalizer for NETSDK1187 and reserving GetCultureInfo() only for NETSDK1188 validation, as described in the PR metadata.
| [Theory] | ||
| [InlineData("net7.0")] | ||
| [InlineData("net6.0")] | ||
| public void It_does_not_warn_for_ckb_locale(string tfm) |
There was a problem hiding this comment.
Test name suggests there should be no warnings at all for ckb, but the assertions only check that NETSDK1187 isn’t produced. If NETSDK1188 is logged on some platforms (where ckb isn’t recognized by CultureInfo), this test would still pass while contradicting its name/intent. Consider renaming the test to mention NETSDK1187 specifically (or assert on NETSDK1188 too, if the intent is truly “no warnings”).
| public void It_does_not_warn_for_ckb_locale(string tfm) | |
| public void It_does_not_warn_with_NETSDK1187_for_ckb_locale(string tfm) |
Agent-Logs-Url: https://github.com/dotnet/sdk/sessions/9f4f397c-a795-43a9-8c4b-1e4144a938e7 Co-authored-by: marcpopMSFT <12663534+marcpopMSFT@users.noreply.github.com>
…t doesn't fix the recasing.
5895cb4 to
0d3c0da
Compare
CultureInfo.GetCultureInfo(locale).Nameused for locale normalization is OS/runtime-dependent and can remap locale codes to different locales (e.g.ckb→ku), causing NETSDK1187 to fire incorrectly and with inconsistent "correct" values across SDK versions — creating an unresolvable flip-flop for package authors.Changes
ResolvePackageAssets.cs: ReplaceCultureInfo.GetCultureInfo(locale).Namenormalization with a newNormalizeBcp47LocaleCasing()method implementing RFC 5646 casing rules deterministically:CultureInfo.GetCultureInfo()is retained only in thetry/catchfor NETSDK1188 (truly unrecognized locale) validation — its return value is no longer used.Behavior
ckbku" (wrong locale!)qps-plocqps-Ploc" orqps-ploc" (OS-dependent)qps-Ploc" (consistent)qps-Plocru-ruru-RU"ru-RU" (unchanged)