Bug 143956 - [CMake] Should be possible for an option to conflict with other options
Summary: [CMake] Should be possible for an option to conflict with other options
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Enhancement
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on: 143839
Blocks: 144105
  Show dependency treegraph
 
Reported: 2015-04-20 11:37 PDT by Michael Catanzaro
Modified: 2015-04-27 15:05 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.94 KB, patch)
2015-04-20 11:47 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.48 KB, patch)
2015-04-20 11:49 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (3.51 KB, patch)
2015-04-22 11:14 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
[CMake] Should be possible for an option to conflict with other options (4.41 KB, patch)
2015-04-23 08:59 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (4.40 KB, patch)
2015-04-23 13:19 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
[CMake] Should be possible for an option to conflict with other options (3.98 KB, patch)
2015-04-23 18:36 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-20 11:37:39 PDT
Currently it's not possible for an option to conflict with another option. This makes it awkward to manually check whether the combination of available options is valid. For example, for bug #143558 I want to introduce ENABLE_X11_TARGET and ENABLE_WAYLAND_TARGET options, but these are mutually exclusive. It'd be nice to declare this in one line, rather than handling it manually.
Comment 1 Michael Catanzaro 2015-04-20 11:47:35 PDT
Created attachment 251174 [details]
Patch
Comment 2 Michael Catanzaro 2015-04-20 11:49:24 PDT
Created attachment 251176 [details]
Patch
Comment 3 Michael Catanzaro 2015-04-22 11:14:16 PDT
Created attachment 251341 [details]
Patch
Comment 4 Martin Robinson 2015-04-22 11:44:13 PDT
Comment on attachment 251341 [details]
Patch

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

> Source/cmake/WebKitFeatures.cmake:207
> +macro(_WEBKIT_OPTION_ENFORCE_CONFLICTS)
> +    set(_OPTION_CHANGED TRUE)
> +    while (${_OPTION_CHANGED})
> +        set(_OPTION_CHANGED FALSE)
> +        foreach (_name ${_WEBKIT_AVAILABLE_OPTIONS})
> +            if (${_name})
> +                foreach (_conflict ${_WEBKIT_AVAILABLE_OPTIONS_${_name}_CONFLICTS})
> +                    if (${_conflict})
> +                        message(STATUS "Disabling ${_name} since ${_conflict} is enabled.")
> +                        set(${_name} OFF)
> +                        set(_OPTION_CHANGED TRUE)
> +                        set(_CONTINUE_DEPENDENCY_PROCESSING TRUE)
>                          break ()
>                      endif ()
>                  endforeach ()

I think it makes sense to error out the build immediately when options conflict. As with the other patch, it probably makes sense to call this macro with each feature name as an argument.
Comment 5 Michael Catanzaro 2015-04-23 08:59:54 PDT
Created attachment 251441 [details]
[CMake] Should be possible for an option to conflict with other options
Comment 6 Michael Catanzaro 2015-04-23 09:07:21 PDT
(In reply to comment #4)
> I think it makes sense to error out the build immediately when options
> conflict.

That's definitely desirable for options that the user explicitly specified: e.g. if the user picks -DENABLE_X11_TARGET=ON and -DENABLE_WAYLAND_TARGET=ON, then an error would be dandy. The reason I did not implement that here is that it could be nice for options to be automatically adjusted to make things work: I figure it would be nice to be able to pass -DENABLE_WAYLAND_TARGET=ON without being required to also pass -DENABLE_X11_TARGET=OFF as well. Making it an error would require more typing, but reduce the chance of mistakes. I have no strong preference either way; if you still want it to be an error, I'll update the patch again.
Comment 7 Michael Catanzaro 2015-04-23 10:09:18 PDT
(In reply to comment #6)
> I have no strong preference
> either way; if you still want it to be an error, I'll update the patch again.

Thinking about this more, I think it's desirable to keep the current behavior. Say we have the public option ENABLE_VIDEO. ENABLE_VIDEO_TRACK depends on that. But we don't want to make ENABLE_VIDEO_TRACK public, so the user will be receiving an error message complaining that he has enabled a feature that apparently does not exist.
Comment 8 Michael Catanzaro 2015-04-23 13:19:34 PDT
Created attachment 251476 [details]
Patch
Comment 9 Michael Catanzaro 2015-04-23 15:11:31 PDT
Comment on attachment 251476 [details]
Patch

We decided to error out anyway. There should only be a few conflicts, and forcing the user to be explicit about a couple things is OK. The example I gave above failed because that is handled by WEBKIT_OPTION_DEPEND, not WEBKIT_OPTION_CONFLICT.
Comment 10 Michael Catanzaro 2015-04-23 18:36:48 PDT
Created attachment 251525 [details]
[CMake] Should be possible for an option to conflict with other options
Comment 11 Michael Catanzaro 2015-04-23 18:37:29 PDT
Example error:

CMake Error at Source/cmake/WebKitFeatures.cmake:206 (message):
  ENABLE_ACCELERATED_2D_CANVAS conflicts with ENABLE_GLES2: you must disable
  one or the other.
Call Stack (most recent call first):
  Source/cmake/WebKitFeatures.cmake:214 (_WEBKIT_OPTION_ENFORCE_CONFLICTS)
  Source/cmake/WebKitFeatures.cmake:237 (_WEBKIT_OPTION_ENFORCE_ALL_CONFLICTS)
  Source/cmake/OptionsGTK.cmake:181 (WEBKIT_OPTION_END)
  CMakeLists.txt:156 (include)


-- Configuring incomplete, errors occurred!
See also "/home/mcatanzaro/WebKit/WebKitBuild/GNOME/CMakeFiles/CMakeOutput.log".
See also "/home/mcatanzaro/WebKit/WebKitBuild/GNOME/CMakeFiles/CMakeError.log".
Comment 12 WebKit Commit Bot 2015-04-27 15:05:17 PDT
Comment on attachment 251525 [details]
[CMake] Should be possible for an option to conflict with other options

Clearing flags on attachment: 251525

Committed r183427: <http://trac.webkit.org/changeset/183427>
Comment 13 WebKit Commit Bot 2015-04-27 15:05:21 PDT
All reviewed patches have been landed.  Closing bug.