Session file left after uninstall breaks X

Bug #136962 reported by Chris Halse Rogers
4
Affects Status Importance Assigned to Milestone
xserver-xgl (Ubuntu)
Fix Released
Undecided
Chris Halse Rogers

Bug Description

Binary package hint: xserver-xgl

xserver-xgl leaves behind /etc/X11/Xsession.d/00-xserver-xgl_start-server on remove. However this script assumes Xgl is installed, and will break if Xgl is not available.

The xserver-xgl package should remove this on package removal, and the script should be made more robust.

Changed in xserver-xgl:
assignee: nobody → raof
status: New → In Progress
Revision history for this message
Chris Halse Rogers (raof) wrote :

Here's a debdiff reworking the Xsession.d configuration stuff. Since it appears that fglrx, at least, wants to put some setup before anything else in Xsession.d, we add Xgl-session to the top of the STARTUP stack. It also adds an upgrade-notifier item, to inform users that we now start xgl automatically, and a per-user killswitch to disable the automatic startup of Xgl.

Revision history for this message
Mario Limonciello (superm1) wrote :

Okay, in looking this over, I see a few things that you will want to improve upon.

1) If your shipping an update notifier notification that is installed to $unud unconditionally, you need to depend upon update-notifier | adept-notifier. If you don't want to depend on update-notifier, check if that directory exists, and then check if the file exists only if the directory exists.
2) In your postinst you need to define $unud in that postinst.
3) This is a matter of preference, but In your update-notifier warning, I think you should let people know about the config option to turn on and off Xgl.
4) Should Xgl-session really be in everyone's default $PATH? Since its not intended to be regularly executed by a user (but rather by the session), perhaps it makes more sense to be put in a /usr/share/xserver-xgl or something to that effect? I recall a few apps that did that (at least some time back) being vino's server, and gnome-settings-daemon.

Revision history for this message
Chris Halse Rogers (raof) wrote :

Here's an updated debdiff:

Review points:
1) postinst now checks for the existence of $unud, and does nothing if it doesn't exist. It didn't seem important enough to add a dependency on update-notifier | adept-notifier
2) $unud is actually defined in postinst (stupid ctrl-k!)
3) It seems the consensus of #ubuntu-motu is that the notifications should be for high-priority notifications only.
4) Although it's possible someone will want to call Xgl-session themselves, it's unlikely. It's been moved to /usr/share/xserver-xgl, as suggested.

Revision history for this message
Chris Halse Rogers (raof) wrote :

Additionally, the debdiff adds -nolisten tcp and +xinerama to the xgl options: The first because Xgl listens on TCP by default, and we don't want that, the second because it should allow people to use the Xinerama hints for multi-head, and it doesn't break anything on single-head configurations.

Revision history for this message
Mario Limonciello (superm1) wrote :

I haven't actually applied/built/tested on my machine, but I see one more thing that sticks out to possibly cause trouble.

--- xserver-xgl-1.1.99.1~git20070727.orig/debian/xserver-xgl.preinst
+++ xserver-xgl-1.1.99.1~git20070727/debian/xserver-xgl.preinst
@@ -0,0 +1,17 @@
+#!/bin/sh -e
+
+OLD_CONF=/etc/X11/Xsession.d/00xserver-xgl_start-server
+OLD_CONF_MD5SUM=24a01acf61425d5cebec53ab07b57a90
+
+if [ "$1" = "upgrade" -o "$1" = "install" ] && \
+ [ "$2" = "1:1.1.99.1~git20070727-0ubuntu1" ] ; then
+ if [ -f $OLD_CONF ] ; then
+ if [ "$(md5sum $OLD_CONF | cut -f 1 -d ' ')" = "$OLD_CONF_MD5SUM" ] ;
+ then
+ # The old file has been left unchanged, we can delete it
+ rm -f $OLD_CONF
+ fi
+ fi
+fi
+

Hardcoding a version number and md5sum in the preinst seems like a bad idea. What happens when someone else on MOTU uploads a bug fix and doesn't see this hardcoded version and forgets to bump it?

Revision history for this message
Chris Halse Rogers (raof) wrote :

This is a once-off preinst - in this version we change from 00xserver-xgl_start-server to 98xserver-xgl_start-server. Since these are both conf files, dpkg will not automatically remove the old 00... file. If the user has not altered this file we're happy to remove it. If the user has modified this file, we leave it - our new 98... file will give a warning, but won't break in this situation.

This action is only necessary when upgrading from 1:1.1.99.1~git20070727-0ubuntu1, since this file only exists in that version. I'll add this as a comment to preinst, and upload.

Revision history for this message
Chris Halse Rogers (raof) wrote :

xserver-xgl (1:1.1.99.1~git20070727-0ubuntu2) gutsy; urgency=low

  * debian/xserver-xgl-notification.update-notifier
    + Inform users that we start Xgl automatically, so previously set up Xgl
      sessions may not work correctly.
  * Remove old Xsession.d file on install, if it hasn't been modified
    (LP: #136962). New Xsession.d file won't fall over after xserver-xgl is
    uninstalled.
  * debian/00-xserver-xgl_start-server
    + Split into a wrapper script debian/Xgl-session, and install this into
      /usr/bin and a much smaller debian/98-xserver-xgl_start-server file.
    + Install Xsession file to 98-xserver-xgl_start-server, so that it's at
      the top of the $STARTUP stack instead of starting Xgl first in the
      Xsession.d process. Fixes startup for those who manually installed
      the fglrx drivers (and possibly others).
  * Add a killswitch to disable Xgl on a per-user basis. If the file
    ~/.config/xserver-xgl/disable exists, Xgl will not be automatically
    started. Documented in README.Debian. (LP: #136878)

 -- Christopher James Halse Rogers (RAOF) <email address hidden> Tue, 04 Sep 2007 12:57:34 +1000

Changed in xserver-xgl:
status: In Progress → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.