[iOS] Introduce support for linking ICU libs #97216
Conversation
mkhamoyan
added
NO-REVIEW
|
Tagging subscribers to 'os-ios': @steveisok, @akoeplinger, @kotlarmilos Issue DetailsAfter #93220 we only support hybrid mode on Apple platforms. Below are the steps to implement support for linking our ICU libs.
Fixes #97147
|
|
/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst |
|
Azure Pipelines successfully started running 3 pipeline(s). |
src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs
Outdated
Show resolved
Hide resolved
If nothing is specified |
|
/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@lewing @akoeplinger could you please review? |
We'll automatically link with all static libraries shipped with Mono. If what we link with changes depending on build configuration / MSBuild properties, then it should be implemented as a Mono Component: https://github.com/dotnet/runtime/blob/main/docs/design/mono/components.md |
| <NetCoreAppNativeLibrary Include="System.Globalization.Native" Condition="'$(StaticICULinking)' != 'true' and '$(InvariantGlobalization)' != 'true'" /> | ||
| <NetCoreAppNativeLibrary Include="System.Globalization.Native" Condition="'$(StaticICULinking)' != 'true' and '$(InvariantGlobalization)' != 'true' and (('$(_IsiOSLikePlatform)' == 'true' and '$(HybridGlobalization)' != 'true') or '$(_IsiOSLikePlatform)' != 'true')" /> | ||
| <NetCoreAppNativeLibrary Include="System.HybridGlobalization.Native" Condition="'$(_IsiOSLikePlatform)' == 'true' and '$(StaticICULinking)' != 'true' and '$(InvariantGlobalization)' != 'true' and '$(HybridGlobalization)' == 'true'" /> |
There was a problem hiding this comment.
If I understood the motivation for this PR correctly, hybrid globalization is the new default for iOS-like platforms. However, these lines will include System.HybridGlobalization.Native library only if HybridGlobalization=true.
What are the expectations on where HybridGlobalization MSBuild property would be set?
There was a problem hiding this comment.
Motivation of this PR is to support our ICU libs on Apple platforms (as currently that is not possible).
By these changes we are creating 2 separate libraries
System.Globalization.Native- this will include our ICU libsSystem.HybridGlobalization.Native- this is only for Hybrid mode, using native implementations for globalization
Hence System.HybridGlobalization.Native library will be included only if HybridGlobalization=true.
If HybridGlobalization=false we will include System.Globalization.Native which is getting globalization data using our ICU libs.
There was a problem hiding this comment.
I see, thank you for the explanation.
However, I am still confused with:
we only support hybrid mode on Apple platforms
Does that mean that in this new default setting, we should start linking against System.HybridGlobalization.Native by default?
And if that is the case, then HybridGlobalization=true would have to be set somewhere early in SDK
There was a problem hiding this comment.
Default mode is implemented such that if the user does not enable Invariant mode, the Hybrid mode is automatically enabled. (see https://github.com/dotnet/runtime/pull/97216/files#diff-2884e8c39a88d26cb57ef06adca164f61238c4ad1c593d8e38bcb167212faa38R18 and https://github.com/dotnet/runtime/pull/97216/files#diff-45555715c5a7b53d8311327e5515c313f9e1c316f612af23fb84b5859a5c48ccR479).
If user wants to use ICU libs then they need to disable HybridGlobalization.
There was a problem hiding this comment.
the Hybrid mode is automatically enabled
It is not how this build logic works. If the user does not set Hybrid mode explicitly by setting the HybridGlobalization to true, this build logic is going to default to ICU.
There was a problem hiding this comment.
Right, it is how it should work; but it is not what https://github.com/dotnet/runtime/pull/97216/files#diff-a8c821001409a0c4d6589a59e663d4d07e53b0127b345f17640410828c580b37R118-R119 does.
When the HybridGlobalization property is not set, '$(HybridGlobalization)' == 'true' is going to be false and it is going to default to ICU.
There was a problem hiding this comment.
For clarification: MsBuild properties have 3 states, true, false, undefined. When the property is not set then it's not false, it's undefined.
There was a problem hiding this comment.
The intent is to default to hybrid. That may mean setting the value explicitly to true or defining another setting to enable icu in the sdk.
There was a problem hiding this comment.
Right, it is how it should work; but it is not what https://github.com/dotnet/runtime/pull/97216/files#diff-a8c821001409a0c4d6589a59e663d4d07e53b0127b345f17640410828c580b37R118-R119 does.
When the HybridGlobalization property is not set,
'$(HybridGlobalization)' == 'true'is going to be false and it is going to default to ICU.
I see, I will change this part to check if '$(HybridGlobalization)' == 'false' or '$(HybridGlobalization)' != 'false'.
There was a problem hiding this comment.
I would like to say that the improvements which hybrid globalization is bringing to both Mono and NativeAOT on iOS are amazing, and thanks @mkhamoyan and the whole team for your hard work and making this possible!
The remark I made on these lines of code seem to match the intended behaviour with the latest commit. However, I think this is only one piece of the puzzle which solves only the NativeAOT builds.
HybridGlobalization as a feature switch
As I mentioned in my comment, I think that for changes like these, when we are potentially breaking our internal and/or external customers, a better approach would be to first introduce a MSBuild property "feature switch" (for example in https://github.com/dotnet/sdk) which would be HybridGlobalization=false at first.
After that, do the implementation and make all the required changes in all our relevant repositories (dotnet/runtime, xamarin/xamarin-macios, dotnet/maui) taking into account this new features switch and test internally with HybridGlobalization=true as the changes flow in (this would potentially get better with dotnet/dotnet, but we are not there yet).
If everything works, throughout our whole stack (E2E testing passes), then simply flip the switch in the dotnet/sdk to true and announce it is the new default.
This could still break our external customers, but at least internally we made sure that we provided ways to integrate and adapt to the new change.
On the other hand, regarding questions around Mono components and their build integration #97216 (comment), this seems to be a different use case.
For Mono native components like: hot_reload, debugger, diagnostics_tracing, marshal-ilgen Mono provides both: an empty native lib version - "stub" (when the component is not needed/used) and the actual native lib with the implementation.
To toggle between what needs to be linked with the final app and to enable other build systems to specify which components should be included in the build Mono provides -> RuntimeComponentManifest.targets
and item groups: _MonoRuntimeComponentDontLink and _MonoRuntimeComponentLink.
Xamarin already implements this through: https://github.com/xamarin/xamarin-macios/blob/282d107a02d80e04a1eca780f022966d52c50215/dotnet/targets/Xamarin.Shared.Sdk.targets#L442-L461
This is a great approach, but the changes introduced here do not seem to fit. We will have two globalization libraries that we ship, but we need to link only against one of them, and according to: https://github.com/xamarin/xamarin-macios/blob/282d107a02d80e04a1eca780f022966d52c50215/dotnet/targets/Xamarin.Shared.Sdk.targets#L1134-L1141
with this PR, we would probably end up in build failures, as we would be linking against both: System.Globalization.Native and System.HybridGlobalization.Native.
This is where a global sdk-level feature switch would play a role, as the build systems (internal or external) would have a single source of truth and know which library to link against.
Another way to provide native dependencies
One thing to note is that NativeAOT also supports native components and switching them on and off (libeventpipe-enabled/disabled, libstandalonegc-enabled/disabled), but the NativeAOT build system resolves this on its own, and can provide native linker switches and dependencies for integrators to utilize and expand upon:
This approach is also supported by the Xamarin build system: https://github.com/xamarin/xamarin-macios/blob/282d107a02d80e04a1eca780f022966d52c50215/dotnet/targets/Xamarin.Shared.Sdk.targets#L1357-L1362
As the hybrid globalization change is a bit unique (and maybe in the future we will figure their would be a new implementation of say: System.Security.Cryptography.Native.Apple) we might want to provide the way for Mono to also resolve the native dependencies on its own (similarly to how NativeAOT does it) for example in: Microsoft.NET.Runtime.MonoTargets.Sdk https://github.com/dotnet/runtime/blob/64204a583ec9563b67b4e9d06538bbf46dcb29ca/src/mono/nuget/Microsoft.NET.Runtime.MonoTargets.Sdk/Microsoft.NET.Runtime.MonoTargets.Sdk.pkgproj package where RuntimeComponentManifest.targets already lives and is provided with workload installation. This way we would simplify the build integrations process altogether and make a first step towards having a runtime-agnostic integration.
These are my two cents, and please let me know if I can help in this effort to unblock the feature getting into the upcoming releases, as it brings great size saving for our customers targeting iOS platforms.
Does this change fit the intended design of mono components? Existing usage is the component is on or there's a stub and it's controlled from within the runtime. In this scenario, there's no stub (either hybrid or pull in icu) and activation is triggered from managed code. |
|
Yeah I agree that mono components is not a good fit for this. |
I agree it doesn't quite fit the design of mono components, but I think there's one important part Mono Components provide: its design ensures that we don't have to hardcode any logic about the runtime to know which native libraries to link with (at least to have a working app build). As it stands, we'll link with both Having to update our build logic to ensure an app builds successfully can be quite disruptive. For instance, due to the previous hybrid/ICU/globalization changes in .NET 9:
|
|
/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Closing as we have opted to pursue a different direction. |
What is the new direction that we are pursuing? I think number of changes in this PR are good cleanup and you should consider checking them in regardless. In particular, changing the linking of the globalization shim to work like all other shims is good. |
|
For changing the linking of the globalization I separated changes here #98495. |
Sounds great! And what's the new direction that we are pursuing? Are we still working towards making hybrid vs. ICU configurable by the user? |
After #93220 we only support hybrid mode on Apple platforms.
Below are the steps to implement support for linking our ICU libs.
follow-up PR: xamarin-macios and android should be needing to link against
libSystem.Globalization.Native.a. For hybrid mode on Apple platformslibSystem.HybridGlobalization.Native.ashould be linked - cc @rolfbjarneFixes #97147
Contributes to #80689