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
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-
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
Correct patch (1.37 KB, patch)
2017-04-20 12:38 PDT, Konstantin Tokarev
no flags
patch (1.97 KB, patch)
2017-04-20 12:46 PDT, Konstantin Tokarev
no flags
Patch (5.84 KB, patch)
2017-04-21 08:30 PDT, Konstantin Tokarev
mcatanzaro: review+
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
Konstantin Tokarev
Comment 16 2017-04-21 08:30:02 PDT
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
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.