Bug 143839 – [CMake] Should be possible for an option to depend on multiple options

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Michael Catanzaro]

Currently it's not possible for an option to depend on more than one other option. This makes it awkward to manually check whether the combination of available options is valid. For example, my patch in bug #143558 uses the following code:

WEBKIT_OPTION_DEPEND(USE_GSTREAMER_GL ENABLE_VIDEO)
WEBKIT_OPTION_DEPEND(USE_GSTREAMER_GL USE_OPENGL)
WEBKIT_OPTION_DEPEND(USE_REDIRECTED_XCOMPOSITE_WINDOW ENABLE_X11_TARGET)
WEBKIT_OPTION_DEPEND(USE_REDIRECTED_XCOMPOSITE_WINDOW USE_OPENGL)

It would be nice if that worked as expected. The current behavior is that the most recent call to WEBKIT_OPTION_DEPEND overrides the previous call.

[Michael Catanzaro]

Created attachment 250939 [details]
Patch

[Michael Catanzaro]

Created attachment 251177 [details]
[CMake] Should be possible for an option to depend on multiple options

[Michael Catanzaro]

Created attachment 251318 [details]
Patch

[Martin Robinson]

Comment on attachment 251318 [details]
Patch

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

Looks good to me. Small suggestion for a cleanup:

> Source/cmake/WebKitFeatures.cmake:185
> +macro(_WEBKIT_OPTION_ENFORCE_DEPENDS)
> +    set(_OPTION_CHANGED TRUE)
> +    while (${_OPTION_CHANGED})
> +        set(_OPTION_CHANGED FALSE)
> +        foreach (_name ${_WEBKIT_AVAILABLE_OPTIONS})
> +            if (${_name})
> +                foreach (_dependency ${_WEBKIT_AVAILABLE_OPTIONS_${_name}_DEPENDENCIES})
> +                    if (NOT ${_dependency})
> +                        message(STATUS "Disabling ${_name} since ${_dependency} is disabled.")
> +                        set(${_name} OFF)
> +                        set(_OPTION_CHANGED TRUE)
> +                        break ()
> +                    endif ()
> +                endforeach ()
> +            endif ()
> +        endforeach ()
> +    endwhile ()
> +endmacro()

You could eliminate the outer loop by running this macro for every option and passing the option name as an argument. I think that would simplify things quite a bit.

> Source/cmake/WebKitFeatures.cmake:190
>      list(SORT _WEBKIT_AVAILABLE_OPTIONS)
> +
>      set(_MAX_FEATURE_LENGTH 0)

This newline looks unrelated.

[Michael Catanzaro]

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

[Michael Catanzaro]

Created attachment 251442 [details]
[CMake] Should be possible for an option to depend on multiple options

[Michael Catanzaro]

Comment on attachment 251442 [details]
[CMake] Should be possible for an option to depend on multiple options

Clearing flags on attachment: 251442

Committed r183202: <http://trac.webkit.org/changeset/183202>

[Michael Catanzaro]

All reviewed patches have been landed.  Closing bug.