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.
Created attachment 251174 [details] Patch
Created attachment 251176 [details] Patch
Created attachment 251341 [details] Patch
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.
Created attachment 251441 [details] [CMake] Should be possible for an option to conflict with other options
(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.
(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.
Created attachment 251476 [details] Patch
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.
Created attachment 251525 [details] [CMake] Should be possible for an option to conflict with other options
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 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>
All reviewed patches have been landed. Closing bug.