Bug 143956 – [CMake] Should be possible for an option to conflict with other options

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 251174 [details]
Patch

[Michael Catanzaro]

Created attachment 251176 [details]
Patch

[Michael Catanzaro]

Created attachment 251341 [details]
Patch

[Martin Robinson]

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.

[Michael Catanzaro]

Created attachment 251441 [details]
[CMake] Should be possible for an option to conflict with other options

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

Created attachment 251476 [details]
Patch

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 251525 [details]
[CMake] Should be possible for an option to conflict with other options

[Michael Catanzaro]

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

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.