Bug 72815 – [CMake] Share build system options across ports

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Patrick R. Gansterer]

[CMake] Share build system options across ports

[Patrick R. Gansterer]

Created attachment 115965 [details]
Patch

This patch depends on the patch at bug 72815.

[Daniel Bates]

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.

[Patrick R. Gansterer]

Created attachment 117597 [details]
Patch

[Patrick R. Gansterer]

Created attachment 117604 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 117604 [details]
Patch

Attachment 117604 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10689367

[Patrick R. Gansterer]

Created attachment 117606 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 117606 [details]
Patch

Attachment 117606 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10728029

[Raphael Kubo da Costa (:rakuco)]

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

[Raphael Kubo da Costa (:rakuco)]

+dbates in case he has anything to add to my comment.

[Patrick R. Gansterer]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Patrick R. Gansterer]

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

[Eric Seidel (no email)]

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.

[Patrick R. Gansterer]

Created attachment 136202 [details]
Patch

It build on my machine; submit for EFL EWS analysis.

[Patrick R. Gansterer]

Comment on attachment 136202 [details]
Patch

Clearing flags on attachment: 136202

Committed r113570: <http://trac.webkit.org/changeset/113570>

[Patrick R. Gansterer]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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.