idn: add native AppleIDN (icucore) support for macOS/iOS #13246

MonkeybreadSoftware · 2024-03-31T10:00:50Z

I implemented the IDN functions for macOS and iOS using Unicode
libraries coming with macOS and iOS.

Builds and runs here on macOS 14.2.1. Also verified to load and
run on older macOS version 10.13.

Build requires macOS SDK 13 or equivalent.

Set -DUSE_APPLE_IDN=ON CMake option to enable it.
With autotools and other build tools, set these manual options:

CPPFLAGS=-DUSE_APPLE_IDN
LIBS=-licucore

Completes TODO 1.6.

TODO: add autotools option and feature-detection.

Refs: #5330 #5371
Co-authored-by: Viktor Szakats
Closes #13246

MonkeybreadSoftware · 2024-03-31T10:07:23Z

Please note that applications using libcurl may need to use -licucore to get icucore library added.
But the normal curl command line tool just builds, so I assume icucore is already referenced somehow.

lib/idn.c

vszakats · 2024-03-31T10:20:20Z

Please note that applications using libcurl may need to use -licucore to get icucore library added. But the normal curl command line tool just builds, so I assume icucore is already referenced somehow.

Speaking of CMake, this snippet would add that:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -870,6 +870,10 @@ if(WIN32)
   endif()
 endif()
 
+if(APPLE)
+  list(APPEND CURL_LIBS "icucore")
+endif()
+
 #libpsl
 option(CURL_USE_LIBPSL "Use libPSL" ON)
 mark_as_advanced(CURL_USE_LIBPSL)

I wonder if we should also add a build option for this, like we have for Windows. Some may prefer to keep IDN-support disabled and also helps if there is any regression.

MonkeybreadSoftware · 2024-03-31T10:21:52Z

I just tried it on iOS and it seems to work fine here.
And much better than my older PR. :-)

lib/idn.c

MonkeybreadSoftware · 2024-03-31T12:26:08Z

trying now with #if __has_include(<unicode/uidna.h>) to check if the header exists.

lib/idn.c

bagder

Maybe skip the variables here?

lib/idn.c

lib/idn.h

vszakats · 2024-04-02T12:08:40Z

At the end, don't we need to link the icucore library from cmake/autotools, to make this work?

MonkeybreadSoftware · 2024-04-02T12:27:41Z

Does it build for you without?
For me it does as I don't have it in the link lists. Maybe some other library...

Okay, found it: CoreFoundation.framework is references by lib curl and that references /usr/lib/libicucore.A.dylib internally. So you don't need to reference it explicitly.

vszakats · 2024-04-02T13:07:11Z

@MonkeybreadSoftware: We're good then, thanks for checking!

vszakats · 2024-04-02T15:26:33Z

Made a test with curl-for-win, and it seems the extra library is missing (with Apple clang):
https://github.com/curl/curl-for-win/actions/runs/8525243980/job/23351725118#step:3:4941

Undefined symbols for architecture arm64:
  "_uidna_close", referenced from:
      _curl_url_get in libcurl.a(unity_0_c.c.o)
      _Curl_idn_decode in libcurl.a(unity_0_c.c.o)
      _Curl_idn_encode in libcurl.a(unity_0_c.c.o)
      _Curl_idnconvert_hostname in libcurl.a(unity_0_c.c.o)
  "_uidna_nameToASCII_UTF8", referenced from:
      _curl_url_get in libcurl.a(unity_0_c.c.o)
      _Curl_idn_decode in libcurl.a(unity_0_c.c.o)
      _Curl_idnconvert_hostname in libcurl.a(unity_0_c.c.o)
  "_uidna_nameToUnicodeUTF8", referenced from:
      _curl_url_get in libcurl.a(unity_0_c.c.o)
      _Curl_idn_encode in libcurl.a(unity_0_c.c.o)
  "_uidna_openUTS46", referenced from:
      _curl_url_get in libcurl.a(unity_0_c.c.o)
      _Curl_idn_decode in libcurl.a(unity_0_c.c.o)
      _Curl_idn_encode in libcurl.a(unity_0_c.c.o)
      _Curl_idnconvert_hostname in libcurl.a(unity_0_c.c.o)
ld: symbol(s) not found for architecture arm64

Also true for standard llvm:
https://github.com/curl/curl-for-win/actions/runs/8525243980/job/23351725441#step:3:4946

MonkeybreadSoftware · 2024-04-02T15:32:54Z

Sounds like we need a "-licucore" command for the linking.
Could you add that for linking when targeting iOS or macOS?
later I tried it myself. Let's see.

vszakats · 2024-04-02T17:10:19Z

CMakeLists.txt

+  if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
+    if (ENABLE_ARES)
+      list(APPEND CURL_LIBS "-licucore")
+    else()


Isn't -licucore necessary for all APPLE? The next issue is that this makes it necessary to detect the presence of this OS feature in CMake/autotools, then pass this on via HAVE_APPLE_IDN (replacing __has_include). Then add this library if the feature is available and enable it in source on this condition too.

Sure, we could add it for the other case, too. Or just move it outside the inner condition.
I like the __has_include as it allows to check for a header on compile time.
Feel free to adjust the build scripts.

Moving it around will not help for Apple targets not supporting this feature. It will fail when linking a non-existing library.

Well, if here is someone who can write CMake conditions and configure checks to check for presents of unicode/uidna.h and whether you can link "icucore". A check for file on disk won't work since the library is in the dylib cache archive.

On the other side, why bother to check at all. All platforms from Apple support the header file and include the library as far as I know: Apple TV OS, iOS, MacOS, WatchOS and Vision Pro OS. And I verified that the library exists on macOS 10.13, so included for a couple of years already.

Of course it would be nice to have a switch to turn it off in case of someone seeing a problem. But CMake/Configure scripts is outside my expertise.

I proposed a patch earlier that added icucore unconditionally to CMake. The current logic seems wrong, e.g. what does ENABLE_ARES have to do with enabling icucore?

But even that simple patch would:

fail when building with an older (pre v13) Apple toolchain.

likely fail to build (or run) when using a new Apple toolchain but targeting an older OS release.

doesn't allow to disable this feature. This might have valid reasons like someone hitting a snag with the newly added logic, and still wanting to build curl. Or, someone just doesn't want IDN support for whatever reason.

leaves this feature broken with autotools. That needs the same detection/enabler/lib-adder logic.

I can't volunteer for this now, because I'm working since Thursday pretty much day and night on open source stuff on my free time.

A way to fix this is to remove header detection from the C code, replace it with HAVE_APPLE_IDN (meaning: support is available) and USE_APPLE_IDN (we also want to use it) macros, all expected to be set manually for the time being. Then later add the CMake logic doing the automatic detection/feature-toggle, and the same for autotools.

We don't have this formally defined, I build curl-for-win for 10.9.

IMO this isn't really be about trying to hardwire or guess these versions in the build or source.

This PR already is already good if the feature availability can be passed manually, and offers another option to enable it. We use these mechanics for similar cases.

As a next step (in a separate PR even) detecting availability can be automated. E.g. by setting up the icucore lib and checking for one of the APIs, using check_function_exists() in CMake.

For curl-for-win for macOS 10.9?

We could detect macOS 10.13 SDK (or iOS 11.0 SDK) via #if like this:

#if TARGET_OS_OSX #if MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_13 // disable feature for old MacOS SDK #endif #elif TARGET_OS_IPHONE #if __IPHONE_OS_VERSION_MIN_REQUIRED < __IPHONE_11_0 // disable feature for old iOS SDK (also Watch, VisionOS, AppleTV, etc.) #endif #endif

So it could be automatically disabled for older SDK.

We'd just need a nice way to detect whether library is there and add it. Configure script needs to get a -licucore, too. But not sure where.

What's the benefit of scattering detection into these three: a hardwired logic in-source, a dynamic-header detection in-source and future-developed library detection in autotools and CMake?

Well, if I could do it all in source, I wouldn't need to wait for someone to do autotools and CMake.
I'd prefer to have it all automatic. If this is opt-in for building on macOS, I fear it may not get used.

Pure in-source detection would be nice, but I can't see how this could be technically feasible.

Since we do need detection, our option is to either make it opt-in for now and add detection in a separate commit, or add detection now. We also need an opt-out option in either case.

This is a useful feature, so it's reasonable to assume that even the two-stage implementation will pass through quickly. My worry is autotools, which would need to be done in the same step, but needs a very different skillset than CMake. Worst case we can make an exception in the CI job responsible for keeping CMake and autotools in sync.

CMakeLists.txt

lib/idn.c

For macOS and iOS using system functions in unicode libraries.

removed debug output fprintf

Added checks for Xcode 15

renamed switch to HAVE_APPLE_IDN

Added -licucore for link parameters

thanks Co-authored-by: Viktor Szakats <vszakats@users.noreply.github.com>

from what it was before messing there

Changed "int len" to "(void)" since we don't need len.

Enabled by default with CMake. To enable with autotools: ``` CPPFLAGS=-DUSE_APPLE_IDN LIBS=-licucore ```

vszakats · 2024-04-15T12:38:55Z

Pushed an update:

add CMake support with auto-detection. Disabled by default. Requires SDK 13 or later.
Can be enabled with -DUSE_APPLE_IDN=ON.
acommodate some logic to the new IDN.
rebase on master. This adds curl-for-win macOS CI builds.
with autotools and other build tools you can enable AppleIDN with:
```
CPPFLAGS=-DUSE_APPLE_IDN
LIBS=-licucore
```

Regarding targeting old macOS versions:

Limited local tests confirm that a build targeting 10.9 and using SDK 13 successfully detects and builds AppleIDN support, and the binary also runs and apparently works (converting an accented domain name to punycode) when run on a macOS that's actually missing the dynamically linked /usr/lib/libicucore.A.dylib. I don't know how this works.

If this isn't always true or my tests are wrong, we should probably disable AppleIDN by default, or allow/block-list it in CMake based on CMAKE_OSX_DEPLOYMENT_TARGET or something.

edit: Settled on being OFF by default, to:

avoid potential colliding with the default USE_LIBIDN2=ON setting. If that's picked up and AppleIDN also, there is a build error due to the conflict. [edit: build works, but AppleIDN gets priority.]
avoid issues targeting old systems till we know for sure that it's compatible with them (see above): -DCMAKE_OSX_DEPLOYMENT_TARGET=10.9 -DCMAKE_C_FLAGS=-mmacosx-version-min=10.9
make it behave like USE_WIN32_IDN (also OFF by default, to not break Windows XP compatibility).

curl-for-win and CI tests have it explicitly enabled.

Ref: curl/curl#13246

Ref: curl#5371

vszakats · 2024-04-15T22:28:57Z

Is it correct that this PR completes TODO 1.6 and we can delete that?

MonkeybreadSoftware · 2024-04-16T04:56:21Z

Yes. Thanks. Finally get Todo 1.6 done :-)

vszakats · 2024-04-16T22:25:23Z

Thank you @MonkeybreadSoftware!

Expected to arrive with curl 8.8.0. Ref: curl/curl@add22fe curl/curl#13246

MonkeybreadSoftware mentioned this pull request Mar 31, 2024

Enabling IDN via macOS/iOS DNS services #13233

Closed