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.
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
This seems like a good idea to me.
Created attachment 250402 [details] Patch
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.
(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.
(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.
(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?
(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.
(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!
(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.
(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.
Created attachment 251859 [details] Patch
Comment on attachment 251859 [details] Patch Nope I missed libnotify
Created attachment 251875 [details] Patch
Created attachment 251876 [details] Patch
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.
Created attachment 251878 [details] Patch
Comment on attachment 251878 [details] Patch Clearing flags on attachment: 251878 Committed r183505: <http://trac.webkit.org/changeset/183505>
All reviewed patches have been landed. Closing bug.
(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.
(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!