Bug 191662 - [CMake][WTF] Mirror XCode header directories
Summary: [CMake][WTF] Mirror XCode header directories
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CMake (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Don Olmstead
URL:
Keywords:
Depends on: 196645
Blocks:
  Show dependency treegraph
 
Reported: 2018-11-14 15:16 PST by Don Olmstead
Modified: 2019-04-10 21:21 PDT (History)
8 users (show)

See Also:


Attachments
Patch (13.17 KB, patch)
2018-11-14 15:28 PST, Don Olmstead
mcatanzaro: review+
Details | Formatted Diff | Diff
Patch (17.35 KB, patch)
2019-04-02 11:49 PDT, Don Olmstead
annulen: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (17.37 KB, patch)
2019-04-03 12:40 PDT, Don Olmstead
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (17.54 KB, patch)
2019-04-03 15:12 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (17.54 KB, patch)
2019-04-03 15:27 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2018-11-14 15:16:53 PST
CMake should emulate the XCode build in terms of where headers go.
Comment 1 Don Olmstead 2018-11-14 15:28:53 PST
Created attachment 354861 [details]
Patch
Comment 2 Michael Catanzaro 2018-11-14 16:08:52 PST
Comment on attachment 354861 [details]
Patch

So why is it that it requires a new target, rather than just adding these to INTERFACE_INCLUDE_DIRECTORIES of the WTF${DEBUG_SUFFIX} target?

(And remind me, why is ${DEBUG_SUFFIX} required again?)
Comment 3 Don Olmstead 2018-11-14 17:17:04 PST
(In reply to Michael Catanzaro from comment #2)
> Comment on attachment 354861 [details]
> Patch
> 
> So why is it that it requires a new target, rather than just adding these to
> INTERFACE_INCLUDE_DIRECTORIES of the WTF${DEBUG_SUFFIX} target?

The first reason to make the dependencies easier.

For example in the next patch JavaScriptCoreFramework would depend on JavaScriptCore, JavaScriptCoreFrameworkHeaders (the public ones), and JavaScriptCorePrivateFrameworkHeaders. The JavaScriptCoreFramework target, which since its an INTERFACE actually is not displayed in a UI like Visual Studio, contains those dependencies and then the includes for the framework headers which propagates it down to the next targets.

The second reason is to potentially optimize the build some more.

Right now WTF depends on copying the headers. This is actually incorrect behavior as WTF doesn't need those to build. Still if you just build WTF its going to copy the headers before building WTF.

Once everything is moved to copies I'll just end up wrapping all this behavior up into WEBKIT_FRAMEWORK since its a lot of the same stuff and breaking the dependencies on framework headers.

I got the idea from WebCore's CMakeLists which has a WebCoreHeaderInterface which is a dependency of WebKit. Its just trying to standardize this behavior.
 
> (And remind me, why is ${DEBUG_SUFFIX} required again?)

I don't honestly think it was ever required. Its not used consistently and its a Windows specific thing.
Comment 4 Michael Catanzaro 2018-11-14 17:27:41 PST
Comment on attachment 354861 [details]
Patch

LGTM.

This is a big change to how forwarding headers work, and I've never seen a CMake patch get worse after review by Konstantin. CC Konstantin.
Comment 5 Fujii Hironori 2018-11-14 18:09:25 PST
(In reply to Don Olmstead from comment #3)
> > (And remind me, why is ${DEBUG_SUFFIX} required again?)
> 
> I don't honestly think it was ever required. Its not used consistently and
> its a Windows specific thing.

${DEBUG_SUFFIX} is needed for the Apple internal build system. See Bug 148083.
Comment 6 Don Olmstead 2018-11-14 18:11:53 PST
(In reply to Fujii Hironori from comment #5)
> (In reply to Don Olmstead from comment #3)
> > > (And remind me, why is ${DEBUG_SUFFIX} required again?)
> > 
> > I don't honestly think it was ever required. Its not used consistently and
> > its a Windows specific thing.
> 
> ${DEBUG_SUFFIX} is needed for the Apple internal build system. See Bug
> 148083.

I'll cc bfulgham and perarne. That's from 3 years ago and its not consistently done across the code base so I'm honestly not sure that its needed.
Comment 7 Don Olmstead 2018-11-14 18:28:37 PST
There's actually CMAKE_DEBUG_POSTFIX https://cmake.org/cmake/help/v3.13/variable/CMAKE_DEBUG_POSTFIX.html which I don't see anywhere in the code and is probably the better way to accomplish this.
Comment 8 Don Olmstead 2018-11-15 14:32:46 PST
Did some investigation in https://bugs.webkit.org/show_bug.cgi?id=191713 and I honestly think this is fine as is.
Comment 9 Don Olmstead 2019-04-02 11:49:54 PDT
Created attachment 366516 [details]
Patch

Trying to take into account what is happening with the AppleWin internal build by adding a cmake file that specifies the WTFFramework.
Comment 10 Konstantin Tokarev 2019-04-03 06:43:52 PDT
I think I like overall direction. While things seem to get more messy, I hope we will be able to factor out idiosyncrasies of internal AppleWin build into files like WTF.cmake eventually
Comment 11 Konstantin Tokarev 2019-04-03 06:50:34 PDT
${DEBUG_SUFFIX} is needed because in internal build WTF, WebCore, and WebKit are built as a separate cmake projects, and e.g. WTF${DEBUG_SUFFIX} in the list of libraries means literal library name, i.e. WTF.lib or WTFd.lib, not "WTF" target. This is different from how CMAKE_DEBUG_POSTFIX works, the latter changes output names of targets, which works fine in target-oriented project but not here
Comment 12 WebKit Commit Bot 2019-04-03 09:16:55 PDT
Comment on attachment 366516 [details]
Patch

Rejecting attachment 366516 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 366516, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebDriver/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: https://webkit-queues.webkit.org/results/11752382
Comment 13 Don Olmstead 2019-04-03 12:40:33 PDT
Created attachment 366632 [details]
Patch

Not sure what happened with the commit queue here.
Comment 14 WebKit Commit Bot 2019-04-03 14:41:07 PDT
Comment on attachment 366632 [details]
Patch

Rejecting attachment 366632 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 366632, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebDriver/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: https://webkit-queues.webkit.org/results/11755693
Comment 15 Don Olmstead 2019-04-03 15:12:00 PDT
Created attachment 366651 [details]
Patch

Fix changelogs.....
Comment 16 Don Olmstead 2019-04-03 15:27:18 PDT
Created attachment 366655 [details]
Patch
Comment 17 WebKit Commit Bot 2019-04-03 16:08:50 PDT
Comment on attachment 366655 [details]
Patch

Clearing flags on attachment: 366655

Committed r243833: <https://trac.webkit.org/changeset/243833>
Comment 18 WebKit Commit Bot 2019-04-03 16:08:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Philippe Normand 2019-04-05 07:40:05 PDT
Why is this enabled for non-apple ports?
Comment 20 Konstantin Tokarev 2019-04-05 07:49:55 PDT
Nothing should change for other ports. This patch creates proxy target for WTF, which acts as one of those indirection levels, which solve every CS problem (except problem of too many indirections)
Comment 21 Philippe Normand 2019-04-05 08:01:31 PDT
Well, I see here some WTF.framework directories in my WPE and GTK build directories. So something is clearly wrong :)
Comment 22 Philippe Normand 2019-04-05 08:02:46 PDT
And it breaks the unified build:

FAILED: Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/unified-sources/UnifiedSource-3a52ce78-21.cpp.o 
/app/bin/c++   -DBUILDING_WITH_CMAKE=1 -DBUILDING_WPE__=1 -DBUILDING_WebCore -DGETTEXT_PACKAGE=\"WPE\" -DHAVE_CONFIG_H=1 -DJSC_GLIB_API_ENABLED -DSTATICALLY_LINKED_WITH_PAL=1 -I. -I../../Source/WebCore -I../../Source/WebCore/Modules/airplay -I../../Source/WebCore/Modules/applepay -I../../Source/WebCore/Modules/applepay/paymentrequest -I../../Source/WebCore/Modules/applicationmanifest -I../../Source/WebCore/Modules/beacon -I../../Source/WebCore/Modules/cache -I../../Source/WebCore/Modules/credentialmanagement -I../../Source/WebCore/Modules/encryptedmedia -I../../Source/WebCore/Modules/encryptedmedia/legacy -I../../Source/WebCore/Modules/entriesapi -I../../Source/WebCore/Modules/fetch -I../../Source/WebCore/Modules/geolocation -I../../Source/WebCore/Modules/indexeddb -I../../Source/WebCore/Modules/indexeddb/client -I../../Source/WebCore/Modules/indexeddb/server -I../../Source/WebCore/Modules/indexeddb/shared -I../../Source/WebCore/Modules/mediacapabilities -I../../Source/WebCore/Modules/mediacontrols -I../../Source/WebCore/Modules/mediarecorder -I../../Source/WebCore/Modules/mediasession -I../../Source/WebCore/Modules/mediasource -I../../Source/WebCore/Modules/mediastream -I../../Source/WebCore/Modules/mediastream/libwebrtc -I../../Source/WebCore/Modules/navigatorcontentutils -I../../Source/WebCore/Modules/notifications -I../../Source/WebCore/Modules/paymentrequest -I../../Source/WebCore/Modules/plugins -I../../Source/WebCore/Modules/quota -I../../Source/WebCore/Modules/speech -I../../Source/WebCore/Modules/streams -I../../Source/WebCore/Modules/webaudio -I../../Source/WebCore/Modules/webauthn -I../../Source/WebCore/Modules/webauthn/cbor -I../../Source/WebCore/Modules/webauthn/fido -I../../Source/WebCore/Modules/webdatabase -I../../Source/WebCore/Modules/webdriver -I../../Source/WebCore/Modules/webgpu -I../../Source/WebCore/Modules/websockets -I../../Source/WebCore/Modules/webvr -I../../Source/WebCore/accessibility -I../../Source/WebCore/accessibility/isolatedtree -I../../Source/WebCore/animation -I../../Source/WebCore/bindings -I../../Source/WebCore/bindings/js -I../../Source/WebCore/bridge -I../../Source/WebCore/bridge/c -I../../Source/WebCore/bridge/jsc -I../../Source/WebCore/contentextensions -I../../Source/WebCore/crypto -I../../Source/WebCore/crypto/algorithms -I../../Source/WebCore/crypto/keys -I../../Source/WebCore/crypto/parameters -I../../Source/WebCore/css -I../../Source/WebCore/css/parser -I../../Source/WebCore/css/typedom -I../../Source/WebCore/cssjit -I../../Source/WebCore/dom -I../../Source/WebCore/dom/messageports -I../../Source/WebCore/domjit -I../../Source/WebCore/editing -I../../Source/WebCore/fileapi -I../../Source/WebCore/history -I../../Source/WebCore/html -I../../Source/WebCore/html/canvas -I../../Source/WebCore/html/forms -I../../Source/WebCore/html/parser -I../../Source/WebCore/html/shadow -I../../Source/WebCore/html/track -I../../Source/WebCore/inspector -I../../Source/WebCore/inspector/agents -I../../Source/WebCore/inspector/agents/page -I../../Source/WebCore/inspector/agents/worker -I../../Source/WebCore/layout -I../../Source/WebCore/layout/blockformatting -I../../Source/WebCore/layout/displaytree -I../../Source/WebCore/layout/floats -I../../Source/WebCore/layout/inlineformatting -I../../Source/WebCore/layout/inlineformatting/text -I../../Source/WebCore/layout/layouttree -I../../Source/WebCore/loader -I../../Source/WebCore/loader/appcache -I../../Source/WebCore/loader/archive -I../../Source/WebCore/loader/archive/mhtml -I../../Source/WebCore/loader/cache -I../../Source/WebCore/loader/icon -I../../Source/WebCore/mathml -I../../Source/WebCore/page -I../../Source/WebCore/page/animation -I../../Source/WebCore/page/csp -I../../Source/WebCore/page/scrolling -I../../Source/WebCore/platform -I../../Source/WebCore/platform/animation -I../../Source/WebCore/platform/audio -I../../Source/WebCore/platform/encryptedmedia -I../../Source/WebCore/platform/gamepad -I../../Source/WebCore/platform/graphics -I../../Source/WebCore/platform/graphics/cpu/arm -I../../Source/WebCore/platform/graphics/cpu/arm/filters -I../../Source/WebCore/platform/graphics/displaylists -I../../Source/WebCore/platform/graphics/filters -I../../Source/WebCore/platform/graphics/iso -I../../Source/WebCore/platform/graphics/opentype -I../../Source/WebCore/platform/graphics/transforms -I../../Source/WebCore/platform/mediacapabilities -I../../Source/WebCore/platform/mediarecorder -I../../Source/WebCore/platform/mediasession -I../../Source/WebCore/platform/mediastream -I../../Source/WebCore/platform/mediastream/libwebrtc -I../../Source/WebCore/platform/mock -I../../Source/WebCore/platform/mock/mediasource -I../../Source/WebCore/platform/network -I../../Source/WebCore/platform/sql -I../../Source/WebCore/platform/text -I../../Source/WebCore/platform/vr -I../../Source/WebCore/plugins -I../../Source/WebCore/rendering -I../../Source/WebCore/rendering/line -I../../Source/WebCore/rendering/mathml -I../../Source/WebCore/rendering/shapes -I../../Source/WebCore/rendering/style -I../../Source/WebCore/rendering/svg -I../../Source/WebCore/rendering/updating -I../../Source/WebCore/replay -I../../Source/WebCore/storage -I../../Source/WebCore/style -I../../Source/WebCore/svg -I../../Source/WebCore/svg/animation -I../../Source/WebCore/svg/graphics -I../../Source/WebCore/svg/graphics/filters -I../../Source/WebCore/svg/properties -I../../Source/WebCore/websockets -I../../Source/WebCore/workers -I../../Source/WebCore/workers/service -I../../Source/WebCore/workers/service/context -I../../Source/WebCore/workers/service/server -I../../Source/WebCore/worklets -I../../Source/WebCore/xml -I../../Source/WebCore/xml/parser -IDerivedSources/WebCore -IDerivedSources/ForwardingHeaders/ANGLE -I../../Source/WebCore/platform/graphics/gpu -I../../Source/ThirdParty/xdgmime/src -I../../Source/WebCore/platform/graphics/cairo -I../../Source/WebCore/platform/graphics/freetype -I../../Source/WebCore/platform/graphics/harfbuzz -I../../Source/WebCore/platform/graphics/harfbuzz/ng -I../../Source/WebCore/platform/graphics/gstreamer -I../../Source/WebCore/platform/graphics/gstreamer/mse -I../../Source/WebCore/platform/graphics/gstreamer/eme -I../../Source/WebCore/platform/audio/gstreamer -I../../Source/WebCore/platform/encryptedmedia/clearkey -I../../Source/WebCore/platform/image-decoders -I../../Source/WebCore/platform/image-decoders/bmp -I../../Source/WebCore/platform/image-decoders/gif -I../../Source/WebCore/platform/image-decoders/ico -I../../Source/WebCore/platform/image-decoders/jpeg -I../../Source/WebCore/platform/image-decoders/jpeg2000 -I../../Source/WebCore/platform/image-decoders/png -I../../Source/WebCore/platform/image-decoders/webp -I../../Source/WebCore/platform/graphics/texmap -I../../Source/WebCore/page/scrolling/nicosia -I../../Source/WebCore/platform/graphics/texmap/coordinated -I../../Source/WebCore/platform/graphics/nicosia -I../../Source/WebCore/platform/graphics/nicosia/cairo -I../../Source/WebCore/platform/graphics/nicosia/texmap -I../../Source/ThirdParty/ANGLE -I../../Source/ThirdParty/ANGLE/include/KHR -I../../Source/WebCore/platform/graphics/egl -I../../Source/WebCore/platform/graphics/epoxy -I../../Source/WebCore/platform/graphics/glx -I../../Source/WebCore/platform/graphics/opengl -I../../Source/WebCore/platform/graphics/libwpe -I../../Source/WebCore/platform/graphics/wayland -I../../Source/WebCore/platform/mediastream/gstreamer -I../../Source/WebCore/platform/network/soup -I../../Source/WebCore/platform/text/icu -isystem /usr/include/libxml2 -isystem ../../Source/ThirdParty/libwebrtc/Source -isystem ../../Source/ThirdParty/libwebrtc/Source/webrtc -isystem ../../Source/ThirdParty/libwebrtc/Source/third_party/abseil-cpp -isystem /usr/include/cairo -isystem /usr/include/freetype2 -isystem /usr/include/harfbuzz -isystem /app/include/gstreamer-1.0 -isystem /usr/include/glib-2.0 -isystem /usr/lib/glib-2.0/include -isystem /usr/include/orc-0.4 -isystem /app/lib/gstreamer-1.0/include -isystem /app/include/openjpeg-2.3 -isystem /usr/include/gio-unix-2.0 -isystem /usr/include/libsoup-2.4 -isystem /app/include/wpe-1.0 -IDerivedSources/ForwardingHeaders -IFrameworkHeaders/WTF.framework/Headers -I../../Source/bmalloc -IDerivedSources -I../../Source/ThirdParty -fdiagnostics-color=always -Wextra -Wall -Wno-attributes -Wno-psabi -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align  -fno-strict-aliasing -fno-exceptions -fno-rtti -std=c++14 -O3 -DNDEBUG -fPIC -MD -MT Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/unified-sources/UnifiedSource-3a52ce78-21.cpp.o -MF Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/unified-sources/UnifiedSource-3a52ce78-21.cpp.o.d -o Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/unified-sources/UnifiedSource-3a52ce78-21.cpp.o -c /app/webkit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource-3a52ce78-21.cpp
In file included from ../../Source/WebCore/html/HTMLFormElement.h:32:0,
                 from ../../Source/WebCore/html/HTMLFormControlsCollection.h:26,
                 from DerivedSources/WebCore/JSHTMLFormControlsCollection.h:23,
                 from DerivedSources/WebCore/JSDOMWindow.cpp:175,
                 from /app/webkit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource-3a52ce78-21.cpp:5:
FrameworkHeaders/WTF.framework/Headers/wtf/WeakHashSet.h:151:12: error: ‘WeakHashSet’ is already declared in this scope
 using WTF::WeakHashSet;
            ^~~~~~~~~~~

Because the compiler includes the header from 2 different locations now.
Comment 23 Konstantin Tokarev 2019-04-05 08:11:53 PDT
This indeed is wrong, I overlooked such detail when reviewing. Directory names like "WTF.framework" should only be used for macOS frameworks and for nothing else.

However, I guess you should be able to proceed if you clear build directory (but this is unchecked)
Comment 24 Philippe Normand 2019-04-05 08:14:09 PDT
This is a clean build.
Comment 25 Konstantin Tokarev 2019-04-05 08:15:07 PDT
OK, let's revert it then
Comment 26 WebKit Commit Bot 2019-04-05 08:18:33 PDT
Re-opened since this is blocked by bug 196645
Comment 27 Philippe Normand 2019-04-05 10:27:41 PDT
(In reply to Konstantin Tokarev from comment #23)
> This indeed is wrong, I overlooked such detail when reviewing. Directory
> names like "WTF.framework" should only be used for macOS frameworks and for
> nothing else.
> 

I agree.
However this specific patch isn't the culprit of the breakage because even after rollout we still have ForwardingHeaders/ getting in the way of the unified build.