Ticket #391 (closed enhancement: fixed)

Opened 6 years ago

Last modified 5 years ago

Add an option to choose whether to install mixer or not

Reported by: lilrc Assigned to:
Priority: major Milestone:
Component: generic Version: FFADO SVN (trunk)
Keywords: build system, patch Cc:
The device the bug applies to:

Description

Hello developers!

I found that the build system detects the prerequisites (PyQt, pyuic4, dbus, dbus.mainloop.qt) for the ffado mixer and automagically depends on them if they are present. This is bad since it intoduces an automagic dependency that makes packaging for source based distributions (such as Gentoo) problematic. There is a write-up (1) on the Gentoo Wiki that deals with this subject a little deeper. Please see that page for a deeper discussion on the topic (and why automagic dependencies are bad).

One can solve this (downstream) by doing one of the following:

  1. Unconditionally depend on the automagic dependencies, which in this forces all users to install the mixer.
  2. Patch the build system.

I decided to go with the latter one. I have written a big patch that creates a BUILD_MIXER option to the SConstruct script. The option can be set to one of 'auto', 'true' or 'false'. If 'true' is given the build system tries to find the dependencies and build the mixer. If any of the dependencies is not found in this case the build system exits with an error. If 'false' is given instead the build system disables all checks for mixer dependencies. The special value 'auto', which is the default, looks for the dependencies and if they are found the mixer is built, else the mixer is not built. This way packagers (and users too for that matter) can disable the installation of the mixer even on a system with all dependencies, which is the intention.

Note that the default 'auto' does not alter the current default behaviour in any way. (The only change my patch introduces is the formulation of the warning/error message.) It should thus be user-friendly as well as packager-friendly to apply.

Anyway, a few technical details: the changes I made to the build system can be summarized in the following points.

  1. The BUILD_MIXER option has been added.
  2. env['PYUIC4'] and build_mixer have been removed as env['BUILD_MIXER'] gives the same information.
  3. User messages have been revised somewhat (feel free to change those if you want).
  4. XDG tools are only checked if the mixer is to be built.
  5. Both xdg-desktop-menu and xdg-icon-resource are now checked whether they exist (instead of just the testing the first).
  6. env['XDG_TOOLS'] is always declared to either False or True and all consumers of 'XDG_TOOLS' have been changed to use if env[...] instead of if env.has_key(...).

Anyway, here is the patch:

Index: SConstruct
===================================================================
--- SConstruct	(revision 2587)
+++ SConstruct	(working copy)
@@ -68,6 +68,7 @@
   this code.""", False ),
     BoolVariable( "ENABLE_ALL", "Enable/Disable support for all devices.", False ),
     BoolVariable( "SERIALIZE_USE_EXPAT", "Use libexpat for XML serialization.", False ),
+    EnumVariable( "BUILD_MIXER", "Build the ffado-mixer", 'auto', allowed_values=('auto', 'true', 'false'), ignorecase=2),
     BoolVariable( "BUILD_TESTS", """\
 Build the tests in their directory. As some contain quite some functionality,
   this is on by default.
@@ -379,28 +380,33 @@
 #
 
 # PyQT checks
-build_mixer = False
-if conf.CheckForApp( 'which pyuic4' ) and conf.CheckForPyModule( 'dbus' ) and conf.CheckForPyModule( 'PyQt4' ) and conf.CheckForPyModule( 'dbus.mainloop.qt' ):
-    env['PYUIC4'] = True
-    build_mixer = True
+if env['BUILD_MIXER'] != 'false':
+    if conf.CheckForApp( 'which pyuic4' ) and conf.CheckForPyModule( 'dbus' ) and conf.CheckForPyModule( 'PyQt4' ) and conf.CheckForPyModule( 'dbus.mainloop.qt' ):
+        env['BUILD_MIXER'] = 'true'
+    elif not env.GetOption('clean'):
+        if env['BUILD_MIXER'] == 'auto':
+            env['BUILD_MIXER'] = 'false'
+            print """
+The prerequisites ('pyuic4' and the python-modules 'dbus' and 'PyQt4', the
+packages could be named like dbus-python and PyQt) to build the mixer were not
+found. Therefore the qt4 mixer will not be installed."""
+        else: # env['BUILD_MIXER'] == 'true'
+            print """
+The prerequisites ('pyuic4' and the python-modules 'dbus' and 'PyQt4', the
+packages could be named like dbus-python and PyQt) to build the mixer were not
+found, but BUILD_MIXER was requested."""
+            Exit( 1 )
 
-if conf.CheckForApp( 'xdg-desktop-menu --help' ):
-    env['XDG_TOOLS'] = True
-else:
-    print """
-I couldn't find the program 'xdg-desktop-menu'. Together with xdg-icon-resource
-this is needed to add the fancy entry to your menu. But if the mixer will be
-installed, you can start it by executing "ffado-mixer".
-"""
+env['XDG_TOOLS'] = False
+if env['BUILD_MIXER'] == 'true':
+    if conf.CheckForApp( 'xdg-desktop-menu --help' ) and conf.CheckForApp( 'xdg-icon-resource --help' ):
+        env['XDG_TOOLS'] = True
+    else:
+        print """
+I couldn't find the 'xdg-desktop-menu' and 'xdg-icon-resource' programs. These
+are needed to add the fancy entry for the mixer to your menu, but you can still
+start it by executing "ffado-mixer"."""
 
-if not build_mixer and not env.GetOption('clean'):
-    print """
-I couldn't find all the prerequisites ('pyuic4' and the python-modules 'dbus'
-and 'PyQt4', the packages could be named like dbus-python and PyQt) to build the
-mixer.
-Therefor the qt4 mixer will not get installed.
-"""
-
 #
 # Optional pkg-config
 #
@@ -510,7 +516,7 @@
 env.Alias( "install", env['sharedir'] )
 env.Alias( "install", env['bindir'] )
 env.Alias( "install", env['mandir'] )
-if build_mixer:
+if env['BUILD_MIXER'] == 'true':
     env.Alias( "install", env['pypkgdir'] )
 
 #
@@ -909,7 +915,7 @@
         if env.GetOption( "clean" ):
             env.Execute( action )
 
-    if env.has_key( 'XDG_TOOLS' ) and env.has_key( 'PYUIC4' ):
+    if env['BUILD_MIXER'] == 'true' and env['XDG_TOOLS']:
         if not env.GetOption("clean"):
             action = "install"
         else:
Index: support/mixer-qt4/SConscript
===================================================================
--- support/mixer-qt4/SConscript	(revision 2587)
+++ support/mixer-qt4/SConscript	(working copy)
@@ -26,7 +26,7 @@
 
 Import( 'env' )
 
-if env.has_key('PYUIC4'):
+if env['BUILD_MIXER'] == 'true':
     e = env.Clone()
 
     def findfiles( arg, dirname, names ):

Please review this patch and let me know what you think! Please also consider applying the patch here upstream to reduce maintenance burden downstream! I am open to any ideas or suggestions!

(1) https://wiki.gentoo.org/wiki/Project:Quality_Assurance/Automagic_dependencies

Thanks in advance,
Karl

Change History

04/09/15 10:05:24 changed by lilrc

Ping?

04/09/15 16:04:48 changed by jwoithe

Have had a busy couple of weeks. I have looked at the patch and don't have any fundamental objections. I'm just deciding whether the naming convention is what we want. I am hoping to get back onto this soon.

04/12/15 04:25:57 changed by jwoithe

  • status changed from new to closed.
  • resolution set to fixed.

The supplied patch has been merged in r2588. Having read the article via the quoted link I am personally still in two minds about the views on so-called automagic dependencies - in some ways it's more idealogical than technical. However, in the specific case of FFADO's build system there's no technical reason not to apply the patch, and its application will make things easier for you (as the gentoo package maintainer) and your users. Thanks for the patch.

(follow-up: ↓ 5 ) 04/12/15 05:02:44 changed by lilrc

Thank you! I am very interested in your ideological view on automagic dependenies!

(in reply to: ↑ 4 ) 04/12/15 16:32:44 changed by jwoithe

Replying to lilrc:

I am very interested in your ideological view on automagic dependenies!

I should clarify that it was the article - not my personal views - which seemed to be ideological. However, it's just a gut feeling at this point having read only the linked article. In any case, it doesn't affect the matter at hand and is out of scope for this ticket. When problems are identified which stem from different (but still technically valid) assumptions (such as the one you reported), my ultimate interest is in finding workable solutions.