Bug 147083 - resurrect CMake build on Windows
Summary: resurrect CMake build on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-18 22:49 PDT by Alex Christensen
Modified: 2015-08-03 13:23 PDT (History)
3 users (show)

See Also:


Attachments
Patch (31.48 KB, patch)
2015-07-18 23:20 PDT, Alex Christensen
gyuyoung.kim: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2015-07-18 22:49:42 PDT
Updated and added missing PlatformWin.cmake files.  Based loosely on Mark and Patrick's work in https://bugs.webkit.org/show_bug.cgi?id=72816
Comment 1 Alex Christensen 2015-07-18 23:20:44 PDT
Created attachment 257055 [details]
Patch
Comment 2 Gyuyoung Kim 2015-07-19 20:05:08 PDT
Comment on attachment 257055 [details]
Patch

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

> Source/WebKit/PlatformWin.cmake:211
> +    win/WebCoreSupport/EmbeddedWidget.h

Question. should we include header files to Source file list ?
Comment 3 Alex Christensen 2015-07-20 00:05:08 PDT
(In reply to comment #2)
> Question. should we include header files to Source file list ?
They're not needed to build correctly, but Visual Studio will only search in files included in the generated projects. If they're not included in other CMake files, then it doesn't make any sense to include them here. I don't care either way right now
Comment 4 Gyuyoung Kim 2015-07-20 00:36:15 PDT
Comment on attachment 257055 [details]
Patch

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

LGTM on cmake side except for some nits. However this patch modifies some sources though, I think it is trivial changes. rs=me for win port.

> Source/WebKit/PlatformWin.cmake:245
> +list(APPEND WebKit_SOURCES_WebCoreSupport

I don't know why below files can't be added to 206 lines.

> Source/WebKit/PlatformWin.cmake:425
> +set(WebKit_LIBRARY_TYPE SHARED)

WebKit_LIBRARY_TYPE is defined in CMakeLists.txt. So it looks this is duplicated definition. 

http://trac.webkit.org/browser/trunk/CMakeLists.txt#L143
Comment 5 Alex Christensen 2015-07-20 08:57:14 PDT
Addressed nits and committed http://trac.webkit.org/changeset/187022
Thanks for the review!
Comment 6 Alex Christensen 2015-07-20 14:44:56 PDT
Build fix in http://trac.webkit.org/changeset/187037
Sorry about that :(
Comment 7 Csaba Osztrogonác 2015-08-03 03:53:37 PDT
Comment on attachment 257055 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:3516
> +list(APPEND WebCore_SOURCES ${DERIVED_SOURCES_WEBCORE_DIR}/EventTargetInterfaces.h)

Why do we add a header to the source list? Do we really need it for the Windows build?
Comment 8 Alex Christensen 2015-08-03 13:23:26 PDT
(In reply to comment #7)
> Comment on attachment 257055 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257055&action=review
> 
> > Source/WebCore/CMakeLists.txt:3516
> > +list(APPEND WebCore_SOURCES ${DERIVED_SOURCES_WEBCORE_DIR}/EventTargetInterfaces.h)
> 
> Why do we add a header to the source list? Do we really need it for the
> Windows build?
Yes, it wasn't generating it unless I added this line. Removing this line would probably only break clean builds. Is it a problem?