GNOME Bugzilla – Bug 742195
Fix automagic dependencies
Last modified: 2017-02-08 19:13:21 UTC
Created attachment 293570 [details] [review] Patch I've patched gnome-control-center to make the bluetooth, network, online-accounts, printers and wacom panels optional via --with-panels. --with-panels=all - equivalent to --with-panels=bluetooth,network,online-accounts,printers,wacom --with-panels=a,b,c means that a,b,c are enabled (configure fails if they need libraries that are not installed) and nothing else is --with-panels=auto,a,b,c means that a,b,c are enabled and everything else is enabled if its libraries are installed The default is --with-panels=auto (just as before) Before no panels were optional because there were no options to enable/disable them. Some (network, printers, wacom and bluetooth) were enabled/disabled depending on the presence of the corresponding libraries on the build system. This is in no way predictable. See http://wiki.gentoo.org/wiki/Project:Quality_Assurance/Automagic_dependencies I'm sure the gentoo guys will love this.
Review of attachment 293570 [details] [review]: I'll repeat it for the nth time, the Bluetooth, Wacom and Network panels (amongst others) aren't optional. When building on Linux, they are required. What's the problem with the background panel? GOA and grilo aren't optional for it either.
Review of attachment 293570 [details] [review]: Correcting the status, until further discussions.
> I'll repeat it for the nth time, the Bluetooth, Wacom and Network panels > (amongst others) aren't optional. When building on Linux, they are required. Not exactly. gnome-control-center builds perfectly fine without NetworkManager installed on Linux (only the network applet and the wifi switch in the power applet are disabled). Try it. If NetworkManager and its libraries are detected, however, the network panel is built. This kind of dependencies (that depend only on the system that the build is on and are not configurable) are called automagic dependencies. See http://wiki.gentoo.org/wiki/Project:Quality_Assurance/Automagic_dependencies and https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/makefile-options.html point 5.12.2. They are bad for multiple reasons, especially on source-based distributions: - If a user builds gnome-control-center without the needed libraries installed, it will build fine, even if the user wants NetworkManager support. My patch fixes that (--with-panels=all) and configure will fail if something required is not detected. - It is not easy to build gnome-control-center without NetworkManager support on a build server with NetworkManager. My patch fixes that (--with-panels=). - On source-based distributions, it's not possible to reliably determine a package's dependencies before build as they may change depending on the set of currently installed software and don't honor the user's configuration. Dependencies being influenced by user configuration happens on Gentoo and its derivatives, as well as in FreeBSD ports. My patch fixes that (--with-panels=${whatever_is_configured}). My patch defaults to preserving the old behavior (--with-panels=auto). There are two possible solutions to this problem: - Make NetworkManager mandatory on all platforms - Make NetworkManager support configurable There are multiple solutions that do not make sense; - Make the network panel mandatory on Linux. For other platforms it may not be necessary to have NetworkManager. Therefore the code (#ifdefs and Makefile ifs) for disabling NetworkManager support must remain. Therefore it's almost as easy to make NetworkManager optional on all platforms. - Disable the network panel only on the 5-th day after a new moon. This makes about as much sense as the option above for the exact same reason. The code for disabling it must remain, therefore it's almost as easy to make it optional on all platforms. The same goes for Wacom and Bluetooth. They all should behave like the Printers panel - be optional. Please give me a reason not to do this different from "We don't like change" or "We don't understand the concept of a thing being the case in moment A not being the case in moment B". Because that's what Comment 1 sounds like. > What's the problem with the background panel? GOA and grilo aren't optional for > it either. I know that and I don't like it. I'll split it out of this patch and file a separate bug report. Oh and I moved GNOME_DEBUG_CHECK and GNOME_COMPILE_WARNINGS([maximum]) up to stop autoconf's complaints. Sorry about the double ' in AC_DEFINE. I'll remove that from the patch. Oh and I just realised I changed HAVE_NETWORK_MANAGER to HAVE_LIBNM. I'll fix that.
(In reply to comment #3) > > I'll repeat it for the nth time, the Bluetooth, Wacom and Network panels > > (amongst others) aren't optional. When building on Linux, they are required. > > Not exactly. gnome-control-center builds perfectly fine without NetworkManager > installed on Linux (only the network applet and the wifi switch in the power > applet are disabled). Try it. The intent might not be what's implemented. That doesn't change the intent. > If NetworkManager and its libraries are detected, however, the network panel is > built. This kind of dependencies (that depend only on the system that the build > is on and are not configurable) are called automagic dependencies. See > http://wiki.gentoo.org/wiki/Project:Quality_Assurance/Automagic_dependencies I handled enough Gentoo bugs to know what it means. <snip> > There are two possible solutions to this problem: > - Make NetworkManager mandatory on all platforms > - Make NetworkManager support configurable Solution 3 is make NM mandatory on Linux. > There are multiple solutions that do not make sense; > - Make the network panel mandatory on Linux. For other platforms it may not be > necessary to have NetworkManager. Therefore the code (#ifdefs and Makefile ifs) > for disabling NetworkManager support must remain. Therefore it's almost as easy > to make NetworkManager optional on all platforms. I don't care whether it's "easy". I'm not interested in shipping a bag of bits. > - Disable the network panel only on the 5-th day after a new moon. This makes > about as much sense as the option above for the exact same reason. The code for > disabling it must remain, therefore it's almost as easy to make it optional on > all platforms. > > The same goes for Wacom and Bluetooth. They all should behave like the Printers > panel - be optional. The different panels are optional for different reasons. The bottom line is that, eventually, all the panels will be forced-turned on on all the platforms where they are supported. > Please give me a reason not to do this different from "We don't like change" or > "We don't understand the concept of a thing being the case in moment A not > being the case in moment B". Because that's what Comment 1 sounds like. It's like comment 1 in that we don't want to support gnome-control-center becoming a bag of bits. All the panels supported on specific platforms must be builtin. That's not the case for some of them right now, because of historical reasons, but that's likely to change. > > What's the problem with the background panel? GOA and grilo aren't optional for > > it either. > > I know that and I don't like it. I'll split it out of this patch and file a > separate bug report. If it's to make that code optional, don't bother. I'm not interested in bug reports where they can't find x or y feature because the packager missed it, or didn't think it was important, or even because they compiled from source themselves. > Oh and I moved GNOME_DEBUG_CHECK and GNOME_COMPILE_WARNINGS([maximum]) up to > stop autoconf's complaints. Sorry about the double ' in AC_DEFINE. I'll remove > that from the patch. > > Oh and I just realised I changed HAVE_NETWORK_MANAGER to HAVE_LIBNM. I'll fix > that.
I do not want NetworkManager on my PC.
(In reply to comment #5) > I do not want NetworkManager on my PC. And you don't need to, you just need the NetworkManager libraries. The code handles NM not being available at run-time. We don't want to support building without the network panel though.
I'll work on that later, too (not that easy on Gentoo). If I revise my patch to default to --with-panels=supported, which enables all panels supported on the platform, and fails if a library is not detected (it will take a few minutes to implement + a list of platforms), will it be acceptable? I doubt a user is dumb enough to explicitly disable a panel and complain it is not available.
(In reply to comment #7) > I'll work on that later, too (not that easy on Gentoo). > > If I revise my patch to default to --with-panels=supported, which enables all > panels supported on the platform, and fails if a library is not detected (it > will take a few minutes to implement + a list of platforms), will it be > acceptable? I doubt a user is dumb enough to explicitly disable a panel and > complain it is not available. It shouldn't require any configure flags. NM and MM libraries will be required on Linux systems, Bluetooth panel should fail if the gnome-bluetooth library is missing on Linux systems, ditto the Printers panel if CUPS is missing. All those similarly to the Wacom panel which already does this. If you're going to make those changes, please separate the force-enablement of each of the panels into different patches.
I didn't plan to implement it exactly this way. NM and MM will be required UNLESS THE USER SPECIFIES --with-panels=something that does not include network panel The default WILL require NM and MM.
Created attachment 294035 [details] [review] Patch, r2 This should be more acceptable. The default is --with-panels=supported, which expands to the list of panels supported on CHOST.
Bump.
(In reply to comment #11) > Bump. Don't do that. This isn't a forum.
Review of attachment 294035 [details] [review]: ::: configure.ac @@ +154,3 @@ PKG_CHECK_MODULES(NOTIFICATIONS_PANEL, $COMMON_MODULES) +PKG_CHECK_MODULES(ONLINE_ACCOUNTS_PANEL, $COMMON_MODULES + goa-1.0 goa-backend-1.0 >= $GOA_REQUIRED_VERSION) That change is unnecessary as far as I can see. @@ +163,3 @@ libsoup-2.4 gnome-desktop-3.0 >= $GNOME_DESKTOP_REQUIRED_VERSION) + Neither is that linefeed addition. @@ +240,3 @@ + ;; +esac +with_panels="${with_panels/,supported,/,${supported_panels}}" You don't check at any point whether the values passed for --with-panels is valid. I just tried to run configure with --with-panels=foobar, and ended up with a Linux build that would have compiled without NetworkManager, Bluetooth, Wacom or Printers support. This is obviously not something I want to be possible. @@ +257,3 @@ + fi + if test "$tp_force" && ! test "$tp_can"; then + AC_MSG_ERROR([Dependencies not satisfied for $1 panel]) I'm afraid that this isn't quite good enough. Before, we would get a warning with a list of missing dependencies when, say, we tried to build on Linux without the gnome-bluetooth libraries installed. ::: panels/power/Makefile.am @@ -24,3 @@ -if BUILD_BLUETOOTH -AM_CPPFLAGS += $(BLUETOOTH_CFLAGS) This should be removed in a separate fix.
I'll fix the error messages and I'll probably rework the patch so that every panel has a separate --enable/--disable option: --enable-foobar-panel=supported (default) # configure fail if dependencies are missing and panel is supported on OS --enable-foobar-panel=auto # don't build if dependencies are missing --enable-foobar-panel # configure fail if dependencies are missing --disable-foobar-panel # don't build, no matter OS/CPU/libraries installed Is that OK?
(In reply to comment #14) > I'll fix the error messages and I'll probably rework the patch so that every > panel has a separate --enable/--disable option: > > --enable-foobar-panel=supported (default) # configure fail if dependencies are > missing and panel is supported on OS > --enable-foobar-panel=auto # don't build if dependencies are missing > --enable-foobar-panel # configure fail if dependencies are missing > --disable-foobar-panel # don't build, no matter OS/CPU/libraries installed No. I said it in comments 1, 4, 6, 8 and 13. On (non-s390) Linux, Bluetooth, Wacom, NetworkManager, CUPS are requirements. They are not optional. I will not take patches that allow compiling gnome-control-center without those panels on Linux.
> No. I said it in comments 1, 4, 6, 8 and 13. On (non-s390) Linux, Bluetooth, > Wacom, NetworkManager, CUPS are requirements. They are not optional. I will not > take patches that allow compiling gnome-control-center without those panels on > Linux. That'll be easy: if [ "$enable_network_panel" != supported ] && \ [ "$enable_network_panel" != no ]; then AC_MSG_ERROR([I won't let you do that]) fi
Sorry, I meant [ "$enable_network_panel" != yes ] instead of [ "$enable_network_panel" != no ]
Created attachment 298777 [details] [review] Patch, r3 This time using a macro. The macro combines most boilerplate around optional panel definitions: - pkg-config - other checks can be done - AC_DEFINE and AM_CONDITIONAL - Show configuration options at the end of ./configure run. - It allows some panels to be required on some platforms.
Comment on attachment 298777 [details] [review] Patch, r3 Doesn't apply. +AC_DEFUN([CC_PANEL], [ + m4_pushdef([CC_PANEL_LOWER], m4_bpatsubst([$1], [[^a-zA-Z0-9_]], [_])) + m4_pushdef([CC_PANEL_UPPER], m4_toupper(m4_bpatsubst([$1], [[^a-zA-Z0-9_]], [_]))) I also refuse to take in m4 macros that ultimately mean that I need to maintain more code to support a use case I don't want to support.
I've simplified the checks so that we error as soon as possible if any panel is missing dependencies on Linux. There are still a number of "automagic" dependencies, but that'll be for another time.