[iOS][non-icu] HybridGlobalization use system ICU #93220

mkhamoyan · 2023-10-09T13:56:41Z

With these changes we will not load ICU anymore for Apple mobile platforms and will use system ICU from Apple sdk.
This will make HybridGlobalization as default and only option for Apple mobile platforms.
As a follow up, we'll allow also to link in ICU.

Below is size comparison for System.Globalization.Tests app.

--	Hybrid mode	Loading ICU
app size	37,8 MB	43,1 MB
ipa size	12,7 MB	14.7 MB

This will give us 2 MB improvement.

Contributes to #80689

ghost · 2023-10-09T13:56:52Z

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details

Load system ICU that Apple is using.

Contributes to #80689

Author:	mkhamoyan
Assignees:	-
Labels:	`area-System.Globalization`, `os-ios`
Milestone:	-

azure-pipelines · 2023-12-18T13:23:25Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2023-12-18T13:57:48Z

/azp run runtime-ioslike

azure-pipelines · 2023-12-18T13:58:03Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2023-12-18T14:59:32Z

/azp run runtime-ioslikesimulator

mkhamoyan · 2023-12-18T14:59:43Z

/azp run runtime-maccatalyst

azure-pipelines · 2023-12-18T14:59:47Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2023-12-18T14:59:54Z

Azure Pipelines successfully started running 1 pipeline(s).

akoeplinger

Couple questions, looks good otherwise. Thanks!

akoeplinger · 2023-12-19T09:16:33Z

src/mono/CMakeLists.txt

@@ -747,10 +747,9 @@ elseif(HOST_BROWSER)
  set(STATIC_ICU 1)
  set(ICU_LIBS "icucore")
 elseif(HOST_IOS OR HOST_TVOS OR HOST_MACCAT)
-  set(ICU_FLAGS "-DTARGET_UNIX -DU_DISABLE_RENAMING -Wno-reserved-id-macro -Wno-documentation -Wno-documentation-unknown-command -Wno-switch-enum -Wno-covered-switch-default -Wno-extra-semi-stmt -Wno-unknown-warning-option -Wno-deprecated-declarations")
+  set(ICU_FLAGS "-DTARGET_UNIX -DTARGET_IOS -DTARGET_TVOS -DTARGET_MACCATALYST -DU_DISABLE_RENAMING -Wno-reserved-id-macro -Wno-documentation -Wno-documentation-unknown-command -Wno-switch-enum -Wno-covered-switch-default -Wno-extra-semi-stmt -Wno-unknown-warning-option -Wno-deprecated-declarations")


is it ok if we pass -DTARGET_IOS -DTARGET_TVOS -DTARGET_MACCATALYST unconditionally? I'd have expected we only pass on the respective platform

Updated, thanks.

akoeplinger · 2023-12-19T09:19:10Z

src/native/libs/System.Globalization.Native/pal_icushim_internal.h

+#endif
+#if !defined(__APPLE__) || (defined(__APPLE__) && !(TARGET_MACCATALYST || TARGET_IOS || TARGET_TVOS))


couldn't we just do #else here?

akoeplinger · 2023-12-19T09:20:38Z

src/native/libs/System.Globalization.Native/pal_locale.m

@@ -61,7 +60,7 @@ static void GetParent(const char* localeID, char* parent, int32_t parentCapacity
    int32_t i;

    if (localeID == NULL)
-        localeID = [NSLocale systemLocale].localeIdentifier.UTF8String;
+        localeID = [NSLocale currentLocale].localeIdentifier.UTF8String;


this seems unrelated to the system ICU change?

yes, will revert it back.

akoeplinger · 2023-12-19T09:22:13Z

src/tests/FunctionalTests/Directory.Build.props

@@ -5,6 +5,7 @@
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <IsTestProject Condition="'$(IsTestProject)' == ''">true</IsTestProject>
    <IsFunctionalTest>true</IsFunctionalTest>
+    <HybridGlobalization Condition="'$(TargetOS)' == 'ios' or '$(TargetOS)' == 'iossimulator' or '$(TargetOS)' == 'tvos' or '$(TargetOS)' == 'tvossimulator' or '$(TargetOS)' == 'maccatalyst'">true</HybridGlobalization>


can we use TargetsAppleMobile here?

Updated, thanks.

mkhamoyan · 2023-12-19T10:19:06Z

/azp run runtime-maccatalyst

azure-pipelines · 2023-12-19T10:19:16Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2023-12-19T10:19:34Z

/azp run runtime-ioslikesimulator

azure-pipelines · 2023-12-19T10:19:43Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2023-12-19T10:20:07Z

/azp run runtime-ioslike

azure-pipelines · 2023-12-19T10:20:17Z

Azure Pipelines successfully started running 1 pipeline(s).

mkhamoyan · 2023-12-19T15:36:06Z

Failures are not related

mkhamoyan added 3 commits October 4, 2023 15:23

comment loading icu when hybrid mode

2d149a4

Fix failing tests without loading icu

4bd2918

Merge branch 'main' into system_icu

8bf39c6

mkhamoyan added area-System.Globalization os-ios Apple iOS labels Oct 9, 2023

ghost assigned mkhamoyan Oct 9, 2023

Uncomment calendar test cases

d5bcd12

build-analysis bot mentioned this pull request Oct 9, 2023

Tracking issue for CI build timeouts #76454

Closed

runfoapp bot mentioned this pull request Oct 9, 2023

Methodical_others test JIT/Methodical/Coverage/copy_prop_byref_to_native_int crashing #69832

Open

build-analysis bot mentioned this pull request Oct 9, 2023

LibraryImportGenerator unit test segfault illegal memory access #89054

Closed

fix build failures

c2f7ac4

build-analysis bot mentioned this pull request Oct 16, 2023

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono #82637

Open

steveisok self-requested a review October 17, 2023 13:01

Steve Pfister and others added 16 commits October 17, 2023 09:03

Remove other ICU references

064b71b

Load pal_idna.c

47f0271

clean the branch

82c04b1

Merge branch 'main' into system_icu

6796080

Remove not needed function

d3fcc60

minor fix

fb34efc

Add country and language tables

2e0900c

Add check for hybrid mode

a91cdf7

minor fix

7d6641d

Fix build

5097d38

link to -licucore in the appbuilder

4ad9b4e

Fix System.Globalization.Tests on hybrid mode

455aa2a

revert changes in IOS tests

64b9309

revert ios tests csproj changes

39785db

inlcude utypes.h

1caed2e

Adjust idna mapping tests

7beec5c

Fix nativeaot failure

2b42a43

mkhamoyan requested a review from akoeplinger December 18, 2023 14:59

build-analysis bot mentioned this pull request Dec 18, 2023

Failure in System.Threading.RateLimiting.Test.ConcurrencyLimiterTests.DoesNotDeadlockCleaningUpCanceledRequestedLease_Post #91653

Closed

akoeplinger approved these changes Dec 19, 2023

View reviewed changes

review refactoring

ba3aecb

mkhamoyan merged commit 4ba4110 into dotnet:main Dec 19, 2023
210 of 212 checks passed

mkhamoyan deleted the system_icu branch December 19, 2023 15:39

rolfbjarne mentioned this pull request Dec 21, 2023

Numerous GlobalizationNative P/Invokes in System.Private.CoreLib.dll don't have a corresponding native function anymore #96251

Closed

This was referenced Dec 21, 2023

[iOS][non-icu] Enable HybridGlobalization by default xamarin/xamarin-macios#19695

Closed

[HybridGlobalization] Fix xamarin_macios build after system ICU change #96270

Merged

jkotas mentioned this pull request Jan 9, 2024

[HybridGlobalization] Include all globalization symbols #96684

Merged

LoopedBard3 mentioned this pull request Jan 11, 2024

iOS NativeAOT testing app failing to build in performance pipeline #96868

Closed

This was referenced Jan 12, 2024

Use HybridGlobalization as default and only option for Apple mobile platforms dotnet/performance#3750

Closed

[perf] Use HybridGlobalization as default and only option for Apple mobile platforms #96918

Merged

mkhamoyan mentioned this pull request Jan 18, 2024

[iOS] Introduce support for linking ICU libs #97147

Closed

github-actions bot locked and limited conversation to collaborators Jan 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[iOS][non-icu] HybridGlobalization use system ICU #93220

[iOS][non-icu] HybridGlobalization use system ICU #93220

mkhamoyan commented Oct 9, 2023 •

edited

Loading

ghost commented Oct 9, 2023

azure-pipelines bot commented Dec 18, 2023

mkhamoyan commented Dec 18, 2023

azure-pipelines bot commented Dec 18, 2023

mkhamoyan commented Dec 18, 2023

mkhamoyan commented Dec 18, 2023

azure-pipelines bot commented Dec 18, 2023

azure-pipelines bot commented Dec 18, 2023

akoeplinger left a comment

akoeplinger Dec 19, 2023

mkhamoyan Dec 19, 2023

akoeplinger Dec 19, 2023

akoeplinger Dec 19, 2023

mkhamoyan Dec 19, 2023

akoeplinger Dec 19, 2023

mkhamoyan Dec 19, 2023

mkhamoyan commented Dec 19, 2023

azure-pipelines bot commented Dec 19, 2023

mkhamoyan commented Dec 19, 2023

azure-pipelines bot commented Dec 19, 2023

mkhamoyan commented Dec 19, 2023

azure-pipelines bot commented Dec 19, 2023

mkhamoyan commented Dec 19, 2023

		#endif
		#if !defined(__APPLE__) \|\| (defined(__APPLE__) && !(TARGET_MACCATALYST \|\| TARGET_IOS \|\| TARGET_TVOS))

[iOS][non-icu] HybridGlobalization use system ICU #93220

[iOS][non-icu] HybridGlobalization use system ICU #93220

Conversation

mkhamoyan commented Oct 9, 2023 • edited Loading

ghost commented Oct 9, 2023

azure-pipelines bot commented Dec 18, 2023

mkhamoyan commented Dec 18, 2023

azure-pipelines bot commented Dec 18, 2023

mkhamoyan commented Dec 18, 2023

mkhamoyan commented Dec 18, 2023

azure-pipelines bot commented Dec 18, 2023

azure-pipelines bot commented Dec 18, 2023

akoeplinger left a comment

Choose a reason for hiding this comment

akoeplinger Dec 19, 2023

Choose a reason for hiding this comment

mkhamoyan Dec 19, 2023

Choose a reason for hiding this comment

akoeplinger Dec 19, 2023

Choose a reason for hiding this comment

akoeplinger Dec 19, 2023

Choose a reason for hiding this comment

mkhamoyan Dec 19, 2023

Choose a reason for hiding this comment

akoeplinger Dec 19, 2023

Choose a reason for hiding this comment

mkhamoyan Dec 19, 2023

Choose a reason for hiding this comment

mkhamoyan commented Dec 19, 2023

azure-pipelines bot commented Dec 19, 2023

mkhamoyan commented Dec 19, 2023

azure-pipelines bot commented Dec 19, 2023

mkhamoyan commented Dec 19, 2023

azure-pipelines bot commented Dec 19, 2023

mkhamoyan commented Dec 19, 2023

mkhamoyan commented Oct 9, 2023 •

edited

Loading