Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support loading ICU data from managed Interop #49406

Merged
merged 5 commits into from
Mar 13, 2021
Merged

Support loading ICU data from managed Interop #49406

merged 5 commits into from
Mar 13, 2021

Conversation

steveisok
Copy link
Member

In iOS we support loading a custom dat file when working with ICU. The way this worked originally was the mono runtime exported a function that native code would call into (similar to wasm). After thinking about it a bit, it makes more sense to load ICU the same way we do on desktop, but with the ability to provide the path to an ICU dat file via an AppContext key ICU_DAT_FILE_PATH. The key can be provided before Xamarin iOS calls monovm_initialize and they won't have to worry about calling some special function.

Support loading ICU data from managed Interop
61de8be 
In iOS we support loading a custom dat file when working with ICU.  The way this worked originally was the mono runtime exported a function that
native code would call into (similar to wasm).  After thinking about it a bit, it makes more sense to load this the same way we do on desktop, but with the
ability to provide the path to an ICU dat file via an AppContext key `ICU_DAT_FILE_PATH`.  This can be provided before Xamarin iOS calls `monovm_initialize` and they won't have to worry about calling some special function.
@ghost
Copy link

ghost commented Mar 10, 2021

Tagging subscribers to this area: @tarekgh, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

In iOS we support loading a custom dat file when working with ICU. The way this worked originally was the mono runtime exported a function that native code would call into (similar to wasm). After thinking about it a bit, it makes more sense to load ICU the same way we do on desktop, but with the ability to provide the path to an ICU dat file via an AppContext key ICU_DAT_FILE_PATH. The key can be provided before Xamarin iOS calls monovm_initialize and they won't have to worry about calling some special function.

Author: steveisok
Assignees: -
Labels:

area-System.Globalization
Milestone: -

@tarekgh
Copy link
Member

tarekgh commented Mar 10, 2021 •
edited
Loading

How we'll guarantee the loaded data file will be compatible with the loaded ICU code library? I am guessing we can run into some issues that will be hard to diagnose if the data is not compatible with the code library.

@steveisok
Copy link
Member Author

How we'll guarantee the loaded data file will be compatible with the loaded ICU code library? I am guessing we can run into some issues that will be hard to diagnose if the data is not compatible with the code library.

For iOS, we'll control the version that we're using and the data file that comes with it. Beyond that, good question.

CoffeeFlux
CoffeeFlux approved these changes Mar 10, 2021
View reviewed changes
Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for hitting you with a bunch of style nits again.

Otherwise looks fine to me, thanks!

src/libraries/Native/Unix/System.Globalization.Native/pal_icushim_static.c Outdated Show resolved Hide resolved
src/libraries/Native/Unix/System.Globalization.Native/pal_icushim_static.c Outdated Show resolved Hide resolved
src/libraries/Native/Unix/System.Globalization.Native/pal_icushim_static.c Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.LoadICU.Unix.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.LoadICU.iOS.cs Outdated Show resolved Hide resolved
src/tasks/AppleAppBuilder/Templates/runtime.m Outdated Show resolved Hide resolved
src/tasks/AppleAppBuilder/Templates/runtime.m Outdated Show resolved Hide resolved
src/tasks/AppleAppBuilder/Templates/runtime.m Outdated Show resolved Hide resolved
src/tasks/AppleAppBuilder/Templates/runtime.m Outdated Show resolved Hide resolved
src/tasks/AppleAppBuilder/Templates/runtime.m Outdated Show resolved Hide resolved
@steveisok
Copy link
Member Author

@safern @tarekgh can you please review again?

tarekgh
tarekgh reviewed Mar 11, 2021
View reviewed changes
src/libraries/Native/Unix/System.Globalization.Native/pal_icushim_static.c Outdated

return load_icu_data (icu_data);
if (load_icu_data(icu_data) == 0) {
fprintf(stderr, "ICU BAD EXIT %d.", ret);
Copy link
Member

@tarekgh tarekgh Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fprintf [](start = 8, length = 7)

should we do log_icu_error instead in all places writing to the stderr?

tarekgh
tarekgh approved these changes Mar 12, 2021
View reviewed changes
Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some minor comments. LGTM otherwise.

lambdageek
lambdageek approved these changes Mar 12, 2021
View reviewed changes
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

monovm_ bits lgtm

lambdageek
lambdageek reviewed Mar 12, 2021
View reviewed changes
src/tasks/AppleAppBuilder/Templates/runtime.m
#ifndef INVARIANT_GLOBALIZATION
icu_dat_path,
icu_dat_path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: trailing commas are not an error.

(and they play nicer with git history when someone adds the next array entry)

@steveisok
Copy link
Member Author

Mono windows test failures are unrelated #49569

@steveisok steveisok merged commit 1d9c402 into dotnet:main Mar 13, 2021
@steveisok steveisok deleted the ios-load-icu-managed branch March 13, 2021 15:32
@rolfbjarne rolfbjarne mentioned this pull request Mar 15, 2021
Closed
@runfoapp runfoapp bot mentioned this pull request Mar 22, 2021
Closed
@spouliot spouliot mentioned this pull request Apr 8, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Apr 12, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@marek-safar marek-safar marek-safar left review comments

@lambdageek lambdageek lambdageek approved these changes

@jkotas jkotas jkotas left review comments

@safern safern safern approved these changes

@CoffeeFlux CoffeeFlux CoffeeFlux approved these changes

@tarekgh tarekgh tarekgh approved these changes

@akoeplinger akoeplinger Awaiting requested review from akoeplinger

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

@vargaz vargaz Awaiting requested review from vargaz

Assignees
No one assigned
Labels
area-System.Globalization
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
8 participants
@steveisok @tarekgh @marek-safar @lambdageek @CoffeeFlux @jkotas @safern @karelz