[CMake] Share build system options across ports
Created attachment 115965 [details] Patch This patch depends on the patch at bug 72815.
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.
Created attachment 117597 [details] Patch
Created attachment 117604 [details] Patch
Comment on attachment 117604 [details] Patch Attachment 117604 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10689367
Created attachment 117606 [details] Patch
Comment on attachment 117606 [details] Patch Attachment 117606 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10728029
(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?
+dbates in case he has anything to add to my comment.
(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.
(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.
(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 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.
Created attachment 136202 [details] Patch It build on my machine; submit for EFL EWS analysis.
Comment on attachment 136202 [details] Patch Clearing flags on attachment: 136202 Committed r113570: <http://trac.webkit.org/changeset/113570>
All reviewed patches have been landed. Closing bug.
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.