Bug 191662 – [CMake][WTF] Mirror XCode header directories

• Component: CMake
• Status: REOPENED

[Don Olmstead]

CMake should emulate the XCode build in terms of where headers go.

[Don Olmstead]

Created attachment 354861 [details]
Patch

[Michael Catanzaro]

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?)

[Don Olmstead]

(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.

[Michael Catanzaro]

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.

[Fujii Hironori]

(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.

[Don Olmstead]

(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.

[Don Olmstead]

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.

[Don Olmstead]

Did some investigation in https://bugs.webkit.org/show_bug.cgi?id=191713 and I honestly think this is fine as is.

[Don Olmstead]

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.

[Konstantin Tokarev]

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

[Konstantin Tokarev]

${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

[WebKit Commit Bot]

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

[Don Olmstead]

Created attachment 366632 [details]
Patch

Not sure what happened with the commit queue here.

[WebKit Commit Bot]

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

[Don Olmstead]

Created attachment 366651 [details]
Patch

Fix changelogs.....

[Don Olmstead]

Created attachment 366655 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 366655 [details]
Patch

Clearing flags on attachment: 366655

Committed r243833: <https://trac.webkit.org/changeset/243833>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

Why is this enabled for non-apple ports?

[Konstantin Tokarev]

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)

[Philippe Normand]

Well, I see here some WTF.framework directories in my WPE and GTK build directories. So something is clearly wrong :)

[Philippe Normand]

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.

[Konstantin Tokarev]

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)

[Philippe Normand]

This is a clean build.

[Konstantin Tokarev]

OK, let's revert it then

[WebKit Commit Bot]

Re-opened since this is blocked by bug 196645

[Philippe Normand]

(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.