Bug 72816

Created attachment 115966 [details] WIP: Basic CMake based build system for Windows

Comment on attachment 115966 [details] WIP: Basic CMake based build system for Windows Attachment 115966 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10462183

Comment on attachment 115966 [details] WIP: Basic CMake based build system for Windows View in context: https://bugs.webkit.org/attachment.cgi?id=115966&action=review > Source/CMakeLists.txt:145 > -IF (ENABLE_WEBKIT) > +#IF (ENABLE_WEBKIT) > ADD_SUBDIRECTORY(WebKit) > -ENDIF () > +#ENDIF () > > -IF (ENABLE_WEBKIT2) > +#IF (ENABLE_WEBKIT2) > ADD_SUBDIRECTORY(WebKit2) > -ENDIF () > +#ENDIF () Does Wk2 depend on Wk1? AFAIK Samsung has not committed the whole webkit2-efl port source code, which makes the efl bot is breaking with this patch. > Source/JavaScriptCore/CMakeLists.txt:282 > +SET_TARGET_PROPERTIES(${JavaScriptCore_LIBRARY_NAME} PROPERTIES LINK_INTERFACE_LIBRARIES ${JavaScriptCore_LIBRARY_NAME}) Can you clarify what problem is caused by the default settings? > Source/JavaScriptCore/PlatformWinApple.cmake:18 > + > + > + > + Style-wise, I think these multiple empty lines could be trimmed down. > Source/JavaScriptCore/PlatformWinApple.cmake:65 > +WEBKIT_CREATE_FORWARDING_HEADERS(JavaScriptCore DIRECTORIES ${JavaScriptCore_FORWARDING_HEADERS_DIRECTORIES} FILES ${JavaScriptCore_FORWARDING_HEADERS_FILES}) Don't the headers in the tree work? > Source/WebKit2/CMakeLists.txt:-13 > - "${WEBKIT2_DIR}/Shared/efl" Where has this been moved to? > Source/WebKit2/CMakeLists.txt:-22 > - "${WEBKIT2_DIR}/UIProcess/efl" Ditto. > Source/WebKit2/CMakeLists.txt:472 > -ADD_DEPENDENCIES(${WebKit2_LIBRARY_NAME} ${ForwardingHeaders_NAME}) > -ADD_DEPENDENCIES(${WebKit2_LIBRARY_NAME} ${ForwardingNetworkHeaders_NAME}) > +#ADD_DEPENDENCIES(${WebKit2_LIBRARY_NAME} ${ForwardingHeaders_NAME}) > +#ADD_DEPENDENCIES(${WebKit2_LIBRARY_NAME} ${ForwardingNetworkHeaders_NAME}) > TARGET_LINK_LIBRARIES(${WebKit2_LIBRARY_NAME} ${WebKit2_LIBRARIES}) > > -ADD_EXECUTABLE(${WebProcess_NAME} ${WebProcess_SOURCES}) > -TARGET_LINK_LIBRARIES(${WebProcess_NAME} ${WebProcess_LIBRARIES}) > +#ADD_EXECUTABLE(${WebProcess_NAME} ${WebProcess_SOURCES}) > +#TARGET_LINK_LIBRARIES(${WebProcess_NAME} ${WebProcess_LIBRARIES}) > > +SET_TARGET_PROPERTIES(${WebKit2_LIBRARY_NAME} PROPERTIES LINK_INTERFACE_LIBRARIES ${WebKit2_LIBRARY_NAME}) > SET_TARGET_PROPERTIES(${WebKit2_LIBRARY_NAME} PROPERTIES VERSION ${PROJECT_VERSION} SOVERSION ${PROJECT_VERSION_MAJOR}) > IF (WebKit2_LINK_FLAGS) > ADD_TARGET_PROPERTIES(${WebKit2_LIBRARY_NAME} LINK_FLAGS "${WebKit2_LINK_FLAGS}") > - ADD_TARGET_PROPERTIES(${WebProcess_NAME} LINK_FLAGS "${WebKit2_LINK_FLAGS}") > +# ADD_TARGET_PROPERTIES(${WebProcess_NAME} LINK_FLAGS "${WebKit2_LINK_FLAGS}") Can you clarify why these lines were commented out? > Source/WebKit2/config.h:58 > -#if defined (BUILDING_WITH_CMAKE) > +#if defined (BUILDING_WITH_CMAKE_______) Isn't this always going to be false? > Source/cmake/FindDirectX.cmake:7 > +# DirectX_FOUND > +# DirectX_INCLUDE_DIR > +# DirectX_LIBRARY > +# DirectX_ROOT_DIR I wonder if it doesn't make sense to follow CMake's variable naming recommendations for FindXXX.cmake files and use _INCLUDE_DIRS and _LIBRARIES here. > Source/cmake/FindDirectX.cmake:46 > + # We start with Microsoft DirectX SDK (August 2008) 9.24.1400 > + # Example of path is C:\apps_x86\Microsoft DirectX SDK (August 2008)\Include > + find_path(DirectX_INCLUDE_DIR d3d9.h > + "$ENV{DXSDK_DIR}/Include" > + "C:/apps_x86/Microsoft DirectX SDK*/Include" > + "C:/Program Files (x86)/Microsoft DirectX SDK*/Include" > + "C:/apps/Microsoft DirectX SDK*/Include" > + "C:/Program Files/Microsoft DirectX SDK*/Include" > + ) > + mark_as_advanced(DirectX_INCLUDE_DIR) > + if(DirectX_INCLUDE_DIR) > + set(DirectX_ROOT_DIR "${DirectX_INCLUDE_DIR}/..") > + endif(DirectX_INCLUDE_DIR) > + > + # dlls are in DirectX_ROOT_DIR/Developer Runtime/x64|x86 > + # lib files are in DirectX_ROOT_DIR/Lib/x64|x86 > + > + set(DirectX_LIBRARY_PATHS) > + if(CMAKE_CL_64) > + set(DirectX_LIBRARY_PATHS > + "${DirectX_ROOT_DIR}/Lib/x64") > + else(CMAKE_CL_64) > + set(DirectX_LIBRARY_PATHS > + "${DirectX_ROOT_DIR}/Lib/x86" "${DirectX_ROOT_DIR}/Lib") > + endif(CMAKE_CL_64) > + > + find_library(DirectX_LIBRARY d3d9 ${DirectX_LIBRARY_PATHS} NO_DEFAULT_PATH) > + if(DirectX_INCLUDE_DIR AND DirectX_LIBRARY) > + set(DirectX_FOUND true) > + > + find_library(_d3dx9 d3dx9 ${DirectX_LIBRARY_PATHS} NO_DEFAULT_PATH) > + set(DirectX_LIBRARY ${DirectX_LIBRARY} ${_d3dx9}) > + endif() > + mark_as_advanced(DirectX_LIBRARY) I've never used CMake on Windows, so I may be wrong here. It looks like it would be simpler to have something like: FIND_PATH(DirectX_INCLUDE_DIRS d3d9.h PATHS ...) SET(DirectX_ROOT_DIR "${DirectX_INCLUDE_DIRS}/..") IF (CMAKE_CL_64) SET(DirectX_LIBRARY_PATHS ...) ELSE () SET(DirectX_LIBRARY_PATHS ...) ENDIF () FIND_LIBRARY(D3D9_LIBRARY d3d9 PATHS ${DirectX_LIBRARY_PATHS}) FIND_LIBRARY(D3DX9_LIBRARY d3dx9 PATHS ${DirectX_LIBRARY_PATHS}) SET(DirectX_LIBRARIES "${D3D9_LIBRARY}" "${D3DX9_LIBRARY}") INCLUDE(FindPackageHandleStandardArgs) FIND_PACKAGE_HANDLE_STANDARD_ARGS(DirectX DEFAULT_MSG DirectX_LIBRARIES DirectX_INCLUDE_DIRS) MARK_AS_ADVANCED(DirectX_INCLUDE_DIRS DirectX_LIBRARIES DirectX_ROOT_DIR) > Source/cmake/FindDirectX.cmake:51 > + #message("DirectX_FOUND=${DirectX_FOUND}") > + #message("DirectX_INCLUDE_DIR=${DirectX_INCLUDE_DIR}") > + #message("DirectX_LIBRARY=${DirectX_LIBRARY}") > + #message("DirectX_ROOT_DIR=${DirectX_ROOT_DIR}") Unneeded? > Source/cmake/FindQuickTimeSDK.cmake:1 > +set(QuickTimeSDK_FOUND 0) My comments also apply here.

Comment on attachment 115966 [details] WIP: Basic CMake based build system for Windows View in context: https://bugs.webkit.org/attachment.cgi?id=115966&action=review The patch is far away from getting landed. ;-) As the title of the patch says: It's work in progress. It' manly a simple code drop to show what's required for the windows port. >> Source/CMakeLists.txt:145 >> +#ENDIF () > > Does Wk2 depend on Wk1? AFAIK Samsung has not committed the whole webkit2-efl port source code, which makes the efl bot is breaking with this patch. Had some troubles to get it working, without this, but i didn't dig deeper. >> Source/JavaScriptCore/CMakeLists.txt:282 >> +SET_TARGET_PROPERTIES(${JavaScriptCore_LIBRARY_NAME} PROPERTIES LINK_INTERFACE_LIBRARIES ${JavaScriptCore_LIBRARY_NAME}) > > Can you clarify what problem is caused by the default settings? QTMoviewWin needs an other CoreFoundation library as the one of the WTF library, so you get big linker troubles. :-( >> Source/JavaScriptCore/PlatformWinApple.cmake:18 >> + > > Style-wise, I think these multiple empty lines could be trimmed down. Of course. :-) >> Source/JavaScriptCore/PlatformWinApple.cmake:65 >> +WEBKIT_CREATE_FORWARDING_HEADERS(JavaScriptCore DIRECTORIES ${JavaScriptCore_FORWARDING_HEADERS_DIRECTORIES} FILES ${JavaScriptCore_FORWARDING_HEADERS_FILES}) > > Don't the headers in the tree work? No, WindowsApple for needs the ForwardingHeaders since the #include statements contain the "LibraryName". >> Source/WebKit2/CMakeLists.txt:-13 >> - "${WEBKIT2_DIR}/Shared/efl" > > Where has this been moved to? There is no PlatformEfl.cmake in the WebKit2 dir! >> Source/WebKit2/CMakeLists.txt:472 >> +# ADD_TARGET_PROPERTIES(${WebProcess_NAME} LINK_FLAGS "${WebKit2_LINK_FLAGS}") > > Can you clarify why these lines were commented out? Some problems to get it working, many because i didn't finished this stuff. >> Source/WebKit2/config.h:58 >> +#if defined (BUILDING_WITH_CMAKE_______) > > Isn't this always going to be false? The preprocessor statements are wrong otherwise... Needs more work to clairify. >> Source/cmake/FindDirectX.cmake:7 >> +# DirectX_ROOT_DIR > > I wonder if it doesn't make sense to follow CMake's variable naming recommendations for FindXXX.cmake files and use _INCLUDE_DIRS and _LIBRARIES here. I only took the first FindDirectX.cmake I found in the net... I need to write my own anyway.

Created attachment 116431 [details] WIP: Basic CMake based build system for Windows * rebased to recent CMake changes * added DRT and ImageDiff targets

Comment on attachment 116431 [details] WIP: Basic CMake based build system for Windows Attachment 116431 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10640074

Comment on attachment 116431 [details] WIP: Basic CMake based build system for Windows View in context: https://bugs.webkit.org/attachment.cgi?id=116431&action=review > Source/JavaScriptCore/PlatformWinApple.cmake:18 > + > + > + > + Nit: One empty line line seems sufficient to visually separate these blocks of code. Having four seems extraneous. > Source/WebCore/PlatformWinApple.cmake:247 > + > + Nit: One empty line line seems sufficient to visually separate these blocks of code. Having two seems extraneous. > Source/WebCore/PlatformWinApple.cmake:261 > + > + > + Nit: One empty line line seems sufficient to visually separate these blocks of code. Having three seems extraneous. > Source/WebCore/PlatformWinApple.cmake:324 > + > + Nit: One empty line line seems sufficient to visually separate these blocks of code. Having two seems extraneous. > Source/WebCore/qtmoviewin/CMakeLists.txt:35 > + > + Nit: One empty line line seems sufficient to visually separate these blocks of code. Having two seems extraneous. > Source/WebKit/win/CMakeListsWinApple.txt:111 > + > + Nit: One empty line line seems sufficient to visually separate these blocks of code. Having two seems extraneous. > Source/WebKit/win/CMakeListsWinApple.txt:120 > + > + Nit: One empty line line seems sufficient to visually separate these blocks of code. Having two seems extraneous. > Source/cmake/FindDirectX.cmake:7 > +# ----------------------------------------------------------------------------- > +# Find DirectX SDK > +# Define: > +# DirectX_FOUND > +# DirectX_INCLUDE_DIR > +# DirectX_LIBRARY > +# DirectX_ROOT_DIR The drastic change in style used in this file compared to the rest of this patch leads me to believe that this file was acquired. We need copyright and license information about this file to both give credit to the author and to determine whether this file can be landed given its license. From talking with Patrick on IRC today (12/1), he mentioned that he found this file using Google. > Source/cmake/FindDirectX.cmake:51 > + #message("DirectX_FOUND=${DirectX_FOUND}") > + #message("DirectX_INCLUDE_DIR=${DirectX_INCLUDE_DIR}") > + #message("DirectX_LIBRARY=${DirectX_LIBRARY}") > + #message("DirectX_ROOT_DIR=${DirectX_ROOT_DIR}") Nit: We shouldn't commit commented out directives. > Source/cmake/FindQuickTimeSDK.cmake:1 > +set(QuickTimeSDK_FOUND 0) The drastic change in style used in this file compared to the rest of this patch leads me to believe that this file was acquired. We need copyright and license information about this file to both give credit to the author and to determine whether this file can be landed given its license.

Created attachment 129403 [details] WIP: Basic CMake based build system for Windows (based on r109089)

Created attachment 130301 [details] WIP: Basic CMake based build system for Windows (based on r109787)

Created attachment 136668 [details] WIP: Basic CMake based build system for Windows (based on r113844)

Created attachment 155222 [details] WIP: Basic CMake based build system for Windows (based on r123985)

Created attachment 196804 [details] WIP: Basic CMake based build system for Windows (based on r145827)

I also think that using CMake to build for Windows would greatly simplify life for anyone who works on Windows ports. A bit of history - we (I'm at HP - working on LaserJet & digital sending devices with large control panels) took a drop of WebKit back in '09 and got it working (with Cairo) on Windows CE and XP with the goal of having the code be as identical as possible between CE & XP. Last year I started an effort to rebase. I saw that there was a Windows CE port of webkit checked in, and that it used CMake. I adopted CMake as I saw that it would eliminate all the headaches we ran into with Visual Studio .vcproj files, property files, environment variables for the build, etc. It's hard to underscore how much nicer it has been working with CMake than the standard windows build for webkit... I ported the wincairo build to CMake (this was possible thanks to the work that has already been done for WTF, JavaScriptCore, and WebCore by the existing ports that use CMake - WinCE, BlackBerry, and EFL). I'm rebasing my wincairo cmake build now, as I think it is useful as another example of how to get CMake working for the windows build. Because we have a goal of having as few differences as possible between XP & CE, I took a different strategy from what Patrick has done. Patrick's WinCE build of webkit has a minimalist WebKit layer. We need a more full featured WinCE build, with inspector support, the COM API, etc. In Patrick's patches, he adds a new PlatformWinApple CMake port. My strategy is creating a PlatformWin CMake port which can be used to build for XP/CE, Cairo or CF, etc. I'm going to take a closer look at Patrick's patch and take pieces from it that improve upon what I've done. If there is no interest in my work, I suppose I'll continue anyway and get my rebase pushed up to my github fork. I'd like to contribute to WebKit though and make life easier for those who want to build for Windows (& WinCE).

Created attachment 197702 [details] CMake build system for windows ports What do people think about having a single Windows CMake platform? I think life would be easier if we had a single PlatformWin. I've created a patch that does that. Patrick has created PlatformWinCE, here has a patch here that creates PlatformWinApple, and when support for Cairo is added PlatformWinCairo will be created too. I see lots of duplication here that can be easily avoided. Disclaimer: The patch is a couple of months out of date. I'll try to rebase it tomorrow.

Comment on attachment 197702 [details] CMake build system for windows ports First:THX for the patch. You mix changes in the cpp files with adding CMake files. Please make independent changes. As already said at webit-dev, I'd like to add separate WinCG and WinCairo platform files in a first step and then split out the common parts. So a PlatformWinCairo does a include(PlatformWin) as OptionsWinCE does already today in the WinCE port. This avoids the different "if (WTF_USE_...) " in the CMake files. The reason for this is, that it sould be possible to do a -DPORT=WinCG or PORT=WinCairo when running CMake to decide the port. Setting all WTF_* stuff is to complicated (for now).

(In reply to comment #15) > (From update of attachment 197702 [details]) > First:THX for the patch. Thank for you reviewing my patch. > You mix changes in the cpp files with adding CMake files. Please make independent changes. Good catch - I changed a couple of header files for work to come later (like changing Platform.h to allow using WCHAR_UNICODE on XP). I changed DOMPath.h to allow building without SVG. I can back out these for now. > As already said at webit-dev, I'd like to add separate WinCG and WinCairo platform files in a first step and then split out the common parts. So a PlatformWinCairo does a include(PlatformWin) as OptionsWinCE does already today in the WinCE port. OK, but might it be easier to start with a common file? Starting with PlatformAppleWin now makes it more work to slice it up (and slice the common pieces out) later. > This avoids the different "if (WTF_USE_...) " in the CMake files. The reason for this is, that it sould be possible to do a -DPORT=WinCG or PORT=WinCairo when running CMake to decide the port. Setting all WTF_* stuff is to complicated (for now). I think having the switches in the file: if (WTF_PLATFORM_WIN_CAIRO) if (WTF_USE_CF) if (WTF_USE_CF_NETWORK) elseif (WTF_USE_CURL) if (ENABLE_DRAG_SUPPORT) if (ENABLE_INSPECTOR) etc. does not preclude from using -DPORT=WinCG or -DPORT=WinCairo as shortcuts that would set WTF_USE_CF, WTF_USE_CF_NETWORK, etc. to the correct initial values. I think this actually make the build more readable (and therefore less complex). These switches then decide which files are compiled in the build.

The larger question I was looking for feedback on - is there any interest/preference in the strategy I'm proposing for a windows cmake build? Or is there a preference to maintain separate platform files (at least initially)?

Created attachment 198544 [details] WinCairo cmake build - based off 148442 (4/15) Some notes about this patch. To build, I create the directory I want to build in, then, from that directory: cmake -G "Visual Studio 8 2005" -DPORT=Win -DPORT_FLAVOR=WinCairo D:\git\webkit-github Or, if I want to build with VS 2008: cmake -G "Visual Studio 9 2008" -DPORT=Win -DPORT_FLAVOR=WinCairo D:\git\webkit-github I haven't tried 2010/2012 yet, but they should work too. I launch visual studio (devenv WebKit.sln) and build. I introduced Win port specific files to specify options. Source\CMake\OptionsWin.cmake includes Source\CMake\OptionsWinCairo.cmake (specified via PORT_FLAVOR). OptionsWinApple.cmake would be similar except it would specify using CG, it CF_NETWORK, etc. OptionsWinCairo.cmake looks like this: +WEBKIT_OPTION_BEGIN() +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ACCESSIBILITY ON) +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CSS_FILTERS ON) +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_DATALIST_ELEMENT OFF) +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_DRAG_SUPPORT ON) +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FILTERS ON) +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_GEOLOCATION ON) +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_HIGH_DPI_CANVAS ON) +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_METER_ELEMENT OFF) +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_SHARED_WORKERS ON) +WEBKIT_OPTION_END() + +set(WTF_USE_ICU_UNICODE 1) +set(WTF_USE_CF 1) +set(WTF_PLATFORM_WIN_CAIRO 1) +set(WTF_USE_CURL 1) My build does not copy header files, but it does use the generate-forwarding-headers.pl script to generate them. Because I don't copy header files, I did change one header file - Source/WebCore/config.h. WebCoreHeaderDetection.h header file seems to be created by copying header files, therefore it does not exist with my CMake build. (I checked with the Apple WinCairo build and this header file is empty anyway, so maybe it can be removed from config.h?) Change to Source/WebCore/config.h: -#if PLATFORM(WIN) && !OS(WINCE) +#if PLATFORM(WIN) && !defined(BUILDING_WITH_CMAKE) #include <WebCore/WebCoreHeaderDetection.h> #endif I have not set up the appropriate code to generate autoversion.h yet.

I filed a new, related bug: https://bugs.webkit.org/show_bug.cgi?id=115944 (Add CMake base build system for WinCairo port)

This work was brought into the overall CMake effort. We now build Windows with CMake.