Bug 115944 – Add CMake base build system for WinCairo port

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Mark Salisbury]

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.

[Mark Salisbury]

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
     .......
)

[Patrick R. Gansterer]

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.

[Mark Salisbury]

(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.

[Mark Salisbury]

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.

[Patrick R. Gansterer]

(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.

[Patrick R. Gansterer]

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...

[WebKit Commit Bot]

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.

[Build Bot]

Comment on attachment 208175 [details]
Patch

Attachment 208175 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1451705

[Patrick R. Gansterer]

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

[Alex Christensen]

I've updated most of this and I'm writing this with a WinCairo WinLauncher browser made using CMake.

[Alex Christensen]

Created attachment 250047 [details]
Patch

[WebKit Commit Bot]

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.

[Gyuyoung Kim]

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.

[Csaba Osztrogonác]

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?

[Alex Christensen]

(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?

[Gyuyoung Kim]

(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.

[Alex Christensen]

Created attachment 250268 [details]
Patch

[Alex Christensen]

I removed the questionable parts of this patch.  I'll land them in another patch getting everything working the right way.

[Chris Dumez]

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.

[Alex Christensen]

http://trac.webkit.org/changeset/182545

[Alex Christensen]

*** Bug 121832 has been marked as a duplicate of this bug. ***