ndk icon indicating copy to clipboard operation
ndk copied to clipboard

launch the icu wrapper

Open DanAlbert opened this issue 8 years ago • 25 comments

Time to dust off that ICU4C wrapper and get it shipped.

DanAlbert avatar Oct 19 '17 17:10 DanAlbert

Is that mean that ICU will be available from NDK in future releases (r17 and up)?

TsvetanBogoev avatar Oct 24 '17 10:10 TsvetanBogoev

yeah, we've had a prototype for a while, and this is the reason we left a hole in the dynamic linker's native library namespace so that apps can access icu4c even though it's not an NDK library.

wrapper: https://android-review.googlesource.com/#/c/platform/development/+/121180/ example use: https://android-review.googlesource.com/#/c/153001/

enh avatar Oct 24 '17 15:10 enh

I just checked the prototype and the example and I think that I misunderstand the idea behind this task. After the check, for me it looks you are going to ship a copy of icu4c (precompiled static and shared lib) with next version of NDK. So using it from native code will bring a copy of the library (static or dynamic) to the app (APK), instead of dynamically link to the one used by the OS itself. Am I missing something? In our project we use icu4c, but the main problem is library and data size, its really huge, and the sad think is that every single android device contain icu4c library, but we can not use it.

TsvetanBogoev avatar Feb 02 '18 08:02 TsvetanBogoev

it looks you are going to ship a copy of icu4c

no. the prototype uses dlsym to find the system implementations.

enh avatar Feb 02 '18 17:02 enh

lol, another 6 mounts to wait for this feature? is there something like features priority, developers can vote for? And last is it possible and safe to use the implementation from https://android-review.googlesource.com/c/platform/development/+/121180/ using last stable NDK

TsvetanBogoev avatar Apr 03 '18 10:04 TsvetanBogoev

yes, the prototype should work fine with the stable NDK.

enh avatar Apr 03 '18 15:04 enh

I've dusted off the set of patches and have the prototype working again: https://android-review.googlesource.com/c/platform/development/+/121180 https://android-review.googlesource.com/c/platform/development/+/688457

Here's a sample app: https://github.com/DanAlbert/ndk-icu-sample.

It's not really ready to ship yet because the way it is currently implemented has some rough edges. Namely, there isn't a very good way to determine if a function will be available at runtime. Will be adding a ndk_icu_available("ucal_open") or similar.

It also does a ton of initialization work the first time you call any ICU function as it dlsyms everything whether you use it or not. Working on switching that to dlsym the given function the first time you call it instead so you only pay for what you use.

We also should build some tooling to preprocess the ICU4C headers to remove any non-stable APIs since it can be quite confusing to figure out which APIs are available and which are not. This tool also needs to rewrite the U_DISABLE_RENAMING value in the config header.

DanAlbert avatar May 22 '18 22:05 DanAlbert

Looking for some feedback:

There's a pretty big trade off involved with this wrapper that may not be obvious at first glance, and it's one that has turned away a few users internally. Namely, there's a huge testing burden on app developers. The approach given above has the advantage of being usable going all the way back to JB (probably earlier, but that's the NDK's minimum target atm) without the app needing to ship the giant ICU data file since it uses the system's instead.

The problem is that, since ICU4C has never been an NDK API, there has never been any requirement for OEMs to include any particular set of locales or even a full set of features for the contained locales in the data file on their devices. It's entirely possible that the wrapper would yield different results across different SKUs of the same device running the same OS version (as well as varying across OS version). We don't have any way of knowing how much variety there is in practice, but it could potentially be a huge source of bugs that vary device to device.

Knowing that... is this still a useful library for people? Or is that caveat enough of a problem that you'd prefer to call up to Java or even just ship your own data file?

One other question: which ICU4C APIs are people mostly looking for? There's a pretty large set, not all of which are in the stable C API (which are all we can reliably access with dlsym).

DanAlbert avatar Jun 07 '18 20:06 DanAlbert

I’d still find it useful, assuming that vendors have made reasonable pruning choices for their respective markets.

brawer avatar Jun 08 '18 06:06 brawer

A typical use of icu in our app (text editor) is for character conversion. Calling Java for character conversion hits performance. Currently we ship a small set of "Charset Mapping Tables" and we call Java only for the missing charsets. Also the size of icu4c library itself (not the data) is also a factor. So this wrapper will be useful in most of the cases, since we can check (runtime) for the set of available features and data

TsvetanBogoev avatar Jun 08 '18 06:06 TsvetanBogoev

Given the concerns above, I want to make sure we get the platform's ICU4C more thoroughly tested (and hopefully test some older devices as well) before we make the decision on whether or not this goes into the NDK proper or just becomes a project on github. Unfortunately that's going to take some time as the ICU4C tests are not straightforward to use in a cross-compiled setting, so I'm moving this to r19.

For anyone feeling brave and impatient, the prototype is available above. Any issues you run into will be interesting for us, so feel free to report any issues with it back here.

DanAlbert avatar Jun 13 '18 21:06 DanAlbert

A typical use of icu in our app (text editor) is for character conversion.

Just out of curiosity, I wonder why you need to handle the encoding conversion (other than UTF-8/16) in 2018. Does your editor need to import a lot of files generated in the past (encoded in "legacy" non-Unicode encodings)? I hope you don't need to export files to one of legacy encodings. :-)

jungshik avatar Jul 04 '18 23:07 jungshik

A typical use of icu in our app (text editor) is for character conversion.

Just out of curiosity, I wonder why you need to handle the encoding conversion (other than UTF-8/16) in 2018. Does your editor need to import a lot of files generated in the past (encoded in "legacy" non-Unicode encodings)? I hope you don't need to export files to one of legacy encodings. :-)

That's right, as an advanced text editor our app has to support visualization of files with any possible encoding

TsvetanBogoev avatar Sep 12 '18 09:09 TsvetanBogoev

Haven't had time to deal with the infrastructural issues involved in making this viable yet. Moving to r20.

DanAlbert avatar Oct 17 '18 20:10 DanAlbert

For those interested, and until this is implemented in the NDK proper, I've "implemented" the extractor at https://gitlab.com/koying/libicundk

See https://github.com/xbmc/xbmc/pull/12268#issuecomment-449635591 for usage example.

koying avatar Dec 23 '18 13:12 koying

We're more or less settled on making this an AAR rather than putting it in the NDK proper. The pieces for that are still coming together (and the testing work mentioned before still needs more attention as well). Moving out of r21.

DanAlbert avatar Sep 04 '19 20:09 DanAlbert

I still need ICU in my NDK code.

hubcin avatar Jan 06 '20 14:01 hubcin

Has ICU been silently just removed all together? I was assured before (on this very GH repo I think...) that it was an exception to the rule and that I could ~~link against~~ dlopen it but now suddenly I have failure to find the ICU libraries on Android 11 -> https://github.com/couchbase/couchbase-lite-core/issues/983

EDIT Looks like Android 10 introduced something called libandroidicu but is it a requirement to open that library instead? I haven't been using absolute paths until now to open the icu libraries, but rather these two lines:

handle_i18n = dlopen("libicui18n.so", RTLD_LOCAL);
handle_common = dlopen("libicuuc.so", RTLD_LOCAL);

borrrden avatar May 29 '20 04:05 borrrden

Sounds like a bug. Please file it.

DanAlbert avatar May 29 '20 04:05 DanAlbert

Disregard it. The reporter discovered it was because of the new arm execution ability on the emulator (Apparently when they run an arm compiled project on the emulator, this issue starts happening). If that is still bug worthy I can ask them to file it.

borrrden avatar May 29 '20 09:05 borrrden

It's a different sort of bug, but I think that is still a bug. Worth filing, I think (against the emulator, probably; they can redirect it if needed).

DanAlbert avatar May 29 '20 09:05 DanAlbert

I was trying to use icu wrapper in my android project using find_library(ICU_LIB icundk), but it cannot be found. Is the icu wrapper not available in NDK? @DanAlbert

Ujjwal21 avatar May 13 '22 07:05 Ujjwal21

No. We'll close the bug when it's available. Some ICU APIs have been added to the platform in recent release but the wrapper that backports them is not currently being worked on.

DanAlbert avatar May 13 '22 19:05 DanAlbert

No. We'll close the bug when it's available. Some ICU APIs have been added to the platform in recent release but the wrapper that backports them is not currently being worked on.

Can you help me how can I build ICU for Android platform. I was referring this link to build static files for ICU for Android - https://github.com/patrickgold/icu4c-android, but there seems to be some issue.

Ujjwal21 avatar May 13 '22 19:05 Ujjwal21

This thread links the prototypes we have. It's been long enough since I've looked at them that I can't really offer any help.

DanAlbert avatar May 13 '22 20:05 DanAlbert