Bug 77118 – [Qt][Mac] Build fails after adding ICU support (r105997).

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Zeno Albisser]

Mac OS does not officially provide libicu. Even though the library is actually being shipped with the OS, the headers are missing.
The WebKit repository therefore includes matching headers for libicu on Mac OS.
We should use these, as the Apple port does, and link to the library provided by the OS.

[Zeno Albisser]

Created attachment 124153 [details]
patch for review.

[Tor Arne Vestbø]

Comment on attachment 124153 [details]
patch for review.

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

> Source/WTF/WTF.pri:29
> -haveQt(5):contains(QT_CONFIG,icu) {
> -    unix:!mac: LIBS += $$system(icu-config --ldflags-searchpath --ldflags-libsonly)
> -    else: LIBS += -licuin
> +haveQt(5):if(contains(QT_CONFIG,icu)|mac) {
> +    unix {
> +        mac: {
> +            INCLUDEPATH += $${ROOT_WEBKIT_DIR}/Source/WTF/icu
> +            LIBS += -licucore
> +        } else: LIBS += $$system(icu-config --ldflags-searchpath --ldflags-libsonly)
> +    } else: LIBS += -licuin
>  } else {
>      haveQt(5): error("To build QtWebKit with Qt 5 you need ICU")
>  }

What about Qt4?

If that's handled already, I'd like to see this in a haveQt(5) { } block, instead of the checks and errors both prefixed. 

Also, no need for : when you do {. 

And since you break  the mac bits into multiple lines, please also break the else. makes it easier to read.

[Zeno Albisser]

Created attachment 124162 [details]
patch for review.

[Kenneth Rohde Christiansen]

Comment on attachment 124162 [details]
patch for review.

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

Looks ok, but maybe someone has better ideas as I am no build expert :) Maybe stupid question but could icu be part of QT_CONFIG if on mac? instead of doing all those |mac

> Source/WTF/WTF.pri:24
> +    mac {
> +            INCLUDEPATH += $${ROOT_WEBKIT_DIR}/Source/WTF/icu
> +            LIBS += -licucore
> +    } else {

indendation seems wrong here

[Zeno Albisser]

(In reply to comment #4)
> (From update of attachment 124162 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124162&action=review
> 
> Looks ok, but maybe someone has better ideas as I am no build expert :) Maybe stupid question but could icu be part of QT_CONFIG if on mac? instead of doing all those |mac

I don't think it is an option to alter QT_CONFIG for that purpose from our side. QT_CONFIG is the configuration of Qt itself.
Ideally of course Qt would already have been compiled with icu support on mac. But it is obviously a rather controversial topic to rely on a library where the headers are not shipping with the OS / developer offering.
Nevertheless i think it is safe for us to do so, because we already provide the missing headers in WebKit.

> 
> > Source/WTF/WTF.pri:24
> > +    mac {
> > +            INCLUDEPATH += $${ROOT_WEBKIT_DIR}/Source/WTF/icu
> > +            LIBS += -licucore
> > +    } else {
> 
> indendation seems wrong here

I will fix that one. :)

[Zeno Albisser]

Created attachment 124300 [details]
patch for review. - rebased and fixed indentation.

[Tor Arne Vestbø]

Comment on attachment 124300 [details]
patch for review. - rebased and fixed indentation.

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

r=me with some comments sprinkled around and tiny fix

> Source/WTF/WTF.pri:22
> +        INCLUDEPATH += $${ROOT_WEBKIT_DIR}/Source/WTF/icu

Perhaps a comment here about why we're getting the headers from the webkit sources, but the lib from the system.

> Source/WTF/WTF.pri:29
> +            haveQt(5): error("To build QtWebKit with Qt 5 you need ICU")

Already in haveQt(5) scope, no need for this one

> Source/WebCore/Target.pri:2874
> +        SOURCES += editing/SmartReplaceCF.cpp

Perhaps a comment on why we use the CF one on mac (that it matches what the mac port does)

> Tools/qmake/mkspecs/features/features.prf:33
> +haveQt(5):if(contains(QT_CONFIG,icu)|mac) {

Perhaps a comment on why we don't rely on QT_CONFIG for mac (since it's not an official support for all of qt)

[Zeno Albisser]

Committed r106118: <http://trac.webkit.org/changeset/106118>