Bug 143546 – [GTK] Clean up feature detection and make it hard to accidentally build without optional features

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

gusrub on IRC noticed that openSUSE accidentally built WebKitGTK+ without support for HTML5 notifications. We did the same in Fedora. This needs to be harder to get wrong; let's hard-fail unless the user builds with -DENABLE_NOTIFICATIONS=OFF.

[Michael Catanzaro]

Let's do the same for geoclue, gnutls, and libsecret, since these are all things that distros really ought to ship: we don't want users to get crippled versions of our software unless distros are actually making an active decision to disable the feature.

We will need to update https://trac.webkit.org/wiki/WebKitGTK/Dependencies

[Martin Robinson]

This seems like a good idea to me.

[Michael Catanzaro]

Created attachment 250402 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 250402 [details]
Patch

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

I like the idea but I see a problem. What happens if configure fails because an optional dependency was not found? I guess you get an error that libfoo was not found and it's required. How does the user know this is an optional dependency and how to build without it? Is it possible to add a message with all that information when the build fails due to a missing optional dependency?

> Source/cmake/OptionsGTK.cmake:310
> +if (ENABLE_NOTIFICATIONS)
> +    find_package(LibNotify REQUIRED)
> +    add_definitions(-DWTF_USE_LIBNOTIFY=1)
> +else ()
> +    add_definitions(-DWTF_USE_LIBNOTIFY=0)
> +endif ()

I don't think this is correct in this particular case. ENABLE_NOTIFICATIONS should never be disabled, because it's exposed in the public API. Libnotify is only used as default implementation of the notifications, you should be able to build with ENABLE_NOTIFICATIONS and without libnotify, because the API allows you to provide your own implementation.

[Carlos Garcia Campos]

(In reply to comment #0)
> gusrub on IRC noticed that openSUSE accidentally built WebKitGTK+ without
> support for HTML5 notifications. We did the same in Fedora. This needs to be
> harder to get wrong; let's hard-fail unless the user builds with
> -DENABLE_NOTIFICATIONS=OFF.

ENABLE_NOTIFICATIONS is ON unconditionally, if openSUSE is building with DENABLE_NOTIFICATIONS=OFF is because they explicitly added that option in command line. There's another issue here, all options are public, and I don't know how to avoid that in cmake, but some of the options (and ENABLE_NOTIFICATIONS is one of them) should not be exposed to the user.

[Martin Robinson]

(In reply to comment #5)
 
> ENABLE_NOTIFICATIONS is ON unconditionally, if openSUSE is building with
> DENABLE_NOTIFICATIONS=OFF is because they explicitly added that option in
> command line. There's another issue here, all options are public, and I
> don't know how to avoid that in cmake, but some of the options (and
> ENABLE_NOTIFICATIONS is one of them) should not be exposed to the user.

CMake has the concept of "advanced" options, which are hidden unless you explicitly show them. Things that we do not want people to turn on and off should at least be marked as advanced. You can see this in action by searching for 'mark_as_advanced' in Source/cmake.

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #5)
>  
> > ENABLE_NOTIFICATIONS is ON unconditionally, if openSUSE is building with
> > DENABLE_NOTIFICATIONS=OFF is because they explicitly added that option in
> > command line. There's another issue here, all options are public, and I
> > don't know how to avoid that in cmake, but some of the options (and
> > ENABLE_NOTIFICATIONS is one of them) should not be exposed to the user.
> 
> CMake has the concept of "advanced" options, which are hidden unless you
> explicitly show them. Things that we do not want people to turn on and off
> should at least be marked as advanced. You can see this in action by
> searching for 'mark_as_advanced' in Source/cmake.

Hidden means you could still use them if you know they actually exist?

[Martin Robinson]

(In reply to comment #7)
 
> Hidden means you could still use them if you know they actually exist?

That's correct, though I imagine very few people would try to modify the advanced options. We can likely also find a way to make them impossible to change.

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #7)
>  
> > Hidden means you could still use them if you know they actually exist?
> 
> That's correct, though I imagine very few people would try to modify the
> advanced options. We can likely also find a way to make them impossible to
> change.

Nah, I guess hiding them is enough so that if people keep using them is at their own risk. See https://bugs.webkit.org/show_bug.cgi?id=143558.

Thanks!

[Michael Catanzaro]

(In reply to comment #5) 
> ENABLE_NOTIFICATIONS is ON unconditionally, if openSUSE is building with
> DENABLE_NOTIFICATIONS=OFF is because they explicitly added that option in
> command line.

No, they just build without libnotify in the buildroot (i.e. they do exactly what we would expect them to do) which is enough for the feature to be silently disabled.

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #5) 
> > ENABLE_NOTIFICATIONS is ON unconditionally, if openSUSE is building with
> > DENABLE_NOTIFICATIONS=OFF is because they explicitly added that option in
> > command line.
> 
> No, they just build without libnotify in the buildroot (i.e. they do exactly
> what we would expect them to do) which is enough for the feature to be
> silently disabled.

Then the feature is not disabled, but the default implementation.

[Michael Catanzaro]

Created attachment 251859 [details]
Patch

[Michael Catanzaro]

Comment on attachment 251859 [details]
Patch

Nope I missed libnotify

[Michael Catanzaro]

Created attachment 251875 [details]
Patch

[Michael Catanzaro]

Created attachment 251876 [details]
Patch

[Martin Robinson]

Comment on attachment 251876 [details]
Patch

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

Very nice. Still a couple small issues:

> Source/cmake/OptionsGTK.cmake:129
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_SUBTLE_CRYPTO PUBLIC OFF)

This should probably be private until the feature is complete enough to ship.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/CMakeLists.txt:120
> +find_package(ATSPI 2.5.3)

I'm more comfortable with this in OptionsGTK.cmake.

[Michael Catanzaro]

Created attachment 251878 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 251878 [details]
Patch

Clearing flags on attachment: 251878

Committed r183505: <http://trac.webkit.org/changeset/183505>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #18)
> Comment on attachment 251878 [details]
> Patch
> 
> Clearing flags on attachment: 251878
> 
> Committed r183505: <http://trac.webkit.org/changeset/183505>

It broke the build on the GTK buildbots and EWS too.

[Martin Robinson]

(In reply to comment #20)
> (In reply to comment #18)
> > Comment on attachment 251878 [details]
> > Patch
> > 
> > Clearing flags on attachment: 251878
> > 
> > Committed r183505: <http://trac.webkit.org/changeset/183505>
> 
> It broke the build on the GTK buildbots and EWS too.

The bot was missing libnotify, so it looks like this patch is already benefiting us by catching hidden problems!