WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
167343
webkit-gtk-2.15.3 fails to build on macOS due to missing declaration of U8_MAX_LENGTH
https://bugs.webkit.org/show_bug.cgi?id=167343
Summary
webkit-gtk-2.15.3 fails to build on macOS due to missing declaration of U8_MA...
Jeremy Huddleston Sequoia
Reported
2017-01-23 16:57:58 PST
webkit-gtk-2.15.3 fails to build on macOS (darwin/gtk/x11) with the following error. This is a regression from 2.15.2: /opt/local/var/macports/build/_Users_jeremy_src_macports_macports-ports_www_webkit2-gtk-devel/webkit2-gtk-devel/work/webkitgtk-2.15.3/Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:146:32: error: use of undeclared identifier 'U8_MAX_LENGTH'; did you mean 'UITER_LENGTH'? LChar utf8OctetsBuffer[U8_MAX_LENGTH]; ^~~~~~~~~~~~~ UITER_LENGTH /opt/local/include/unicode/uiter.h:50:58: note: 'UITER_LENGTH' declared here UITER_START, UITER_CURRENT, UITER_LIMIT, UITER_ZERO, UITER_LENGTH ^ /opt/local/var/macports/build/_Users_jeremy_src_macports_macports-ports_www_webkit2-gtk-devel/webkit2-gtk-devel/work/webkitgtk-2.15.3/Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:151:9: error: use of undeclared identifier 'U8_APPEND_UNSAFE' U8_APPEND_UNSAFE(utf8OctetsBuffer, utf8Length, codePoint); ^
Attachments
Rename Source/WTF/wtf/unicode/UTF8.h to .../wtf_utf8.h
(7.29 MB, patch)
2017-04-18 17:54 PDT
,
John Ralls
no flags
Details
Formatted Diff
Diff
Rename Source/WTF/wtf/unicode/UTF8.h to .../wtf_utf8.h, this time without the transposition.
(7.29 MB, patch)
2017-04-18 17:56 PDT
,
John Ralls
mcatanzaro
: review-
Details
Formatted Diff
Diff
Rename Source/WTF/wtf/unicode/UTF8.h to .../wtf_utf8.h with corrections as requested.
(61.54 KB, application/mbox)
2017-04-20 12:24 PDT
,
John Ralls
no flags
Details
Correct patch
(1.37 KB, patch)
2017-04-20 12:38 PDT
,
Konstantin Tokarev
no flags
Details
Formatted Diff
Diff
patch
(1.97 KB, patch)
2017-04-20 12:46 PDT
,
Konstantin Tokarev
no flags
Details
Formatted Diff
Diff
Patch
(5.84 KB, patch)
2017-04-21 08:30 PDT
,
Konstantin Tokarev
mcatanzaro
: review+
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
John Ralls
Comment 1
2017-04-18 17:54:06 PDT
Created
attachment 307443
[details]
Rename Source/WTF/wtf/unicode/UTF8.h to .../wtf_utf8.h This problem has a long history: Bugs
63948
,
65811
,
70913
,
72152
, and 88307. The "fix" implemented in a patch at
bug 70913
worked until CMake came along, but now CMake's dependency-discovery mechanism appends Source/WTF/wtf to the include list and ICU's #include "unicode/utf8.h" (in unicode/utf.h) finds it again. This patch renames the offending UTF8.h and UTF8.cpp to wtf_utf8.h and wtf_utf8.cpp, which is what should have been done in the first place.
John Ralls
Comment 2
2017-04-18 17:56:54 PDT
Created
attachment 307444
[details]
Rename Source/WTF/wtf/unicode/UTF8.h to .../wtf_utf8.h, this time without the transposition.
Build Bot
Comment 3
2017-04-18 18:04:51 PDT
Attachment 307444
[details]
did not pass style-queue: ERROR: Source/WTF/wtf/unicode/wtf_utf8.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] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:122: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:221: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:234: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:234: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:235: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:236: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:247: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:254: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:256: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:256: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:257: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:257: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:258: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:262: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:262: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:263: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:264: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:265: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:266: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:270: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:288: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:288: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:289: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:290: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:291: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:292: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:330: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.cpp:414: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WTF/wtf/unicode/wtf_utf8.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/unicode/wtf_utf8.h:38: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WTF/wtf/unicode/wtf_utf8.h:46: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WTF/wtf/unicode/wtf_utf8.h:46: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WTF/wtf/unicode/wtf_utf8.h:47: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WTF/wtf/unicode/wtf_utf8.h:48: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WTF/wtf/unicode/wtf_utf8.h:49: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WTF/wtf/unicode/wtf_utf8.h:54: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WTF/wtf/unicode/wtf_utf8.h:67: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WTF/wtf/unicode/wtf_utf8.h:71: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WTF/wtf/unicode/wtf_utf8.h:75: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 40 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Sam Weinig
Comment 4
2017-04-18 19:23:27 PDT
(In reply to John Ralls from
comment #1
)
> Created
attachment 307443
[details]
> Rename Source/WTF/wtf/unicode/UTF8.h to .../wtf_utf8.h > > This problem has a long history: Bugs
63948
,
65811
,
70913
,
72152
, and 88307. > The "fix" implemented in a patch at
bug 70913
worked until CMake came along, > but now CMake's dependency-discovery mechanism appends Source/WTF/wtf to the > include list and ICU's #include "unicode/utf8.h" (in unicode/utf.h) finds it > again. > > This patch renames the offending UTF8.h and UTF8.cpp to wtf_utf8.h and > wtf_utf8.cpp, which is what should have been done in the first place.
wtf_utf8.h/cpp doesn't really match out naming at all. If this rename does need to happen, though I admit, I don't quite understand why that is, it should probably follow the String classes lead, and be called WTFUTF8.h, hideous though that be.
Michael Catanzaro
Comment 5
2017-04-18 19:53:51 PDT
(In reply to Sam Weinig from
comment #4
)
> wtf_utf8.h/cpp doesn't really match out naming at all. If this rename does > need to happen, though I admit, I don't quite understand why that is, it > should probably follow the String classes lead, and be called WTFUTF8.h, > hideous though that be.
Yeah, that's the name I would choose. It also matches WTFThreadData.h. The problem is that both ICU and WTF provide unicode/UTF8.h, they're both in the include path, and the wrong one is being chosen in this case.
John Ralls
Comment 6
2017-04-18 19:59:36 PDT
> Yeah, that's the name I would choose. It also matches WTFThreadData.h.
OK, I'll redo the patch. It'll have to wait till Thursday.
> The problem is that both ICU and WTF provide unicode/UTF8.h, they're both in the include path, and the wrong one is being chosen in this case.
That's half the problem. The other half is that MacOS's default fils system is case-preserving but not case-sensitive. ICU actually provides unicode/utf8.h which is a different filename on most Linux file systems but the same on MacOS.
Michael Catanzaro
Comment 7
2017-04-18 20:06:18 PDT
Comment on
attachment 307444
[details]
Rename Source/WTF/wtf/unicode/UTF8.h to .../wtf_utf8.h, this time without the transposition. Thanks for your patch. This does need some work. First off, and most importantly, the patch should not touch the hand-written Makefiles, since those are part of the XCode build. It should certainly not replace them with CMake-generated files, which are not source and should not be under version control. I haven't reviewed the rest of the changes since the vast majority of the diff is changes to the Makefiles, so I'll look over the rest of the patch once those changes are removed. Note: you should use a separate build directory in order to ensure that generated files like these Makefiles aren't created in the source directory; I'm kind of surprised that an in-tree build worked at all, since we've never tested or supported that. You can ignore all the complaints from style checker, since this is a straight rename, but you can't ignore the red Early Warning System (EWS) bots. This broke the iOS Simulator, JSC, and Mac EWS bots. The problem is that the XCode build needs to be updated to account for the rename in addition to the CMake build; those bots don't use CMake at all. Normally I would ask someone from Apple to do that, but since you use macOS I guess you can probably handle it yourself? If not, perhaps Sam would be willing to help with that. Please use Tools/Scripts/prepare-ChangeLog to prepare your changelog entries. In particular, note that you won't actually modify the toplevel ChangeLog file, but the ChangeLog files in the JavaScriptCore, WTF, WebCore, and WebKit2 directories. It will say "Reviewed by NOBODY (OOPS!)" and it's best to leave that in the patch you upload, since our tools will handle replacing it automatically.
Konstantin Tokarev
Comment 8
2017-04-19 00:49:24 PDT
I think we should not rename UTF8.h, but rather wtf/unicode, to prevent possibility of future file name conflicts.
John Ralls
Comment 9
2017-04-20 12:06:26 PDT
> I think we should not rename UTF8.h, but rather wtf/unicode, to prevent possibility of future file name conflicts.
OK, I suppose. A somewhat more extensive change (72 files vs. 14 when changing only UTF8.h & cpp). Would the new directory name be Source/WTF/wtf/WTFunicode?
John Ralls
Comment 10
2017-04-20 12:24:54 PDT
Created
attachment 307612
[details]
Rename Source/WTF/wtf/unicode/UTF8.h to .../wtf_utf8.h with corrections as requested. Sorry about all of the noise in the previous iteration. It didn't occur to me that there'd be hand-built Makefiles when there's a CMake implementation too. I didn't get any messages from EWS. Should I have? I did a test-build in Xcode which completed successfully, but that would be of Safari's backend rather than WebKitGtk.
Build Bot
Comment 11
2017-04-20 12:26:56 PDT
Attachment 307612
[details]
did not pass style-queue: ERROR: Source/WTF/wtf/unicode/WTFUTF8.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/WTF/wtf/unicode/WTFUTF8.h:38: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.h:46: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WTF/wtf/unicode/WTFUTF8.h:46: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WTF/wtf/unicode/WTFUTF8.h:47: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WTF/wtf/unicode/WTFUTF8.h:48: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WTF/wtf/unicode/WTFUTF8.h:49: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WTF/wtf/unicode/WTFUTF8.h:54: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WTF/wtf/unicode/WTFUTF8.h:67: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WTF/wtf/unicode/WTFUTF8.h:71: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WTF/wtf/unicode/WTFUTF8.h:75: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WebCore/ChangeLog:10: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] ERROR: 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] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:122: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:221: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:234: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:234: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:235: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:236: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:247: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:254: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:256: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:256: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:257: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:257: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:258: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:262: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:262: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:263: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:264: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:265: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:266: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:270: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:288: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:288: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:289: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:290: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:291: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:292: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:330: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] ERROR: Source/WTF/wtf/unicode/WTFUTF8.cpp:414: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 41 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Konstantin Tokarev
Comment 12
2017-04-20 12:35:27 PDT
>Would the new directory name be Source/WTF/wtf/WTFunicode?
I'm afraid that would result in too much WTF per minute. Could you chewck if the following patch fixes the build for you? diff --git a/Source/WTF/wtf/CMakeLists.txt b/Source/WTF/wtf/CMakeLists.txt index 1bae3bb90b3..7d902484fcd 100644 --- a/Source/WTF/wtf/CMakeLists.txt +++ b/Source/WTF/wtf/CMakeLists.txt @@ -286,6 +286,12 @@ set(WTF_SOURCES set(WTF_INCLUDE_DIRECTORIES "${BMALLOC_DIR}" "${WTF_DIR}" + "${THIRDPARTY_DIR}" + "${CMAKE_BINARY_DIR}" + "${DERIVED_SOURCES_DIR}" +) + +set(WTF_PRIVATE_INCLUDE_DIRECTORIES "${WTF_DIR}/wtf" "${WTF_DIR}/wtf/dtoa" "${WTF_DIR}/wtf/persistence" @@ -293,9 +299,6 @@ set(WTF_INCLUDE_DIRECTORIES "${WTF_DIR}/wtf/text/icu" "${WTF_DIR}/wtf/threads" "${WTF_DIR}/wtf/unicode" - "${THIRDPARTY_DIR}" - "${CMAKE_BINARY_DIR}" - "${DERIVED_SOURCES_DIR}" ) set(WTF_LIBRARIES
Konstantin Tokarev
Comment 13
2017-04-20 12:36:36 PDT
I'm really sorry, here is a complete patch: diff --git a/Source/WTF/wtf/CMakeLists.txt b/Source/WTF/wtf/CMakeLists.txt index 1bae3bb90b3..7d902484fcd 100644 --- a/Source/WTF/wtf/CMakeLists.txt +++ b/Source/WTF/wtf/CMakeLists.txt @@ -286,6 +286,12 @@ set(WTF_SOURCES set(WTF_INCLUDE_DIRECTORIES "${BMALLOC_DIR}" "${WTF_DIR}" + "${THIRDPARTY_DIR}" + "${CMAKE_BINARY_DIR}" + "${DERIVED_SOURCES_DIR}" +) + +set(WTF_PRIVATE_INCLUDE_DIRECTORIES "${WTF_DIR}/wtf" "${WTF_DIR}/wtf/dtoa" "${WTF_DIR}/wtf/persistence" @@ -293,9 +299,6 @@ set(WTF_INCLUDE_DIRECTORIES "${WTF_DIR}/wtf/text/icu" "${WTF_DIR}/wtf/threads" "${WTF_DIR}/wtf/unicode" - "${THIRDPARTY_DIR}" - "${CMAKE_BINARY_DIR}" - "${DERIVED_SOURCES_DIR}" ) set(WTF_LIBRARIES diff --git a/Source/cmake/WebKitMacros.cmake b/Source/cmake/WebKitMacros.cmake index a793ce40683..a0e81ee50dd 100644 --- a/Source/cmake/WebKitMacros.cmake +++ b/Source/cmake/WebKitMacros.cmake @@ -283,6 +283,7 @@ macro(WEBKIT_FRAMEWORK _target) ${${_target}_SOURCES} ) target_include_directories(${_target} PUBLIC "$<BUILD_INTERFACE:${${_target}_INCLUDE_DIRECTORIES}>") + target_include_directories(${_target} PRIVATE "$<BUILD_INTERFACE:${${_target}_PRIVATE_INCLUDE_DIRECTORIES}>") target_link_libraries(${_target} ${${_target}_LIBRARIES}) set_target_properties(${_target} PROPERTIES COMPILE_DEFINITIONS "BUILDING_${_target}") @@ -332,7 +333,7 @@ macro(WEBKIT_CREATE_FORWARDING_HEADERS _framework) if (NOT WIN32) set(_processing_directories 0) set(_processing_files 0) - set(_target_directory "${DERIVED_SOURCES_DIR}/ForwardingHeaders/${_framework}") + set(_target_directory "${FORWARDING_HEADERS_DIR}/${_framework}") file(GLOB _files "${_target_directory}/*.h") foreach (_file ${_files})
Konstantin Tokarev
Comment 14
2017-04-20 12:38:56 PDT
Created
attachment 307617
[details]
Correct patch Please try attchaed patch
Konstantin Tokarev
Comment 15
2017-04-20 12:46:13 PDT
Created
attachment 307619
[details]
patch
Konstantin Tokarev
Comment 16
2017-04-21 08:30:02 PDT
Created
attachment 307733
[details]
Patch
Michael Catanzaro
Comment 17
2017-04-21 08:56:33 PDT
Comment on
attachment 307733
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=307733&action=review
Wow. Please rename the bug to match the changelog title.
> Source/WTF/ChangeLog:10 > + implicitly without copying directory lists around. This matches exisiting
exisiting -> existing
> Source/WTF/ChangeLog:11 > + practise for all targets except WTF, headers from which are always included
practise -> practice, unless you want to go full British
Konstantin Tokarev
Comment 18
2017-04-21 09:08:24 PDT
Thanks for proofreading!
>Please rename the bug to match the changelog title.
I think I'll open different bug then in case original issue remains, as I didn't check it (though I believe it should be fixed, also
r209665
was right between 2.15.2 and 2.15.3)
Konstantin Tokarev
Comment 19
2017-04-21 09:12:24 PDT
Comment on
attachment 307733
[details]
Patch Moved patch to
https://bugs.webkit.org/show_bug.cgi?id=171115
John Ralls
Comment 20
2017-04-24 14:15:34 PDT
Sorry for the delay. Yes, fixing the WTF includes does allow the compile to continue to the next problem (a missing #include "VisibilityChangeClient.h" in WebCore/dom/Document.cpp). I suggest that you should still change the name of UTF8.h or wtf/unicode as insurance against yet another regression. Incidentally, another similar problem looms: ICU has added sometime between 1.55 and 1.58 some macros to unicode/platform.h for adding C++11 keywords if the compiler can use them (e.g. U_NOEXCEPT). WebKit has older copies of platform.h in JavaScriptCore/icu/unicode, WebCore/icu/unicode/platform.h, and WTF/icu/unicode/platform.h that are inserted into the include chain for ICU, and that breaks the declarations in the ICU headers because the new macros aren't defined.
Michael Catanzaro
Comment 21
2017-04-24 18:40:50 PDT
We need to make sure these third-party ICU directories are never added to the include path for CMake builds.
Konstantin Tokarev
Comment 22
2017-04-24 23:48:57 PDT
(In reply to Michael Catanzaro from
comment #21
)
> We need to make sure these third-party ICU directories are never added to > the include path for CMake builds.
AFAIU, GTK port does not have any of these directories in include path
Konstantin Tokarev
Comment 23
2017-04-24 23:53:00 PDT
(In reply to John Ralls from
comment #20
)
> I suggest that you should still change the name of UTF8.h or wtf/unicode as > insurance against yet another regression.
You mean, to hide another regression?
Michael Catanzaro
Comment 24
2017-04-25 07:22:53 PDT
(In reply to Konstantin Tokarev from
comment #22
)
> (In reply to Michael Catanzaro from
comment #21
) > > We need to make sure these third-party ICU directories are never added to > > the include path for CMake builds. > > AFAIU, GTK port does not have any of these directories in include path
It shouldn't, but then
comment #20
argues otherwise....
John Ralls
Comment 25
2017-04-25 08:18:19 PDT
> It shouldn't, but then
comment #20
argues otherwise....
Maybe. I should have included that I encountered the error while building an old version of webkit that still used autotools. I haven't yet tried it on master.
Michael Catanzaro
Comment 26
2017-04-25 08:23:49 PDT
OK, we deleted the Autotools build years ago so there's no way your experience there is relevant to current problems. If you have problems with 2.16.1 or master, you can file a new bug. The original problem here is fixed by
bug #171115
.
John Ralls
Comment 27
2017-04-25 08:28:31 PDT
>> I suggest that you should still change the name of UTF8.h or wtf/unicode as >> insurance against yet another regression.
> You mean, to hide another regression?
No, to make a regression impossible. Your patch and its predecessor that changed the intentional include path to wtf/unicode/UTF8.h are what keeps masking the actual bug, which is the duplicate filename and copying of ICU's "unicode" namespace.
Konstantin Tokarev
Comment 28
2017-04-25 08:56:04 PDT
I agree that having includes with "unicode/" prefix is asking for trouble, but if we ensure that <wtf/unicode/...> cannot be used as <unicode/...>, I think that's OK.
John Ralls
Comment 29
2017-04-25 10:05:25 PDT
> I agree that having includes with "unicode/" prefix is asking for trouble, > but if we ensure that <wtf/unicode/...> cannot be used as <unicode/...>, I > think that's OK.
But that's exactly the failure to ensure that that's caused this problem at least twice. Regardless, don't forget to back-port 215614 to 2.16.
Carlos Garcia Campos
Comment 30
2017-05-08 10:04:29 PDT
(In reply to John Ralls from
comment #29
)
> > I agree that having includes with "unicode/" prefix is asking for trouble, > > but if we ensure that <wtf/unicode/...> cannot be used as <unicode/...>, I > > think that's OK. > > But that's exactly the failure to ensure that that's caused this problem at > least twice. > > Regardless, don't forget to back-port 215614 to 2.16.
Please, when you want something to get backported to 2.16 add it to the wiki,
https://trac.webkit.org/wiki/WebKitGTK/2.16.x
It's the only way I won't forget. I'm not even CC-ed to this bug. Anyway, I tried to backport it and it failed to build, I've lost the build error, though.
Carlos Garcia Campos
Comment 31
2017-05-08 10:30:56 PDT
(In reply to Carlos Garcia Campos from
comment #30
)
> (In reply to John Ralls from
comment #29
) > > > I agree that having includes with "unicode/" prefix is asking for trouble, > > > but if we ensure that <wtf/unicode/...> cannot be used as <unicode/...>, I > > > think that's OK. > > > > But that's exactly the failure to ensure that that's caused this problem at > > least twice. > > > > Regardless, don't forget to back-port 215614 to 2.16. > > Please, when you want something to get backported to 2.16 add it to the > wiki,
https://trac.webkit.org/wiki/WebKitGTK/2.16.x
It's the only way I > won't forget. I'm not even CC-ed to this bug. Anyway, I tried to backport it > and it failed to build, I've lost the build error, though.
/home/cgarcia/src/git/gnome/WebKit-2.16/Source/WebCore/platform/graphics/texmap/coordinated/TiledBackingStore.cpp:27:39: fatal error: wtf/MemoryPressureHandler.h: No existe el fichero o el directorio #include <wtf/MemoryPressureHandler.h> ^ compilation terminated. Ok, this is because in 2.16 MemoryPressureHandler.h is still in WebCore/platform, so I'll backport the commit without the TiledBackingStore.cpp change. I hope that's enough to fix the build for you.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug