Bug 196792

Created attachment 367180 [details] Patch Patch to request feedback on and see how it works with AppleWin internal build

Comment on attachment 367180 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367180&action=review I wanted to give an idea on what was going on with these frameworks/${_framework}.cmake files. It seemed like a decent way to go about handling the internal build but I'm open to other thoughts here. Konstantin I'd really like your thoughts on this approach. Alex and Per Arne if this approach works for everyone we should apply this patch and then figure out the correct include directories and libraries for everything. This idea hasn't been applied to the Tools directory since I'm not sure if that's even built with the internal build. > Source/cmake/frameworks/WebCore.cmake:1 > +if (NOT TARGET WebCoreFramework) Since this is included multiple times due to the AppleWin Internal Build the presence of a target is used as an include guard. > Source/cmake/frameworks/WebCore.cmake:2 > + add_library(WebCoreFramework INTERFACE) Then the interface library is created. If we weren't supporting the AppleWin Internal Build I would probably place all this logic into WTF_FRAMEWORK. > Source/cmake/frameworks/WebCore.cmake:12 > + target_include_directories(JavaScriptCoreFramework INTERFACE > + # AppleWin puts all the third party library headers into the same directory > + "${WEBKIT_LIBRARIES_DIR}/include" > + # AppleWin uses include/private to store headers > + "${CMAKE_BINARY_DIR}/../include/private" > + "${CMAKE_BINARY_DIR}/../include/private/JavaScriptCore" > + "${CMAKE_BINARY_DIR}/../include/private/WebCore" > + ) For each library I guessed at what the includes SHOULD be for the internal build based on what is happening in the CMake files in other places. You'll see that the ../include/private stuff is removed in the other CMake files since it appears to be specific to the AppleWin Internal Builds. > Source/cmake/frameworks/WebCore.cmake:13 > + target_link_libraries(WebCoreFramework INTERFACE WebCore${DEBUG_SUFFIX}) WebCore is built statically so there should probably be a lot more libraries defined here. We can audit that for the internal build. > Source/cmake/frameworks/WebCore.cmake:18 > + target_include_directories(WebCoreFramework INTERFACE > + ${WebCore_INCLUDE_DIRECTORIES} > + ${WebCore_PRIVATE_FRAMEWORK_HEADERS_DIR} > + ) For everyone else it adds include directories for the framework headers and propogates includes > Source/cmake/frameworks/WebCore.cmake:21 > + add_dependencies(WebCoreFramework WebCorePrivateFrameworkHeaders) It also makes sure to add the header copying as a dependent target.

Attachment 367180 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/PlatformWin.cmake:450: Alphabetical sorting problem. "PRIVATE WebKitGUID" should be before "PRIVATE Winmm". [list/order] [5] ERROR: Source/WebKitLegacy/CMakeLists.txt:41: Alphabetical sorting problem. "PRIVATE PALFramework" should be before "PRIVATE WebCoreFramework". [list/order] [5] ERROR: Source/cmake/frameworks/ANGLE.cmake:2: One space between command "if" and its parentheses, should be "if (" [whitespace/parentheses] [5] Total errors found: 3 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 367180 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367180&action=review > Source/cmake/frameworks/ANGLE.cmake:6 > + add_library(libANGLE INTERFACE) It should be IMPORTED, not INTERFACE, because it references existing file instead of creating "virtual" target Imported target can have such properties as INTERFACE_INCLUDE_DIRECTORIES, INTERFACE_LINK_LIBRARIES, INTERFACE_COMPILE_DEFINITIONS, etc. See for example https://www.apt-browse.org/browse/ubuntu/xenial/main/amd64/qtbase5-dev/5.5.1+dfsg-16ubuntu7/file/usr/lib/x86_64-linux-gnu/cmake/Qt5Gui/Qt5GuiConfig.cmake where Qt5::Gui target is imported. See also https://gitlab.kitware.com/cmake/community/wikis/doc/tutorials/Exporting-and-Importing-Targets

I like the idea defining interface libraries for Apple internal builds. We should define 'WTF' interface library only for Apple internal builds, instead of 'WTFFramework'. And, we should have a CMake function defining the interface libraries for Apple internal builds. > WEBKIT_DEFINE_FRAMEWORK_INTERFACES(WTF JavaScriptCore) instead of having the following. > include(frameworks/WTF) > include(frameworks/JavaScriptCore)

I think at this stage of refactoring it's too early to eleminate code duplication. We don't see final code for defining targets yet, and making it generic will make things even more complicated to work with.

Created attachment 367222 [details] Patch Fix style and AppleWin hopefully.

(In reply to Konstantin Tokarev from comment #4) > Comment on attachment 367180 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=367180&action=review > > > Source/cmake/frameworks/ANGLE.cmake:6 > > + add_library(libANGLE INTERFACE) > > It should be IMPORTED, not INTERFACE, because it references existing file > instead of creating "virtual" target Currently in WEBKIT_MAKE_FORWARDING_HEADERS there is an explicit add_dependencies call which adds the headers to the framework target. https://trac.webkit.org/browser/webkit/trunk/Source/cmake/WebKitMacros.cmake#L277 In a future patch I want to remove that coupling and treat header copying as a step that happens after the library is built. I'm hopeful this would make the builds proceed a bit faster. I think of the ${_framework}Framework as build the library plus do an install step. I don't know that IMPORTED works for the in source build but it definitely fits the behavior for the Apple internal builds.

IMPORTED should only be used for internal AppleWin builds, other builds should use actual library target (preferred) or INTERFACE (as a temporary refactoring measure)

(In reply to Konstantin Tokarev from comment #9) > IMPORTED should only be used for internal AppleWin builds, other builds > should use actual library target (preferred) or INTERFACE (as a temporary > refactoring measure) Ok this is actually working out well thanks Konstantin! I'm trying to build AppleWin as if I was an internal consumer and I think I need more information on exactly what is going on here. Maybe someone on the Apple side can fill me in. So to just try things out I made WebKitBuild/${_framework}.build directories inside it. I did the sub directory because of the ../include which to me implies that there's a root build directory and then a sub directory. I got JavaScriptCore to build by modifying the WTF.cmake framework target to look like this. if (NOT TARGET WTFFramework) if (WIN32 AND INTERNAL_BUILD) add_library(WTFFramework SHARED IMPORTED) set_property(TARGET WTFFramework PROPERTY IMPORTED_LOCATION ${WEBKIT_LIBRARIES_DIR}/bin32/WTF${DEBUG_SUFFIX}.dll) set_property(TARGET WTFFramework PROPERTY IMPORTED_IMPLIB ${WEBKIT_LIBRARIES_DIR}/lib32/WTF${DEBUG_SUFFIX}.lib) target_include_directories(WTFFramework INTERFACE # AppleWin puts all the third party library headers into the same directory "${WEBKIT_LIBRARIES_DIR}/include" # AppleWin uses include/private to store headers "${CMAKE_BINARY_DIR}/../include/private" ) target_link_libraries(WTFFramework INTERFACE DbgHelp shlwapi winmm ) else () add_library(WTFFramework INTERFACE) target_include_directories(WTFFramework INTERFACE ${WTF_INCLUDE_DIRECTORIES} ${WTF_FRAMEWORK_HEADERS_DIR} ) target_include_directories(WTFFramework SYSTEM INTERFACE ${WTF_SYSTEM_INCLUDE_DIRECTORIES}) target_link_libraries(WTFFramework INTERFACE WTF ${WTF_LIBRARIES}) add_dependencies(WTFFramework WTFFrameworkHeaders) endif () endif () I don't know enough about the AppleWin internal build to know if the IMPORTED_LOCATION stuff is right. Can someone over there clue me in to what things look like for just building WTF and JSC?

Created attachment 367256 [details] Patch Checking the bots and asking if Konstantin is happy with this approach.

Comment on attachment 367256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367256&action=review So I took a stab at what things should probably look like to support the AppleWin internal build. Its my understanding from Lucas that the bots are currently down because there have been problems building it. However I think with a little work that everything can be made to work. I think the future work to get it up and running is * Properly set the locations for the IMPORTED targets * Ensure that the header includes are correct * Is ANGLE needed? * Verify that WebKitQuartzCoreAdditions needs the includes. Seems like it is included from the WebKitLegacy CMake so I'm not sure why its CMakeLists.txt is setup as if it was checked out. * How are tools built? * Can we output headers into the directory that the internal build expects? aka include/private I'm pretty confident things work because I tried to emulate what was happening in the vcproj files. I made a WebKitBuild directory and then made a subdirectory for each target. Then I ran CMake according to the command from the vcproj. I manually staged the artifacts to where the build wanted them and repeated all the way through WebKitLegacy. A WebKitLegacy.cmake is provided just in case there are other things that need it. Since the build is technically broken I would like to just land this assuming it looks good and the bots are green. From there I'm happy to work with someone on Apple to review and figure out how the internal build can live again. Below I did a review to just inform whats going on. > Source/cmake/frameworks/WTF.cmake:3 > + add_library(WTFFramework SHARED IMPORTED) Here we add an IMPORTED target. It needs to know whether the imported target is STATIC or SHARED. This affects the property that needs to be set to point to the lib and optionally the dll. > Source/cmake/frameworks/WTF.cmake:5 > + set_property(TARGET WTFFramework PROPERTY IMPORTED_LOCATION ${WEBKIT_LIBRARIES_DIR}/bin32/WTF${DEBUG_SUFFIX}.dll) > + set_property(TARGET WTFFramework PROPERTY IMPORTED_IMPLIB ${WEBKIT_LIBRARIES_LINK_DIR}/WTF${DEBUG_SUFFIX}.lib) Here we set the location of the libraries. I'm just guessing where these things may be. When I was testing I just moved libs in WebKitLibraries/win/lib32 and dlls into WebKitLibraries/win/bin32. These locations will need to be modified by someone with access to the Apple build. It'll have to take into account the architecture since there's a 64-bit and 32-bit build. > Source/cmake/frameworks/WTF.cmake:11 > + target_include_directories(WTFFramework INTERFACE > + # AppleWin puts all the third party library headers into the same directory > + "${WEBKIT_LIBRARIES_DIR}/include" > + # AppleWin uses include/private to store headers > + "${CMAKE_BINARY_DIR}/../include/private" > + ) Here we add target includes. Some of the other ones have additional header includes. I honestly think that these two are probably all that's actually needed in the other ones. I have a feeling that include private looks like + private + WTF + JavaScriptCore + WebCore + WebKit But I have no way to verify. > Source/cmake/frameworks/WTF.cmake:16 > + target_link_libraries(WTFFramework INTERFACE > + DbgHelp > + shlwapi > + winmm > + ) With these I outputted the values from the target and then modified accordingly.

I'm going to split the AppleWin internal build stuff over to https://bugs.webkit.org/show_bug.cgi?id=196871 and will provide a patch once this bug lands.