-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[iOS] Introduce support for linking ICU libs #97216
Closed
Closed
Changes from 21 commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
3681a84
Create separate lib System.HybridGlobalization.Native
mkhamoyan 6fd58f0
Fix build on Apple platforms
mkhamoyan d0a6820
HYBRID_GLOBALIZATION_DLL_NAME define only for Apple
mkhamoyan 9d91003
Fix build failure
mkhamoyan 29a248e
Fix non-Apple build
mkhamoyan 8bc9d03
Fix syntax error
mkhamoyan 50f4ce8
Fix linux builds
mkhamoyan b082055
Fix duplicate symbol issue
mkhamoyan e1ac526
Filter in Xcode libs depending on hybrid mode, pinvoke changes
mkhamoyan 1566200
Revert not needed changes
mkhamoyan f660f6d
Revert changes
mkhamoyan 4d009b1
Fix ICU lib flow
mkhamoyan f5d2926
Change hybrid casing and idn functions
mkhamoyan 9f00a46
Merge branch 'main' into support_icu_libs
mkhamoyan a7cc352
temporary solution for library mode not include hybrid libs
mkhamoyan 0587d2d
Merge branch 'main' into support_icu_libs
mkhamoyan 01f9da0
Merge branch 'support_icu_libs' of https://github.com/mkhamoyan/runti…
mkhamoyan 376dcb0
Refactoring and running tests on hybrid mode
mkhamoyan 7534dc6
When hybrid mode don't include icu libs
mkhamoyan 4db5989
Merge branch 'main' into support_icu_libs
mkhamoyan 0ede71f
Fix syntax error
mkhamoyan 6e3dc61
Fix OSX build failure
mkhamoyan 23d91d9
Remove unused code
mkhamoyan 2263998
Remove PKG_CONFIG_PATH info
mkhamoyan fc5d2c5
Move libe code under System.Globalization.Native
mkhamoyan 7f436a7
Fix build
mkhamoyan 910d640
Fix windows build
mkhamoyan 7342955
clean up the code
mkhamoyan 23f15fb
clean up the code, test both ICU and hybrid on CI
mkhamoyan 9bef7b5
Merge branch 'main' into support_icu_libs
mkhamoyan aaacddb
Fix build
mkhamoyan e0fb75c
Make hybrid mode enabled by default
mkhamoyan 9294637
Change setting Hybrid property
mkhamoyan f869884
Remove redundant test suites and add System.Globalization.Tests for iOS
mkhamoyan bbfed76
Fix HybridGlobalization value check
mkhamoyan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Runtime.InteropServices; | ||
|
||
internal static partial class Interop | ||
{ | ||
internal static partial class Globalization | ||
{ | ||
|
||
[LibraryImport(Libraries.HybridGlobalizationNative, EntryPoint = "GlobalizationNative_ToAscii", StringMarshalling = StringMarshalling.Utf16)] | ||
internal static unsafe partial int ToAsciiNative(uint flags, char* src, int srcLen, char* dstBuffer, int dstBufferCapacity); | ||
|
||
[LibraryImport(Libraries.HybridGlobalizationNative, EntryPoint = "GlobalizationNative_ToUnicode", StringMarshalling = StringMarshalling.Utf16)] | ||
internal static unsafe partial int ToUnicodeNative(uint flags, char* src, int srcLen, char* dstBuffer, int dstBufferCapacity); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ifHybridGlobalization=true
.What are the expectations on where
HybridGlobalization
MSBuild property would be set?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 globalizationHence
System.HybridGlobalization.Native
library will be included only ifHybridGlobalization=true
.If
HybridGlobalization=false
we will includeSystem.Globalization.Native
which is getting globalization data using our ICU libs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thank you for the explanation.
However, I am still confused with:
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 SDKThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default mode is implemented such that if the user does not enable
Invariant
mode, theHybrid
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarification: MsBuild properties have 3 states,
true
,false
,undefined
. When the property is not set then it's notfalse
, it'sundefined
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I will change this part to check if
'$(HybridGlobalization)' == 'false'
or'$(HybridGlobalization)' != 'false'
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andSystem.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:runtime/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets
Lines 114 to 122 in 64204a5
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 whereRuntimeComponentManifest.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.