Bug 77118 - [Qt][Mac] Build fails after adding ICU support (r105997).
Summary: [Qt][Mac] Build fails after adding ICU support (r105997).
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zeno Albisser
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-26 10:35 PST by Zeno Albisser
Modified: 2012-01-27 06:42 PST (History)
2 users (show)

See Also:


Attachments
patch for review. (5.32 KB, patch)
2012-01-26 11:24 PST, Zeno Albisser
no flags Details | Formatted Diff | Diff
patch for review. (5.88 KB, patch)
2012-01-26 12:29 PST, Zeno Albisser
no flags Details | Formatted Diff | Diff
patch for review. - rebased and fixed indentation. (6.01 KB, patch)
2012-01-27 06:02 PST, Zeno Albisser
vestbo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zeno Albisser 2012-01-26 10:35:07 PST
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.
Comment 1 Zeno Albisser 2012-01-26 11:24:46 PST
Created attachment 124153 [details]
patch for review.
Comment 2 Tor Arne Vestbø 2012-01-26 11:35:28 PST
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.
Comment 3 Zeno Albisser 2012-01-26 12:29:42 PST
Created attachment 124162 [details]
patch for review.
Comment 4 Kenneth Rohde Christiansen 2012-01-26 15:05:17 PST
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
Comment 5 Zeno Albisser 2012-01-27 02:09:57 PST
(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. :)
Comment 6 Zeno Albisser 2012-01-27 06:02:03 PST
Created attachment 124300 [details]
patch for review. - rebased and fixed indentation.
Comment 7 Tor Arne Vestbø 2012-01-27 06:06:45 PST
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)
Comment 8 Zeno Albisser 2012-01-27 06:42:24 PST
Committed r106118: <http://trac.webkit.org/changeset/106118>