Bug 220081 – [CMake] Use Apple's ICU as fallback

• Status: RESOLVED
• Resolution: FIXED
• Component: CMake

[monson]

[CMake] Add USE_APPLE_ICU option

[monson]

Created attachment 416654 [details]
Patch

[monson]

Add USE_APPLE_ICU option to allow non-Mac ports (GTK or JSCOnly) on Darwin could build with non-Apple ICU.

[Radar WebKit Bug Importer]

<rdar://problem/72593973>

[Fujii Hironori]

What is your motivation? Which port are building?
There are only ICU header files in Source/WTF/icu directory. Do you have a actual ICU library of the same version with the headers?

[Fujii Hironori]

I'm sorry. I didn't read your comment#2.

[Fujii Hironori]

Comment on attachment 416654 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416654&action=review

Looks good to me

> ChangeLog:5
> +

Could you add your comment#2 in the ChangeLog.

[monson]

Created attachment 416707 [details]
Patch

[Alex Christensen]

Comment on attachment 416707 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416707&action=review

> Source/cmake/OptionsCommon.cmake:103
> +option(USE_APPLE_ICU "Use Apple's internal ICU" ${APPLE})

Should this be in OptionsMac.cmake  instead of here?

[monson]

Comment on attachment 416707 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416707&action=review

>> Source/cmake/OptionsCommon.cmake:103
>> +option(USE_APPLE_ICU "Use Apple's internal ICU" ${APPLE})
> 
> Should this be in OptionsMac.cmake  instead of here?

It's not about PORT Mac but about OS(Darwin), it affects other ports. For GTK on Darwin, it usually prefers self-compiled version-specified ICU instead of the Apple one, mainly in the case of cross-platform applications keeping their dependencies unified.

An alternative to option USE_APPLE_ICU is, we search ICU normally first, if not found, then fallback to use the Apple shipped one. I can make another patch if you like this approach.

[monson]

Created attachment 416754 [details]
Patch

[Don Olmstead]

Comment on attachment 416754 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416754&action=review

Overall looks like a good direction for people wanting to build non Apple ports on Apple hardware.

> Source/WTF/ChangeLog:8
> +        Drop mandatory using Apple's ICU on APPLE, instead use it only when no others found.

I think you could be more descriptive here on what the original behavior is and what was changed to support it.

> Source/cmake/WebKitFindPackage.cmake:104
> +            set(ICU_INCLUDE_DIRS ${CMAKE_BINARY_DIR}/ICU/Headers)

Isn't ICU_INCLUDE_DIRS being set somewhere already? Shouldn't that be respected unless there is a difference?

[monson]

Comment on attachment 416754 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416754&action=review

>> Source/WTF/ChangeLog:8
>> +        Drop mandatory using Apple's ICU on APPLE, instead use it only when no others found.
> 
> I think you could be more descriptive here on what the original behavior is and what was changed to support it.

sure

>> Source/cmake/WebKitFindPackage.cmake:104
>> +            set(ICU_INCLUDE_DIRS ${CMAKE_BINARY_DIR}/ICU/Headers)
> 
> Isn't ICU_INCLUDE_DIRS being set somewhere already? Shouldn't that be respected unless there is a difference?

I don't think so, here ICU_INCLUDE_DIRS is set only when (NOT ICU_FOUND), and this line exists prior to this patch. If it's redundant we should call apple to remove.

[monson]

Created attachment 417083 [details]
Patch

[Don Olmstead]

r=me

Bots seem happy. I'm going to give Alex and anyone over at Apple, since they're trying to revive the CMake bits, a chance to cq+ it. If I don't hear any objections I'll just cq+ it this afternoon.

Thanks for the patch! Really good idea here.

[Yusuke Suzuki]

Comment on attachment 417083 [details]
Patch

Can you ensure USE_APPLE_ICU is on-by-default in Apple Mac ports?

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #15)
> Comment on attachment 417083 [details]
> Patch
> 
> Can you ensure USE_APPLE_ICU is on-by-default in Apple Mac ports?

I think we should pass an option, and find an installed ICU only when an option is provided. It is possible that random ICU is installed in the system (due to homebrew etc.) and I think JSCOnly and Mac ports should not select these ICU by default.

[monson]

(In reply to Yusuke Suzuki from comment #16)
> (In reply to Yusuke Suzuki from comment #15)
> > Comment on attachment 417083 [details]
> > Patch
> > 
> > Can you ensure USE_APPLE_ICU is on-by-default in Apple Mac ports?
> 
> I think we should pass an option, and find an installed ICU only when an
> option is provided. It is possible that random ICU is installed in the
> system (due to homebrew etc.) and I think JSCOnly and Mac ports should not
> select these ICU by default.

No, this patch could also make Mac port able to built with non-Apple ICU. In OptionsMac.cmake (below), it will find LibXml2 and LibXslt in normal paths (a homebrew one maybe), and ICU seemingly should not be an exception.

If a user (not apple official) want to build their own Mac port with own ICU, I didn't find restriction to stick to Apple ICU. Besides, this patch would print out which ICU to be used, I think it will reduce misusing an unexpected lib.

Alex and Yusuke, former attachment 416707 [details] is the option-used implement, is it preferred in your opinions?


```
find_package(ICU 60.2 REQUIRED COMPONENTS data i18n uc)
find_package(LibXml2 2.8.0 REQUIRED)
find_package(LibXslt 1.1.7 REQUIRED)
```

[Yusuke Suzuki]

(In reply to monson from comment #17)
> (In reply to Yusuke Suzuki from comment #16)
> > (In reply to Yusuke Suzuki from comment #15)
> > > Comment on attachment 417083 [details]
> > > Patch
> > > 
> > > Can you ensure USE_APPLE_ICU is on-by-default in Apple Mac ports?
> > 
> > I think we should pass an option, and find an installed ICU only when an
> > option is provided. It is possible that random ICU is installed in the
> > system (due to homebrew etc.) and I think JSCOnly and Mac ports should not
> > select these ICU by default.
> 
> No, this patch could also make Mac port able to built with non-Apple ICU. In
> OptionsMac.cmake (below), it will find LibXml2 and LibXslt in normal paths
> (a homebrew one maybe), and ICU seemingly should not be an exception.
> 
> If a user (not apple official) want to build their own Mac port with own
> ICU, I didn't find restriction to stick to Apple ICU. Besides, this patch
> would print out which ICU to be used, I think it will reduce misusing an
> unexpected lib.
> 
> Alex and Yusuke, former attachment 416707 [details] is the option-used
> implement, is it preferred in your opinions?
> 
> 
> ```
> find_package(ICU 60.2 REQUIRED COMPONENTS data i18n uc)
> find_package(LibXml2 2.8.0 REQUIRED)
> find_package(LibXslt 1.1.7 REQUIRED)
> ```

Yes, I think we should use AppleICU on Mac and JSCOnly ports in a default configuration.

Using user-specified ICU is awesome. I really love this feature, and this is really exciting to me as a person who implemented most of Intl features in JavaScriptCore lately.
This dramatically makes my work easy.

But JSCOnly ports are actively used in the other places. And ICU can be installed unintentionally due to homebrew's dependency or something else.
At that time, we do not want to pick that ICU implicitly, which makes build configuration complicated.
And this makes getting uniform build product of JSCOnly ports in a given macOS environment difficult.

ICU is really complicated library which includes bunch of changes in every release (CLDR changes etc.).
So when building, what ICU is used is very important.

So, passing explicit option (not using AppleICU) is better in JSCOnly and Mac ports. This way offers the way to enable user-installed ICU feature, while it keeps current JSCOnly / Mac w/ AppleICU built without considering what libraries are installed.

[Konstantin Tokarev]

Also note that there are cases when automatic fallback to Apple's ICU is undesirable behavior. For example, if someone is building binaries for distribution in AppStore, using system ICU is not allowed as it's considered to be private system API.

[Konstantin Tokarev]

In fact, it might be better to use different cmake package names for "standard" and "Apple" ICU so that it would be obvious if find_package() call site what is going to be used, and port could customize fallback order if it's needed at all.

[monson]

Comment on attachment 416707 [details]
Patch

Hi Yusuke, please review Attachment 416707 [details] if you like that. The new option USE_APPLE_ICU will use ${APPLE} as default, so it won't change behavior of current built if not set.

As for Konstantin's idea, maybe it needs someone from apple to give patch because they might be affected most.

[Yusuke Suzuki]

Comment on attachment 416707 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416707&action=review

>>> Source/cmake/OptionsCommon.cmake:103
>>> +option(USE_APPLE_ICU "Use Apple's internal ICU" ${APPLE})
>> 
>> Should this be in OptionsMac.cmake  instead of here?
> 
> It's not about PORT Mac but about OS(Darwin), it affects other ports. For GTK on Darwin, it usually prefers self-compiled version-specified ICU instead of the Apple one, mainly in the case of cross-platform applications keeping their dependencies unified.
> 
> An alternative to option USE_APPLE_ICU is, we search ICU normally first, if not found, then fallback to use the Apple shipped one. I can make another patch if you like this approach.

I think this is OK for the first patch. Then, later, we can put USE_APPLE_ICU to OptionsGTK.cmake / OptionsWPT.cmake etc. to explicitly configure it in each ports. For now, let's put the global status-quo option here since we would like to keep Mac and JSCOnly using AppleICU by-default (while having an option to use user-installed ICU is *AWESOME*, it actually streamlines my Intl work in JSC). Previously, to test whether the given ICU bug is fixed in very new ICU, I was using Linux box. But with this option, I can simply do it in macOS.

[Yusuke Suzuki]

(In reply to monson from comment #21)
> Comment on attachment 416707 [details]
> Patch
> 
> Hi Yusuke, please review Attachment 416707 [details] if you like that. The
> new option USE_APPLE_ICU will use ${APPLE} as default, so it won't change
> behavior of current built if not set.
> 
> As for Konstantin's idea, maybe it needs someone from apple to give patch
> because they might be affected most.

Nice, I think this patch looks quite good for the first step. This does not change the status-quo, while it adds new great feature (switching ICU based on option).

[EWS]

Committed r271257: <https://trac.webkit.org/changeset/271257>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416707 [details].