Bug 143572 - [CMake] Options should be marked as advanced by default
Summary: [CMake] Options should be marked as advanced by default
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Enhancement
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks: 143558
  Show dependency treegraph
 
Reported: 2015-04-09 13:26 PDT by Michael Catanzaro
Modified: 2015-04-11 11:15 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.64 KB, patch)
2015-04-09 13:47 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.64 KB, patch)
2015-04-09 19:50 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2015-04-09 13:26:19 PDT
Options defined with WEBKIT_OPTION_DEFINE should be marked as advanced so that they don't show up in CMake GUIs or with cmake -L. We also shouldn't print them when running cmake. This prevents ports from unexpectedly getting new public options. The idea is that you only make an option public if you're really willing to support building both with and without it.

If WEBKIT_OPTION_DEFINE_PORT_VALUE is called, then the option should become public. WEBKIT_OPTION_PRIVATE_PORT_VALUE is provided to change the default value for a port without making it public.

WEBKIT_OPTION_DEFINE_PUBLIC is provided to create a new port-specific option and mark it public without needing to use both WEBKIT_OPTION_DEFINE and WEBKIT_OPTION_DEFINE_PORT_VALUE.

Note that there is no enforcement: users can still set all of these options if they really want to, as before, e.g. 'cmake -DSOME_UNSUPPORTED_OPTION=ON', but probably only developers will do this.
Comment 1 Michael Catanzaro 2015-04-09 13:32:54 PDT
Warning: if your port doesn't use WEBKIT_OPTION_DEFINE_PORT_VALUE for an option, then this patch will cause that option to disappear!

This seemed like the best way to make it easy to hide options, and make sure new options don't unexpectedly appear for particular ports, without making all the Mac and EFL options disappear.
Comment 2 Alex Christensen 2015-04-09 13:35:16 PDT
That should not be a problem for the Mac and Windows ports' CMake files right now.  They are experimental and under construction right now anyways.
Comment 3 Michael Catanzaro 2015-04-09 13:47:08 PDT
Created attachment 250464 [details]
Patch
Comment 4 Michael Catanzaro 2015-04-09 19:35:25 PDT
Comment on attachment 250464 [details]
Patch

I'll need to try again. WEBKIT_OPTION_DEFINE_PUBLIC doesn't work.
Comment 5 Michael Catanzaro 2015-04-09 19:50:41 PDT
Created attachment 250496 [details]
Patch
Comment 6 Gyuyoung Kim 2015-04-09 20:10:23 PDT
Comment on attachment 250496 [details]
Patch

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

I think _PUBLIC and _PRIVATE macros look like better method to define options. Basically I agree to use it. However someone else might want to take a look this patch before landing.

> Source/cmake/WebKitFeatures.cmake:11
> +    set(_WEBKIT_AVAILABLE_OPTIONS_ISPUBLIC_${_name} TRUE)

Isn't "_IS_PUBLIC" looks more clear than "_ISPUBLIC" ?
Comment 7 Michael Catanzaro 2015-04-09 20:41:56 PDT
(In reply to comment #6)
> Isn't "_IS_PUBLIC" looks more clear than "_ISPUBLIC" ?

I almost did that, but went with ISPUBLIC to match _WEBKIT_AVAILABLE_OPTIONS_INITIALVALUE. Probably IS_PUBLIC would be better, indeed.
Comment 8 WebKit Commit Bot 2015-04-11 11:15:31 PDT
Comment on attachment 250496 [details]
Patch

Clearing flags on attachment: 250496

Committed r182658: <http://trac.webkit.org/changeset/182658>
Comment 9 WebKit Commit Bot 2015-04-11 11:15:42 PDT
All reviewed patches have been landed.  Closing bug.