Bug 220081 - [CMake] Use Apple's ICU as fallback
Summary: [CMake] Use Apple's ICU as fallback
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CMake (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-22 00:48 PST by monson
Modified: 2021-01-07 12:56 PST (History)
14 users (show)

See Also:


Attachments
Patch (3.34 KB, patch)
2020-12-22 00:50 PST, monson
no flags Details | Formatted Diff | Diff
Patch (3.59 KB, patch)
2020-12-23 04:07 PST, monson
no flags Details | Formatted Diff | Diff
Patch (3.61 KB, patch)
2020-12-25 00:39 PST, monson
no flags Details | Formatted Diff | Diff
Patch (4.10 KB, patch)
2021-01-06 07:36 PST, monson
don.olmstead: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description monson 2020-12-22 00:48:04 PST
[CMake] Add USE_APPLE_ICU option
Comment 1 monson 2020-12-22 00:50:27 PST
Created attachment 416654 [details]
Patch
Comment 2 monson 2020-12-22 00:55:00 PST
Add USE_APPLE_ICU option to allow non-Mac ports (GTK or JSCOnly) on Darwin could build with non-Apple ICU.
Comment 3 Radar WebKit Bug Importer 2020-12-22 11:19:40 PST
<rdar://problem/72593973>
Comment 4 Fujii Hironori 2020-12-22 13:44:34 PST
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?
Comment 5 Fujii Hironori 2020-12-22 13:46:22 PST
I'm sorry. I didn't read your comment#2.
Comment 6 Fujii Hironori 2020-12-22 13:49:11 PST
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.
Comment 7 monson 2020-12-23 04:07:56 PST
Created attachment 416707 [details]
Patch
Comment 8 Alex Christensen 2020-12-23 08:49:48 PST
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?
Comment 9 monson 2020-12-23 10:01:51 PST
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.
Comment 10 monson 2020-12-25 00:39:29 PST
Created attachment 416754 [details]
Patch
Comment 11 Don Olmstead 2021-01-04 09:43:27 PST
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?
Comment 12 monson 2021-01-06 05:33:21 PST
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.
Comment 13 monson 2021-01-06 07:36:48 PST
Created attachment 417083 [details]
Patch
Comment 14 Don Olmstead 2021-01-06 08:03:57 PST
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.
Comment 15 Yusuke Suzuki 2021-01-06 12:40:36 PST
Comment on attachment 417083 [details]
Patch

Can you ensure USE_APPLE_ICU is on-by-default in Apple Mac ports?
Comment 16 Yusuke Suzuki 2021-01-06 13:12:51 PST
(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.
Comment 17 monson 2021-01-06 20:02:55 PST
(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)
```
Comment 18 Yusuke Suzuki 2021-01-06 20:31:03 PST
(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.
Comment 19 Konstantin Tokarev 2021-01-06 20:34:53 PST
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.
Comment 20 Konstantin Tokarev 2021-01-06 20:39:02 PST
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.
Comment 21 monson 2021-01-06 22:37:08 PST
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.
Comment 22 Yusuke Suzuki 2021-01-06 22:40:11 PST
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.
Comment 23 Yusuke Suzuki 2021-01-06 22:40:57 PST
(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).
Comment 24 EWS 2021-01-07 12:56:34 PST
Committed r271257: <https://trac.webkit.org/changeset/271257>

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