Bug 153189 - [CMake] Unclear distinction between WebKitHelpers and WebKitMacros
Summary: [CMake] Unclear distinction between WebKitHelpers and WebKitMacros
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Minor
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-16 18:51 PST by Michael Catanzaro
Modified: 2017-07-14 13:29 PDT (History)
8 users (show)

See Also:


Attachments
Patch (41.46 KB, patch)
2017-07-14 10:47 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (39.74 KB, patch)
2017-07-14 11:16 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (39.63 KB, patch)
2017-07-14 12:27 PDT, Michael Catanzaro
tonikitoo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2016-01-16 18:51:54 PST
What is the difference between WebKitHelpers.cmake and WebKitMacros.cmake?

If nobody knows, I am thinking we should merge the contents of WebKitHelpers.cmake into WebKitMacros.cmake.
Comment 1 Alex Christensen 2016-01-20 11:38:07 PST
I think the two could be merged.  I also think that many of the macros are only used in  WebCore, and those macros should be defined in WebCore/CMakeLists.txt.  cmake/WebKitMacros.cmake should be for macros that are shared by multiple projects.
Comment 2 Konstantin Tokarev 2017-07-14 10:45:24 PDT
+1 for merge. There are even duplications between two files, e.g. ADD_SOURCE_DEPENDENCIES
Comment 3 Michael Catanzaro 2017-07-14 10:47:16 PDT
Created attachment 315445 [details]
Patch
Comment 4 Konstantin Tokarev 2017-07-14 10:54:39 PDT
I don't like that macros (especially large ones) are thrown into thick of WebCore/CMakeLists.txt

I usually try to group macros at the top of the file, though here it may be a better idea to introduce separate WebCoreMacros.cmake file

Rationale is that "regular" cmake code is usually more readable (maybe almost declarative in good cases) than macro code
Comment 5 Michael Catanzaro 2017-07-14 11:16:23 PDT
Created attachment 315459 [details]
Patch
Comment 6 Michael Catanzaro 2017-07-14 11:19:41 PDT
OK, I added WebCoreMacros.cmake. Please check to see if you approve of the hack I used to include it in WebCore/CMakeLists.txt.
Comment 7 Konstantin Tokarev 2017-07-14 11:29:12 PDT
Comment on attachment 315459 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:7
> +include(WebCoreMacros)

Just include(WebCoreMacros.cmake), no need to change module path
Comment 8 Michael Catanzaro 2017-07-14 12:27:14 PDT
Created attachment 315471 [details]
Patch
Comment 9 Michael Catanzaro 2017-07-14 12:27:40 PDT
(In reply to Konstantin Tokarev from comment #7)
> Just include(WebCoreMacros.cmake), no need to change module path

OK, good tip!
Comment 10 Antonio Gomes 2017-07-14 13:24:56 PDT
> OK, I added WebCoreMacros.cmake. Please check to see if you approve of the
> hack I used to include it in WebCore/CMakeLists.txt.

(In reply to Michael Catanzaro from comment #6)

+1 for WebCoreMacros.cmake!
Comment 11 Michael Catanzaro 2017-07-14 13:29:47 PDT
Committed r219522: <http://trac.webkit.org/changeset/219522>