Bug 70913 - using extrernal ICU library on case unsensitive drives will not work
Summary: using extrernal ICU library on case unsensitive drives will not work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
: 72152 88307 (view as bug list)
Depends on:
Blocks: 70914 88300
  Show dependency treegraph
 
Reported: 2011-10-26 06:14 PDT by Onne Gorter
Modified: 2017-04-16 19:47 PDT (History)
11 users (show)

See Also:


Attachments
patch (21.90 KB, patch)
2011-10-26 06:14 PDT, Onne Gorter
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
refreshed, removing the U_DISABLE_RENAMING (21.40 KB, patch)
2011-10-27 00:32 PDT, Onne Gorter
no flags Details | Formatted Diff | Diff
Patch (57.79 KB, patch)
2012-06-08 01:10 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (647.55 KB, application/zip)
2012-06-08 02:23 PDT, WebKit Review Bot
no flags Details
Patch (4.75 KB, patch)
2012-06-11 11:05 PDT, Jocelyn Turcotte
ossy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Onne Gorter 2011-10-26 06:14:13 PDT
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
Comment 1 Onne Gorter 2011-10-26 06:14:57 PDT
Created attachment 112493 [details]
patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2011-10-26 06:43:12 PDT
For this bug and bug 70914 you need to at least flag the patch with an "r?" so reviewers can notice they exist.
Comment 3 WebKit Review Bot 2011-10-26 06:43:37 PDT
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.
Comment 4 WebKit Review Bot 2011-10-26 07:13:28 PDT
Comment on attachment 112493 [details]
patch

Attachment 112493 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10213584
Comment 5 Gyuyoung Kim 2011-10-26 07:18:26 PDT
Comment on attachment 112493 [details]
patch

Attachment 112493 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10208754
Comment 6 Gustavo Noronha (kov) 2011-10-26 08:06:41 PDT
Comment on attachment 112493 [details]
patch

Attachment 112493 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10213599
Comment 7 Alexey Proskuryakov 2011-10-26 12:17:34 PDT
> 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.
Comment 8 Lucas De Marchi 2011-10-26 16:38:30 PDT
(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.
Comment 9 Onne Gorter 2011-10-27 00:31:51 PDT
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.
Comment 10 Onne Gorter 2011-10-27 00:32:27 PDT
Created attachment 112645 [details]
refreshed, removing the U_DISABLE_RENAMING
Comment 11 Onne Gorter 2011-10-27 00:55:12 PDT
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 ...
Comment 12 WebKit Review Bot 2011-10-27 06:08:42 PDT
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.
Comment 13 Alexey Proskuryakov 2011-10-27 09:00:26 PDT
> 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.
Comment 14 Onne Gorter 2011-10-27 09:16:14 PDT
> 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).
Comment 15 Daniel Bates 2011-11-11 11:13:40 PST
*** Bug 72152 has been marked as a duplicate of this bug. ***
Comment 16 Csaba Osztrogonác 2012-06-08 00:20:52 PDT
*** Bug 88307 has been marked as a duplicate of this bug. ***
Comment 17 Csaba Osztrogonác 2012-06-08 00:28:38 PDT
(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.
Comment 18 Csaba Osztrogonác 2012-06-08 01:10:16 PDT
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.
Comment 19 WebKit Review Bot 2012-06-08 01:14:27 PDT
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.
Comment 20 Build Bot 2012-06-08 01:51:14 PDT
Comment on attachment 146502 [details]
Patch

Attachment 146502 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12917443
Comment 21 WebKit Review Bot 2012-06-08 02:23:17 PDT
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
Comment 22 WebKit Review Bot 2012-06-08 02:23:31 PDT
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
Comment 23 Alexey Proskuryakov 2012-06-08 10:57:10 PDT
> 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.
Comment 24 Jocelyn Turcotte 2012-06-11 11:05:28 PDT
Created attachment 146876 [details]
Patch

Solving the issue differently, following comment #7.
Comment 25 Csaba Osztrogonác 2012-06-12 06:17:19 PDT
(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 ... )
Comment 26 Csaba Osztrogonác 2012-06-12 06:21:40 PDT
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.
Comment 27 Jocelyn Turcotte 2012-06-12 08:05:07 PDT
Committed r120076: <http://trac.webkit.org/changeset/120076>