Bug 196655 – [CMake][WinCairo] Separate copied headers into different directories

• Status: RESOLVED
• Resolution: FIXED
• Component: CMake

[Don Olmstead]

Currently everything is stored in ${FORWARDING_HEADERS_DIR} which can potentially lead to errors with improper includes.

This brings WinCairo in line with what an Apple build looks like.

[Don Olmstead]

Created attachment 366842 [details]
Patch

[EWS Watchlist]

Attachment 366842 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 366843 [details]
Patch

Fix changelog....

[Michael Catanzaro]

Comment on attachment 366843 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366843&action=review

I'm OK with this, but I'm a little reluctant that none of our CMake experts have reviewed it yet. Please wait one day before landing to give time for others to comment.

> Source/WebCore/CMakeLists.txt:20
> +    "${JavaScriptCore_PRIVATE_FRAMEWORK_HEADERS_DIR}"

Well this is confusing. What is the difference between JSC public and private framework headers, and why is the private include dir accessible outside JSC?

[Konstantin Tokarev]

Comment on attachment 366843 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366843&action=review

> Source/JavaScriptCore/shell/PlatformWin.cmake:1
> +include_directories(./ PRIVATE ${JavaScriptCore_INCLUDE_DIRECTORIES} ${JavaScriptCore_PRIVATE_INCLUDE_DIRECTORIES} ${JavaScriptCore_PRIVATE_FRAMEWORK_HEADERS_DIR})

When we move to target-oriented project, all include_directories() and similar directory-level directives shall be gone

> Source/cmake/OptionsWinCairo.cmake:42
> +set(WTF_FRAMEWORK_HEADERS_DIR ${CMAKE_BINARY_DIR}/FrameworkHeaders/WTF.framework/Headers)

I don't think we should use directory names with ".framework" extension here, because this just leads to confusion and achieves nothing. On macOS and iOS framework directories are created by built-in CMake features, and on other platforms such "framework-like" bundles cannot act in macOS-like way

> Source/cmake/OptionsWinCairo.cmake:44
> +set(JavaScriptCore_PRIVATE_FRAMEWORK_HEADERS_DIR ${CMAKE_BINARY_DIR}/FrameworkHeaders/JavaScriptCore.framework/PrivateHeaders)

Now I see what did you mean with "FRAMEWORK" word in variable name, these headers are logically INTERFACE, but not intended for all users. Are there any targets inside WebKit tree which include only one of these header sets?

[Konstantin Tokarev]

Sorry Michael, I had to drop your flag in order to publish comments :)

[Don Olmstead]

(In reply to Konstantin Tokarev from comment #5)
> > Source/cmake/OptionsWinCairo.cmake:42
> > +set(WTF_FRAMEWORK_HEADERS_DIR ${CMAKE_BINARY_DIR}/FrameworkHeaders/WTF.framework/Headers)
> 
> I don't think we should use directory names with ".framework" extension
> here, because this just leads to confusion and achieves nothing. On macOS
> and iOS framework directories are created by built-in CMake features, and on
> other platforms such "framework-like" bundles cannot act in macOS-like way

Will use ${CMAKE_BINARY_DIR}/${_framework}/Headers and ${CMAKE_BINARY_DIR}/${_framework}/PrivateHeaders in next iteration. Just checking with a local WinCairo build.

> > Source/cmake/OptionsWinCairo.cmake:44
> > +set(JavaScriptCore_PRIVATE_FRAMEWORK_HEADERS_DIR ${CMAKE_BINARY_DIR}/FrameworkHeaders/JavaScriptCore.framework/PrivateHeaders)
> 
> Now I see what did you mean with "FRAMEWORK" word in variable name, these
> headers are logically INTERFACE, but not intended for all users. Are there
> any targets inside WebKit tree which include only one of these header sets?

Not that I've seen.

For context in terms of dependencies for something like JSC the library itself should have a dependency on the public framework headers. It should not have a dependency on the private framework headers.

Currently WEBKIT_MAKE_FORWARDING_HEADERS, which needs a rename, does not abide by that. In a subsequent patch I plan on removing code that adds a dependency on the framework for the headers and instead make it explicit. Then I would be adding an INTERFACE target that would be used as a target for subsequent libraries.

So to illustrate

JavaScriptCore depends on JavaScriptCoreFrameworkHeaders
JavaScriptCoreFramework depends on JavaScriptCoreFrameworkHeaders, JavaScriptCore and JavaScriptCorePrivateFrameworkHeaders
WebCore depends on JavaScriptCoreFramework

[Don Olmstead]

Created attachment 366992 [details]
Patch

Fixing review feedback

[Michael Catanzaro]

(In reply to Don Olmstead from comment #7)
> JavaScriptCore depends on JavaScriptCoreFrameworkHeaders
> JavaScriptCoreFramework depends on JavaScriptCoreFrameworkHeaders,
> JavaScriptCore and JavaScriptCorePrivateFrameworkHeaders
> WebCore depends on JavaScriptCoreFramework

Maybe I'm missing the point, but: why?

[Don Olmstead]

(In reply to Michael Catanzaro from comment #9)
> (In reply to Don Olmstead from comment #7)
> > JavaScriptCore depends on JavaScriptCoreFrameworkHeaders
> > JavaScriptCoreFramework depends on JavaScriptCoreFrameworkHeaders,
> > JavaScriptCore and JavaScriptCorePrivateFrameworkHeaders
> > WebCore depends on JavaScriptCoreFramework
> 
> Maybe I'm missing the point, but: why?

Think of JavaScriptCoreFrameworkHeaders as the public API. It is expected to be stable. Any consumer of JavaScriptCore both inside and outside Apple can use this API to make applications.

Within JavaScriptCore the following are public framework headers

    API/JSBase.h
    API/JSContextRef.h
    API/JSObjectRef.h
    API/JSStringRef.h
    API/JSTypedArray.h
    API/JSValueRef.h
    API/JavaScript.h
    API/WebKitAvailability.h

That's not the totality of the API directory though. There are various private headers in there. For example there's API/JSStringRefPrivate.h. It defines JSStringCreateWithCharactersNoCopy which is used in an API test. It could also be used by Apple internal projects that embed JSC. Its a private header.

Private headers don't provide any API stability and are apparently for Apple use only. I see this https://stackoverflow.com/questions/2678047/what-are-private-frameworks-and-how-will-we-use-them on Stack Overflow. There's probably more nuance there that an Apple WebKit dev could probably explain.

You'll also see that inside of JavaScriptCore/API there are #include <JavaScriptCore/*.h>'s for that list of public framework headers. They also use include guards over pragma once. That's why there needs to be a dependency to copy those headers before building JavaScriptCore.

JavaScriptCore has no dependency on the private framework headers so it should not be dependent on copying the headers. But WebCore is dependent on those private headers so I would like to use an INTERFACE library. INTERFACE libraries aren't actual targets per say. For example I don't see them appear in Visual Studio as something buildable. It can however propagate the dependencies and also allows you to set things like the include directories.

I hope that makes a bit more sense cause all these framework header things take a good amount of cognitive load.

[Michael Catanzaro]

Comment on attachment 366992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366992&action=review

> Tools/MiniBrowser/win/CMakeLists.txt:10
>  set(MiniBrowser_INCLUDE_DIRECTORIES
> -    "${FORWARDING_HEADERS_DIR}"
> -    "${FORWARDING_HEADERS_DIR}/WebKitLegacy"
> -    "${CMAKE_SOURCE_DIR}"
> -    "${CMAKE_SOURCE_DIR}/Source"
> +    ${WTF_FRAMEWORK_HEADERS_DIR}
> +    ${JavaScriptCore_FRAMEWORK_HEADERS_DIR}
> +    ${JavaScriptCore_PRIVATE_FRAMEWORK_HEADERS_DIR}
> +    ${PAL_FRAMEWORK_HEADERS_DIR}
> +    ${WebCore_PRIVATE_FRAMEWORK_HEADERS_DIR}
> +    ${WebKitLegacy_FRAMEWORK_HEADERS_DIR}
> +    ${WebKit_FRAMEWORK_HEADERS_DIR}
> +    ${WebKit_PRIVATE_FRAMEWORK_HEADERS_DIR}
>  )

So why are you not actually using INTERFACE_INCLUDE_DIRECTORIES? Then you wouldn't need this huge list of include directories, right? It would just be propagated upwards.

[Michael Catanzaro]

Or is that future plan?

[Don Olmstead]

(In reply to Michael Catanzaro from comment #12)
> Or is that future plan?

Future plan. However tests are a bit of a pain at the moment. The reason being that the config.h has no concept of what group of tests its building. So technically to make it build you have to have ALL THE HEADERS present.

See https://bugs.webkit.org/show_bug.cgi?id=196583 for that particular case.

I keep thinking of The Simpsons scene where Chief Wiggum is stuck in the hot dog machine and says "Oh boy this is gonna get worse before it gets better" but anyways there's a lot of things to get into shape with the CMake build.

[Don Olmstead]

(In reply to Don Olmstead from comment #13)
> (In reply to Michael Catanzaro from comment #12)
> > Or is that future plan?
> 
> Future plan. However tests are a bit of a pain at the moment. The reason
> being that the config.h has no concept of what group of tests its building.
> So technically to make it build you have to have ALL THE HEADERS present.
> 
> See https://bugs.webkit.org/show_bug.cgi?id=196583 for that particular case.
> 
> I keep thinking of The Simpsons scene where Chief Wiggum is stuck in the hot
> dog machine and says "Oh boy this is gonna get worse before it gets better"
> but anyways there's a lot of things to get into shape with the CMake build.

For context https://i.redd.it/xx36qyal8e721.gif 😺

[Konstantin Tokarev]

This picture certainly deserves to be a cover of book on refactoring legacy code.

[WebKit Commit Bot]

Comment on attachment 366992 [details]
Patch

Clearing flags on attachment: 366992

Committed r244065: <https://trac.webkit.org/changeset/244065>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.