Bug 153189

Summary: [CMake] Unclear distinction between WebKitHelpers and WebKitMacros
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKit Misc.Assignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Minor CC: achristensen, annulen, bugs-noreply, lforschler, mcatanzaro, mrobinson, ossy, tonikitoo
Priority: P2    
Version: Other   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
none
Patch tonikitoo: review+

Michael Catanzaro
Reported 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.
Attachments
Patch (41.46 KB, patch)
2017-07-14 10:47 PDT, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Patch (39.74 KB, patch)
2017-07-14 11:16 PDT, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Patch (39.63 KB, patch)
2017-07-14 12:27 PDT, Michael Catanzaro 		tonikitoo: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Alex Christensen
Comment 1 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.
Konstantin Tokarev
Comment 2 2017-07-14 10:45:24 PDT
+1 for merge. There are even duplications between two files, e.g. ADD_SOURCE_DEPENDENCIES
Michael Catanzaro
Comment 3 2017-07-14 10:47:16 PDT
Created attachment 315445 [details] Patch
Konstantin Tokarev
Comment 4 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
Michael Catanzaro
Comment 5 2017-07-14 11:16:23 PDT
Created attachment 315459 [details] Patch
Michael Catanzaro
Comment 6 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.
Konstantin Tokarev
Comment 7 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
Michael Catanzaro
Comment 8 2017-07-14 12:27:14 PDT
Created attachment 315471 [details] Patch
Michael Catanzaro
Comment 9 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!
Antonio Gomes
Comment 10 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!
Michael Catanzaro
Comment 11 2017-07-14 13:29:47 PDT
Committed r219522: <http://trac.webkit.org/changeset/219522>
Note You need to log in before you can comment on or make changes to this bug.