Bug 70913 – using extrernal ICU library on case unsensitive drives will not work

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Onne Gorter]

it confuses wtf/unicode/UTF8.h with unicode/utf8.h

this patch fixes that by renaming UTF8.h to WTFUTF8.h

It patches all includes and patches all build systems

[Onne Gorter]

Created attachment 112493 [details]
patch

[Raphael Kubo da Costa (:rakuco)]

For this bug and bug 70914 you need to at least flag the patch with an "r?" so reviewers can notice they exist.

[WebKit Review Bot]

Attachment 112493 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.cpp',..." exit_code: 1

Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:26:  #ifndef header guard has wrong style, please use: WTF_WTFUTF8_h  [build/header_guard] [5]
Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:45:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:46:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:47:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:48:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:53:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:75:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 112493 [details]
patch

Attachment 112493 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10213584

[Gyuyoung Kim]

Comment on attachment 112493 [details]
patch

Attachment 112493 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10208754

[Gustavo Noronha (kov)]

Comment on attachment 112493 [details]
patch

Attachment 112493 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10213599

[Alexey Proskuryakov]

> it confuses wtf/unicode/UTF8.h with unicode/utf8.h

What exactly are the errors?

The idea is that WTF header is always included as <wtf/unicode/UTF8.h>, while WebCore one is included as "unicode/utf8.h". I don't see how a compiler can be confused about that.

No other ICU headers should be in the search path.

[Lucas De Marchi]

(In reply to comment #0)
> it confuses wtf/unicode/UTF8.h with unicode/utf8.h
>
> this patch fixes that by renaming UTF8.h to WTFUTF8.h
> 
> It patches all includes and patches all build systems

Please, post the error message.

[Onne Gorter]

Local .h and .cpp files (internal icu and internal wtf/unicode) include them as "utf8.h" and "UTF8.h" respectively. (E.g. wtf/unicode/UTF8.cpp)

Since we cannot fix icu, and we cannot simply swap the include order, we have to rename UTF8.h. (Or use a case sensitive drive, ...)

I've refreshed the patch, removing the stale U_DISABLE_RENAMING, should fix other platforms building it.

[Onne Gorter]

Created attachment 112645 [details]
refreshed, removing the U_DISABLE_RENAMING

[Onne Gorter]

this is the error without the patch:

In file included from /Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp:28:
/Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.h:50: error: 'U8_MAX_LENGTH' was not declared in this scope
/Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp: In member function 'void WebCore::TextCodecUTF8::consumePartialSequenceByte()':
/Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp:156: error: 'm_partialSequence' was not declared in this scope
/Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp: In member function 'void WebCore::TextCodecUTF8::handlePartialSequence(UChar*&, const uint8_t*&, const uint8_t*, bool, bool, bool&)':
/Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp:173: error: 'm_partialSequence' was not declared in this scope
/Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp:178: error: 'm_partialSequence' was not declared in this scope
/Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp: In member function 'virtual WTF::String WebCore::TextCodecUTF8::decode(const char*, size_t, bool, bool, bool&)':
/Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp:271: error: 'm_partialSequence' was not declared in this scope
/Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp: In member function 'virtual WTF::CString WebCore::TextCodecUTF8::encode(const UChar*, size_t, WebCore::UnencodableHandling)':
/Users/ogorter/WebKit/Source/WebCore/platform/text/TextCodecUTF8.cpp:310: error: 'U8_APPEND_UNSAFE' was not declared in this scope
make[2]: *** [WebCore/CMakeFiles/webcore_efl.dir/platform/text/TextCodecUTF8.cpp.o] Error 1
make[1]: *** [WebCore/CMakeFiles/webcore_efl.dir/all] Error 2
make: *** [all] Error 2


U8_MAC_LENGTH is defined in unicode/utf8.h included (very) indirectly from unicode/uchar.h but the include there redirects to wtf/unicode/UTF8.h on a case sensitive drive ...

[WebKit Review Bot]

Attachment 112645 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.cpp',..." exit_code: 1

Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:26:  #ifndef header guard has wrong style, please use: WTF_WTFUTF8_h  [build/header_guard] [5]
Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:45:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:46:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:47:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:48:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:53:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/wtf/unicode/WTFUTF8.h:75:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

> Since we cannot fix icu

I still don't understand what the problem is, but if really needed, we could changes our copy of ICU headers. Again, building with any other ICU headers is not supposed to work.

[Onne Gorter]

> building with any other ICU headers
well ... that doesn't make much sense, but that is not the issue

actually, you might be right on how the headers are included:
ICU headers use <unicode/utf8.h> or "utf8.h"
and wtf uses <wtf/unicode/UTF8.h> or "UTF8.h"

so it might just work if I remove the wtf/unicode from the search path *and* move the order around. If that works, the issue can be solved in the CMake (or any other build system that might run into this).

[Daniel Bates]

*** Bug 72152 has been marked as a duplicate of this bug. ***

[Csaba Osztrogonác]

*** Bug 88307 has been marked as a duplicate of this bug. ***

[Csaba Osztrogonác]

(In reply to comment #16)
> *** Bug 88307 has been marked as a duplicate of this bug. ***

I made digging and you can find what the exact problem is: https://bugs.webkit.org/show_bug.cgi?id=88307#c2 

In nutshell: An ICU header (ICU/include/unicode/utf.h) includes "#include "unicode/utf8.h". It wants to include its own unicode/utf8.h, not WTF's unicode/UTF8.h. They are completely different.

(In reply to comment #13)
> > Since we cannot fix icu
> 
> I still don't understand what the problem is, but if really needed, we could changes our copy of ICU headers. Again, building with any other ICU headers is not supposed to work.

I think only Apple port uses the ICU headers checked into the WebKit 
source, all other ports use the installed system headers. Similar to
other 3rd party dev packages.

[Csaba Osztrogonác]

Created attachment 146502 [details]
Patch

Based on my previous patch in Bug 88307 and some style fix with check-webkit-stlye suggested in the previous bug.

[WebKit Review Bot]

Attachment 146502 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.cpp',..." exit_code: 1
Source/WTF/wtf/unicode/WTFUTF8.h:45:  One space before end of line comments  [whitespace/comments] [5]
Source/WTF/wtf/unicode/WTFUTF8.h:46:  One space before end of line comments  [whitespace/comments] [5]
Source/WTF/wtf/unicode/WTFUTF8.h:47:  One space before end of line comments  [whitespace/comments] [5]
Source/WTF/wtf/unicode/WTFUTF8.h:48:  One space before end of line comments  [whitespace/comments] [5]
Source/WTF/wtf/unicode/WTFUTF8.h:79:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WTF/wtf/unicode/WTFUTF8.cpp:65:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WTF/wtf/unicode/WTFUTF8.cpp:264:  More than one command on the same line in if  [whitespace/parens] [4]
Source/WTF/wtf/unicode/WTFUTF8.cpp:266:  More than one command on the same line in if  [whitespace/parens] [4]
Source/WTF/wtf/unicode/WTFUTF8.cpp:268:  More than one command on the same line in if  [whitespace/parens] [4]
Source/WTF/wtf/unicode/WTFUTF8.cpp:273:  More than one command on the same line in if  [whitespace/parens] [4]
Source/WTF/wtf/unicode/WTFUTF8.cpp:275:  More than one command on the same line in if  [whitespace/parens] [4]
Source/WTF/wtf/unicode/WTFUTF8.cpp:277:  More than one command on the same line in if  [whitespace/parens] [4]
Source/WTF/wtf/unicode/WTFUTF8.cpp:279:  More than one command on the same line in if  [whitespace/parens] [4]
Source/WTF/wtf/unicode/WTFUTF8.cpp:281:  More than one command on the same line in if  [whitespace/parens] [4]
Source/WTF/wtf/unicode/WTFUTF8.cpp:284:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 15 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 146502 [details]
Patch

Attachment 146502 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12917443

[WebKit Review Bot]

Comment on attachment 146502 [details]
Patch

Attachment 146502 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12909577

New failing tests:
svg/text/non-bmp-positioning-lists.svg
fast/parser/entities-in-xhtml.xhtml
fast/url/anchor.html
fast/encoding/xml-utf-8-default.xml

[WebKit Review Bot]

Created attachment 146518 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexey Proskuryakov]

> I think only Apple port uses the ICU headers checked into the WebKit 
> source, all other ports use the installed system headers. Similar to
> other 3rd party dev packages.

This sounds like the root of the problem then. Using a different copy of headers does nothing good, and just causes build issues. If there is something new in system copy, you generally still cannot use it in WebKit code without breaking other platforms anyway.

> WTFUTF8.h

I hate this name.

[Jocelyn Turcotte]

Created attachment 146876 [details]
Patch

Solving the issue differently, following comment #7.

[Csaba Osztrogonác]

(In reply to comment #23)
> > I think only Apple port uses the ICU headers checked into the WebKit 
> > source, all other ports use the installed system headers. Similar to
> > other 3rd party dev packages.
> 
> This sounds like the root of the problem then. Using a different copy of headers does nothing good, and just causes build issues. If there is something new in system copy, you generally still cannot use it in WebKit code without breaking other platforms anyway.

No, the root of the problem isn't the checked in ICU headers (used by only Apple, because we can't expect from developers to install ICU-dev packages). The root of the problem is that clashes between wtf/unicode/UTF8.h (which is absolutely independent of ICU, it is a WebKit header) and ICU's unicode/utf8.h header (which has same name in WebKit/Source/WTF/icu and other ICU's)

Otherwise other ports shouldn't/can't use checked in ICU headers, so
renaming checked in ICU headers won't work. See WebKit/Source/WTF/icu/README:
"The headers in this directory are for compiling on Mac OS X 10.4.
The Mac OS X 10.4 release includes the ICU binary, but not ICU headers.
For other platforms, installed ICU headers should be used rather than these.
They are specific to Mac OS X 10.4."

 
> > WTFUTF8.h
> I hate this name.

It wasn't so constructive. Could you suggest a better name, please?
( I prefer renaming a WebKit header instead of suggesting renaming a 3rd header ... )

[Csaba Osztrogonác]

Comment on attachment 146876 [details]
Patch

r=me, we really need this fix. 

side note:
Unfortunately this is a Qt only fix, and it can occur again in the 
future if somebody adds back WTF/wtf to the searchpath for some reason.

Otherwise other ports have same problem can follow this way.

[Jocelyn Turcotte]

Committed r120076: <http://trac.webkit.org/changeset/120076>