Bug 115944

Summary: Add CMake base build system for WinCairo port
Product: WebKit Reporter: Mark Salisbury <mark.salisbury>
Component: Tools / TestsAssignee: Patrick R. Gansterer <paroga>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, cdumez, commit-queue, gyuyoung.kim, laszlo.gombos, ossy, paroga, rakuco
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows 7   
Bug Depends on: 119514    
Bug Blocks:    
Attachments:
Description Flags
Proposed fix
paroga: review-
Proposed fix -- rebased off 150501
none
Patch
paroga: commit-queue-
Patch (based on r156181)
none
Patch
none
Patch cdumez: review+

Description Mark Salisbury 2013-05-10 16:21:52 PDT
This is similar to https://bugs.webkit.org/show_bug.cgi?id=72816, except the focus is to build WinCairo using CMake.

Unifying build systems is still important to many people; the 5-6 build systems (mac, apple win / wincairo, gtk, qt, and cmake) we have now continues to generate extra work.  By adopting CMake, we're eliminating and improving build systems.  GTK is working on adopting CMake now (EFL, Blackberry, WinCE are already there).  At the contributor's meeting this year, we discussed unifying build systems.  (Notes taken http://trac.webkit.org/wiki/May%202013%20Meeting%20-%20Unifying%20Build%20Systems%20-%20Notes).  The consensus at the meeting was this - even if we can't have all build systems switch to CMake now, any time a build system switches to CMake it's a win.
Comment 1 Mark Salisbury 2013-05-10 16:34:40 PDT
Created attachment 201439 [details]
Proposed fix

From build directory, execute:

cmake -G "Visual Studio 8 2005" -DPORT=Win -DPORT_FLAVOR=WinCairo <repo src path>

- Patch is based on the tree from April 15th; I'll rebase it soon.
- DumpRenderTree needs to be included.
- Patch is the same as the first commit off tree at https://github.com/masali-hp/webkit/commits/master

After this patch, I'd like to submit patches:
1) To allow building without CF/CFLite on windows
2) To build the same CMake Win port for WinCE

Because of this, there are a few places where you may notice something like this:

list(APPEND WebKit_Source_Files
     Common file 1
     Common file 2
     .....
)

list(APPEND WebKit_Source_Files
     CF specific file 1
     CF specific file 2
     .......
)
Comment 2 Patrick R. Gansterer 2013-05-10 19:43:02 PDT
Comment on attachment 201439 [details]
Proposed fix

Please reuse the WinCE files too: See https://github.com/paroga/webkit/commit/0b6c0bafbb02d7ef50914f8895de5d85265598e3
Also rebase your patch (e.g. pthread isn't required anymore) and make additional patches for the changes in the CPP files.
Comment 3 Mark Salisbury 2013-05-22 14:54:22 PDT
(In reply to comment #2)
> (From update of attachment 201439 [details])
> Please reuse the WinCE files too: See https://github.com/paroga/webkit/commit/0b6c0bafbb02d7ef50914f8895de5d85265598e3

Patrick - can you explain what you mean by "reuse the WinCE files too"?

I looked at your private branch and believe your design is:
- make each windows port a separate port (WinCairo, WinCE, eventually WinApple)
- a port can define what the parent port (fallback port) is, the fallback port is included first then the port file is included

I could modify my patch to go this route.  Currently with my patch you set PORT=Win, PORT_FLAVOR=WinCairo.  I could adopt your approach with minimal turmoil; the common build files would know to include an Options{fallback}.cmake and the files I've set up should work.

I want to add support for WinCE in these steps (ultimately my vision is to build WinCairo for WinCE with the same - perhaps a few exceptions - set of source files used to build for Windows XP):

1) Add a CMake port for WinCairo.
2) Make it possible to build WinCairo without CoreFoundation.
3) Upstream cairo changes to support WinCE.
4) Make changes to build WinCairo for WinCE.

> Also rebase your patch (e.g. pthread isn't required anymore) and make additional patches for the changes in the CPP files.

FastMalloc.cpp still requires pthreads when not USE_SYSTEM_MALLOC.

I just rebased, I'll attach that patch.
Comment 4 Mark Salisbury 2013-05-22 14:56:05 PDT
Created attachment 202624 [details]
Proposed fix -- rebased off 150501

No changelogs modified yet, I think there is probably more feedback before this can be integrated.
Comment 5 Patrick R. Gansterer 2013-05-22 20:14:30 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 201439 [details] [details])
> > Please reuse the WinCE files too: See https://github.com/paroga/webkit/commit/0b6c0bafbb02d7ef50914f8895de5d85265598e3
> 
> Patrick - can you explain what you mean by "reuse the WinCE files too"?

Many files in PlatformWin.cmake and PlatformWinCE.cmake are similar, so "reuse the list in the WinCE files" to avoid duplication-

> I looked at your private branch and believe your design is:
> - make each windows port a separate port (WinCairo, WinCE, eventually WinApple)
> - a port can define what the parent port (fallback port) is, the fallback port is included first then the port file is included
> 
> I could modify my patch to go this route.  Currently with my patch you set PORT=Win, PORT_FLAVOR=WinCairo.  I could adopt your approach with minimal turmoil; the common build files would know to include an Options{fallback}.cmake and the files I've set up should work.
> 
> I want to add support for WinCE in these steps (ultimately my vision is to build WinCairo for WinCE with the same - perhaps a few exceptions - set of source files used to build for Windows XP):
> 
> 1) Add a CMake port for WinCairo.
> 2) Make it possible to build WinCairo without CoreFoundation.
> 3) Upstream cairo changes to support WinCE.
> 4) Make changes to build WinCairo for WinCE.

I'm also working on the CMake changes, but I want to commit a clean version in the first step. So I'd like to merge FilesSystemWin(CE).cpp first, which is blocked with bug 116205.
Would it be ok for you to wait with your CMake files until the split of the CMake files (as in my github branch) is in trunk too? IMHO you can just start with your second point, since they do not depend on each other.

> > Also rebase your patch (e.g. pthread isn't required anymore) and make additional patches for the changes in the CPP files.
> 
> FastMalloc.cpp still requires pthreads when not USE_SYSTEM_MALLOC.

Not for OS(WINDOWS). All other Windows ports removed pthread dependency.
Comment 6 Patrick R. Gansterer 2013-08-06 01:52:50 PDT
Created attachment 208175 [details]
Patch

This is a first version to show the direction, but I'd like to hear some feedback from the current WinCairo maintainer...
Comment 7 WebKit Commit Bot 2013-08-06 01:55:55 PDT
Attachment 208175 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'CMakeLists.txt', u'ChangeLog', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/PlatformWin.cmake', u'Source/JavaScriptCore/PlatformWinCairo.cmake', u'Source/WebCore/ChangeLog', u'Source/WebCore/PlatformWinCairo.cmake', u'Source/WebKit/ChangeLog', u'Source/WebKit/PlatformWinCairo.cmake', u'Source/WebKit/WebKit.vcxproj/WebKitExportGenerator/WebKitExports.def.in', u'Source/WebKit/cf/ChangeLog', u'Source/WebKit/cf/WebCoreSupport/WebInspectorClientCF.cpp', u'Source/cmake/OptionsWinCairo.cmake', u'Source/cmake/OptionsWindows.cmake', u'Tools/CMakeLists.txt', u'Tools/ChangeLog', u'Tools/DumpRenderTree/win/CMakeLists.txt', u'Tools/WinLauncher/CMakeLists.txt', u'Tools/win/DLLLauncher/CMakeLists.txt']" exit_code: 1
Source/cmake/OptionsWinCairo.cmake:50:  Use lowercase command "file"  [command/lowercase] [5]
Source/cmake/OptionsWinCairo.cmake:54:  Use lowercase command "file"  [command/lowercase] [5]
Source/cmake/OptionsWinCairo.cmake:58:  Use lowercase command "file"  [command/lowercase] [5]
Source/cmake/OptionsWinCairo.cmake:62:  Use lowercase command "file"  [command/lowercase] [5]
Total errors found: 4 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Build Bot 2013-08-13 21:14:41 PDT
Comment on attachment 208175 [details]
Patch

Attachment 208175 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1451705
Comment 9 Patrick R. Gansterer 2013-09-21 01:12:53 PDT
Created attachment 212265 [details]
Patch (based on r156181)

Depending on the used perl version https://bugs.webkit.org/attachment.cgi?id=212261&action=review must be applied first.

To build:
- All requrements for the build tool of trac.webkit.org/wiki/WinCE apply (bison, flex, gperf, perl, python)
- Download and extract the WinCairoRequrements.zip
- Run the following command in an empty directory: cmake -G "Visual Studio 10" -DPORT=WinCairo path/to/source
- Open the generated sln file and build or run "cmake --build ." in that directory
Comment 10 Alex Christensen 2015-04-02 23:15:28 PDT
I've updated most of this and I'm writing this with a WinCairo WinLauncher browser made using CMake.
Comment 11 Alex Christensen 2015-04-02 23:52:38 PDT
Created attachment 250047 [details]
Patch
Comment 12 WebKit Commit Bot 2015-04-02 23:54:59 PDT
Attachment 250047 [details] did not pass style-queue:


ERROR: Tools/WinLauncher/stdafx.h:73:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Gyuyoung Kim 2015-04-03 00:50:13 PDT
Comment on attachment 250047 [details]
Patch

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

> Source/WebCore/PlatformWinCairo.cmake:106
> +    platform/image-decoders/bmp/BMPImageReader.cpp

Wrong alphabet order.

> Source/cmake/OptionsWindows.cmake:8
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_GEOLOCATION ON)

nit: wrong alphabet order.

> Tools/CMakeLists.txt:30
> +if (WIN32)

To be compatible with existing code, how about using below condition ?

elseif ("${PORT}" STREQUAL "WIN32")
    add_subdirectory(DumpRenderTree/win)
    add_subdirectory(WinLauncher)
endif ()

> Tools/DumpRenderTree/win/CMakeLists.txt:23
> +    ../AccessibilityController.cpp

Doesn't these files already included in DumpRenderTree/CMakeLists.txt ?

> Tools/DumpRenderTree/win/CMakeLists.txt:50
> +set(DumpRenderTree_LIBRARIES ${DumpRenderTree_LIBRARIES})

Why do we need to set DumpRenderTree_LIBRARIES again ?

> Tools/WinLauncher/stdafx.h:73
> +#include <wtf/ExportMacros.h>

nit: wrong alphabet order.
Comment 14 Csaba Osztrogon√°c 2015-04-03 04:58:15 PDT
Comment on attachment 250047 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:-280
> -    Modules/streams/ReadableStream.idl
> -    Modules/streams/ReadableStreamReader.idl
> -

why? We shouldn't switch off this feature everywhere.
If WinCairo has problems with STREAM_API, define it to 0.

> Source/WebCore/CMakeLists.txt:-931
> -    Modules/streams/ReadableStream.cpp
> -    Modules/streams/ReadableStreamReader.cpp

why?

> Source/WebCore/CMakeLists.txt:-1163
> -    bindings/js/JSReadableStreamCustom.cpp
> -    bindings/js/JSReadableStreamReaderCustom.cpp
> -    bindings/js/ReadableStreamJSSource.cpp

why?
Comment 15 Alex Christensen 2015-04-03 09:08:26 PDT
(In reply to comment #13)
> > Tools/CMakeLists.txt:30
> > +if (WIN32)
> 
> To be compatible with existing code, how about using below condition ?
> 
> elseif ("${PORT}" STREQUAL "WIN32")
>     add_subdirectory(DumpRenderTree/win)
>     add_subdirectory(WinLauncher)
> endif ()
The other things obviously need to be fixed in a new patch, but I think WIN32 is nicer than this:
"${PORT}" STREQUAL "WinCairo" OR "${PORT}" STREQUAL "AppleWin"

Would you be opposed if I started using WIN32 and APPLE CMake variables in the CMake code for OS-specific things?
Comment 16 Gyuyoung Kim 2015-04-06 02:43:03 PDT
(In reply to comment #15)
> (In reply to comment #13)
> > > Tools/CMakeLists.txt:30
> > > +if (WIN32)
> > 
> > To be compatible with existing code, how about using below condition ?
> > 
> > elseif ("${PORT}" STREQUAL "WIN32")
> >     add_subdirectory(DumpRenderTree/win)
> >     add_subdirectory(WinLauncher)
> > endif ()
> The other things obviously need to be fixed in a new patch, but I think
> WIN32 is nicer than this:
> "${PORT}" STREQUAL "WinCairo" OR "${PORT}" STREQUAL "AppleWin"
> 
> Would you be opposed if I started using WIN32 and APPLE CMake variables in
> the CMake code for OS-specific things?

I don't have big opposition. Just wanted to be aligned with existing one. So, if someone else doesn't mention about this use, I see. Let's use it you want.
Comment 17 Alex Christensen 2015-04-07 09:03:35 PDT
Created attachment 250268 [details]
Patch
Comment 18 Alex Christensen 2015-04-07 09:11:12 PDT
I removed the questionable parts of this patch.  I'll land them in another patch getting everything working the right way.
Comment 19 Chris Dumez 2015-04-07 16:46:12 PDT
Comment on attachment 250268 [details]
Patch

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

Looks reasonable. r=me.

> Source/WebCore/PlatformWinCairo.cmake:105
> +    platform/image-decoders/bmp/BMPImageDecoder.cpp

alphabetical sorting.

> Source/WebCore/PlatformWinCairo.cmake:106
> +    platform/image-decoders/bmp/BMPImageReader.cpp

ditto.
Comment 20 Alex Christensen 2015-04-08 09:49:07 PDT
http://trac.webkit.org/changeset/182545
Comment 21 Alex Christensen 2015-12-10 11:04:24 PST
*** Bug 121832 has been marked as a duplicate of this bug. ***