Bug 196655 - [CMake][WinCairo] Separate copied headers into different directories
Summary: [CMake][WinCairo] Separate copied headers into different directories
Status: RESOLVED FIXED
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:
Blocks: 196704
  Show dependency treegraph
 
Reported: 2019-04-05 14:14 PDT by Don Olmstead
Modified: 2019-04-08 18:38 PDT (History)
5 users (show)

See Also:


Attachments
Patch (18.43 KB, patch)
2019-04-05 14:38 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (18.40 KB, patch)
2019-04-05 14:44 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (18.17 KB, patch)
2019-04-08 16:02 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 2019-04-05 14:14:05 PDT
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.
Comment 1 Don Olmstead 2019-04-05 14:38:09 PDT Comment hidden (obsolete)
Comment 2 Build Bot 2019-04-05 14:41:36 PDT Comment hidden (obsolete)
Comment 3 Don Olmstead 2019-04-05 14:44:03 PDT Comment hidden (obsolete)
Comment 4 Michael Catanzaro 2019-04-08 15:05:20 PDT
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?
Comment 5 Konstantin Tokarev 2019-04-08 15:09:11 PDT
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?
Comment 6 Konstantin Tokarev 2019-04-08 15:09:45 PDT
Sorry Michael, I had to drop your flag in order to publish comments :)
Comment 7 Don Olmstead 2019-04-08 15:57:55 PDT
(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
Comment 8 Don Olmstead 2019-04-08 16:02:04 PDT
Created attachment 366992 [details]
Patch

Fixing review feedback
Comment 9 Michael Catanzaro 2019-04-08 16:07:04 PDT
(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?
Comment 10 Don Olmstead 2019-04-08 16:33:38 PDT
(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.
Comment 11 Michael Catanzaro 2019-04-08 17:02:20 PDT
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.
Comment 12 Michael Catanzaro 2019-04-08 17:02:42 PDT
Or is that future plan?
Comment 13 Don Olmstead 2019-04-08 17:07:05 PDT
(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.
Comment 14 Don Olmstead 2019-04-08 17:21:29 PDT
(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 😺
Comment 15 Konstantin Tokarev 2019-04-08 18:03:26 PDT
This picture certainly deserves to be a cover of book on refactoring legacy code.
Comment 16 WebKit Commit Bot 2019-04-08 18:38:56 PDT
Comment on attachment 366992 [details]
Patch

Clearing flags on attachment: 366992

Committed r244065: <https://trac.webkit.org/changeset/244065>
Comment 17 WebKit Commit Bot 2019-04-08 18:38:58 PDT
All reviewed patches have been landed.  Closing bug.