Bug 72815 - [CMake] Share build system options across ports
Summary: [CMake] Share build system options across ports
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Patrick R. Gansterer
URL:
Keywords:
Depends on: 72812
Blocks: 83590
  Show dependency treegraph
 
Reported: 2011-11-19 17:52 PST by Patrick R. Gansterer
Modified: 2012-05-02 23:14 PDT (History)
4 users (show)

See Also:


Attachments
Patch (25.33 KB, patch)
2011-11-19 17:58 PST, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (25.33 KB, patch)
2011-12-02 03:09 PST, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (25.29 KB, patch)
2011-12-02 04:01 PST, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (25.90 KB, patch)
2011-12-02 04:12 PST, Patrick R. Gansterer
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Patch (26.93 KB, patch)
2012-04-09 02:42 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2011-11-19 17:52:24 PST
[CMake] Share build system options across ports
Comment 1 Patrick R. Gansterer 2011-11-19 17:58:16 PST
Created attachment 115965 [details]
Patch

This patch depends on the patch at bug 72815.
Comment 2 Daniel Bates 2011-12-01 09:39:01 PST
Comment on attachment 115965 [details]
Patch

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

Yay for less duplication!

> Source/cmake/WebKitFeatures.cmake:3
> +MACRO (WEBKIT_OPTION_DEFINE _name _description _initialvalue)

Nit: I've noticed an inconsistency in the placement of a space between the directive name and the opening parenthesis '('. You seem to always place a space between the MACRO directive and its opening parenthesis, but you don't carry over this style to other directives such as MESSAGE, SET, and STRING throughout this patch. I suggest picking a style and being consistent.
Comment 3 Patrick R. Gansterer 2011-12-02 03:09:03 PST
Created attachment 117597 [details]
Patch
Comment 4 Patrick R. Gansterer 2011-12-02 04:01:55 PST
Created attachment 117604 [details]
Patch
Comment 5 Gyuyoung Kim 2011-12-02 04:10:08 PST
Comment on attachment 117604 [details]
Patch

Attachment 117604 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10689367
Comment 6 Patrick R. Gansterer 2011-12-02 04:12:59 PST
Created attachment 117606 [details]
Patch
Comment 7 Gyuyoung Kim 2011-12-02 04:32:43 PST
Comment on attachment 117606 [details]
Patch

Attachment 117606 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10728029
Comment 8 Raphael Kubo da Costa (:rakuco) 2011-12-02 06:07:58 PST
(In reply to comment #1)
> Created an attachment (id=115965) [details]
> Patch
> 
> This patch depends on the patch at bug 72815.

I thought this was bug 72815? :)

WEBKIT_OPTION_BEGIN and WEBKIT_OPTION_END don't sound very natural to me. Wouldn't it be possible to move the default options to OptionsCommon.cmake and then just override them in Options${PORT}.cmake?
Comment 9 Raphael Kubo da Costa (:rakuco) 2011-12-02 06:08:25 PST
+dbates in case he has anything to add to my comment.
Comment 10 Patrick R. Gansterer 2011-12-02 06:18:36 PST
(In reply to comment #8)
> (In reply to comment #1)
> > Created an attachment (id=115965) [details] [details]
> > Patch
> > 
> > This patch depends on the patch at bug 72815.
> 
> I thought this was bug 72815? :)
> 
> WEBKIT_OPTION_BEGIN and WEBKIT_OPTION_END don't sound very natural to me. Wouldn't it be possible to move the default options to OptionsCommon.cmake and then just override them in Options${PORT}.cmake?
That's exactly what this patch does? It defines the default options in WebKitFeatures.cmake and then allows ports to overwrite the defaults.
Thanks to the many includes, it's not possible to remove the WEBKIT_OPTION_BEGIN and WEBKIT_OPTION_END for now. I need to define them first, then overwrite the defaults and then call the CMake OPTION() to make them visible in CMake GUI.
Comment 11 Raphael Kubo da Costa (:rakuco) 2011-12-02 07:08:33 PST
(In reply to comment #10)
> (In reply to comment #8)
> > WEBKIT_OPTION_BEGIN and WEBKIT_OPTION_END don't sound very natural to me. Wouldn't it be possible to move the default options to OptionsCommon.cmake and then just override them in Options${PORT}.cmake?
> That's exactly what this patch does? It defines the default options in WebKitFeatures.cmake and then allows ports to overwrite the defaults.
> Thanks to the many includes, it's not possible to remove the WEBKIT_OPTION_BEGIN and WEBKIT_OPTION_END for now. I need to define them first, then overwrite the defaults and then call the CMake OPTION() to make them visible in CMake GUI.

I know. My point is that I think it would look cleaner if OptionsCommon.cmake did what WEBKIT_OPTION_BEGIN does (or called it), then Options${PORT}.cmake overrode the options it needs to and WEBKIT_OPTION_END could be called by the top-level CMakeLists.txt after including Options*.cmake (or the code could be moved there). This would move the begin-end combo to a single place instead of the current 3.
Comment 12 Patrick R. Gansterer 2011-12-02 07:17:50 PST
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > WEBKIT_OPTION_BEGIN and WEBKIT_OPTION_END don't sound very natural to me. Wouldn't it be possible to move the default options to OptionsCommon.cmake and then just override them in Options${PORT}.cmake?
> > That's exactly what this patch does? It defines the default options in WebKitFeatures.cmake and then allows ports to overwrite the defaults.
> > Thanks to the many includes, it's not possible to remove the WEBKIT_OPTION_BEGIN and WEBKIT_OPTION_END for now. I need to define them first, then overwrite the defaults and then call the CMake OPTION() to make them visible in CMake GUI.
> 
> I know. My point is that I think it would look cleaner if OptionsCommon.cmake did what WEBKIT_OPTION_BEGIN does (or called it), then Options${PORT}.cmake overrode the options it needs to and WEBKIT_OPTION_END could be called by the top-level CMakeLists.txt after including Options*.cmake (or the code could be moved there). This would move the begin-end combo to a single place instead of the current 3.

IMHO the current state of the CMake build system is far away from perfect. This is only a step in the right direction. For me the alternative is to write the whole CMake files starting with null. I also have many many troubles with other port (see bug 72816 and bug 73100). As you might know it's hard to change the current stuff in reviewable parts. The first commit of the system was easy since it didn't get a deep review.
So if it's ok I'd like to get this in (with EFL fixes), since it is at least better than the current version.
Comment 13 Eric Seidel (no email) 2011-12-19 10:47:48 PST
Comment on attachment 115965 [details]
Patch

Cleared Daniel Bates's review+ from obsolete attachment 115965 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 14 Patrick R. Gansterer 2012-04-09 02:42:21 PDT
Created attachment 136202 [details]
Patch

It build on my machine; submit for EFL EWS analysis.
Comment 15 Patrick R. Gansterer 2012-04-09 03:45:26 PDT
Comment on attachment 136202 [details]
Patch

Clearing flags on attachment: 136202

Committed r113570: <http://trac.webkit.org/changeset/113570>
Comment 16 Patrick R. Gansterer 2012-04-09 03:45:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Eric Seidel (no email) 2012-05-02 23:14:07 PDT
Comment on attachment 136202 [details]
Patch

With a little refactoring to move the ENABLE_ declarations into their own file, this would be simple to autogenerate as per bug 85456.