[Pkg-dkms-maint] RFS: dkms

[David Paleino]

On Tue, 23 Dec 2008 23:20:53 +0000, Ben Hutchings wrote:

> Here's the overdue review of the dkms script itself.  I'll quote and
> comment on the key functions for Debian integration:
> 
> > function remake_initrd()
> > {
> >     [..]
> >     # Support initramfs distributions (Ubuntu).
> >     if [ -x "/usr/sbin/mkinitramfs" ]; then
> >         mkinitrd='mkinitramfs'
> >     fi
> 
> Under Debian, this should read /etc/kernel-img.conf and search through
> the list assigned to ramdisk, or "mkinitrd mkinitrd.yaird mkinitramfs"
> by default.

Done.


> >     [..]
> > 
> >     # Rerun lilo if necessary
> >     if ! [ -e /boot/grub/grub.conf ] && [ -e /etc/lilo.conf ] && !
> > [ -e /boot/grub/menu.lst ]; then invoke_command "/sbin/lilo" "Updating lilo"
> >     fi
> 
> Just because there's a GRUB configuration present doesn't mean LILO
> isn't installed.  This should check /etc/kernel-img.conf to find out
> what to do.

Fixed as well.

> > function distro_version()
> > {
> > # What distribution are we running?
> >     [..]
> >     if [ -r /etc/lsb-release ]; then
> > 	. /etc/lsb-release
> > 	LSB_RELEASE=1
> >     fi
> 
> This is wrong; /etc/lsb-release should only be interpreted by
> lsb_release and may not be present (it isn't in Debian).  dkms should
> run lsb_release -i -r to find the distributor name and release.

Fixed.

> > function override_dest_module_location()
> > {
> >     [..]
> >     case "$running_distribution" in
> > 	[..]
> > 	Ubuntu*) echo "/updates/dkms" && return ;;
> 
> This should handle Debian* like Ubuntu*.  That depends on fixing
> distro_version() first.

Done.

> > function make_debian()
> > {
> >     [..]
> >     debian_package=$(echo $module | sed s/_/-/)
> 
> The s/// needs a g modifier to replace all underscores.

Fixed.

> >     [..]
> >     if [ "$USER" != "root" ]; then
> >         if [ -x /usr/bin/gksudo ] && [ ! -z "$DISPLAY" ]; then
> >             ROOT="/usr/bin/gksudo --description 'DKMS Debian package
> > builder' " elif [ -x /usr/bin/kdesu ] && [ ! -z "$DISPLAY" ]; then
> >             ROOT="/usr/bin/kdesu"
> >         elif [ -x /usr/bin/sudo ]; then
> >             ROOT="/usr/bin/sudo"
> >         fi
> 
> This should look for su-to-root (from the menu package) first.

Fixed.

> >         if groups | sed 's/lpadmin//' | grep admin 2>&1 1>/dev/null; then
> >             ADMINABLE="TRUE"
> >         fi
> 
> This is absolutely not a correct way to grep for exactly the word
> "admin" (what do they think the -w option is for?!), but in any case
> this is not a standard group in Debian and has no special status in
> being able to run e.g. sudo.  The test should be only that $ROOT is
> non-null. 

Done.

> >     [..]
> >     #test if we are missing dependencies that are needed during package
> > build INSTALL_PACKAGES="`make_debian_test_depends`"
> >     if [ ! -z "$INSTALL_PACKAGES" ]; then
> >         if [ -z "ADMINABLE" ]; then
> 
> This is testing the string ADMINABLE, not the variable value
> $ADMINABLE. 

Fixed.

> >             echo $"" >&2
> >             echo $"Error! Missing $INSTALL_PACKAGES" >&2
> >             echo $"and unable to install.  Please ask an admin to install
> > for you." >&2 exit 4
> >         fi
> >         if [ ! -z "$SYNAPTIC" ] && [ ! -z "$DISPLAY" ]; then
> >             local TEMPFILE=`/bin/tempfile`
> >             echo $INSTALL_PACKAGES | sed 's/|/\ install\
> > /g' > $TEMPFILE
> >             $ROOT "sh -c '/usr/sbin/synaptic --set-selections
> > --non-interactive --hide-main-window < $TEMPFILE'" trap 'rm -f $TEMPFILE'
> > EXIT HUP TERM else
> >             $ROOT apt-get -y install $INSTALL_PACKAGES
> >         fi
> > 
> >         INSTALL_PACKAGES="`make_debian_test_depends`"
> >         #Retest packages
> >         if [ ! -z "$INSTALL_PACKAGES" ]; then
> >             echo $"" >&2
> >             echo $"Error! Missing $INSTALL_PACKAGES" >&2
> >             echo $"and unable to install.  Please ask an admin to install
> > for you." >&2 exit 4
> >         fi
> >     fi
> 
> All of the above installation crap should be dead code in the package,
> anyway.  But you do need to add dpkg-dev to the package dependencies. 

Added as Recommends, since it's not always needed.

> >     [..]
> >     case "$create_type" in
> >         dsc)
> >             invoke_command "fakeroot dpkg-buildpackage -S -us -uc
> > 1>/dev/null" "Building source package"
> 
> Drop the fakeroot; it's not needed.

Done.

> >             invoke_command "mv
> > '$temp_dir/${debian_package}-dkms_${module_version}.dsc'
> 
> Replace the fakeroot with the -rfakeroot option and dpkg-buildpackage
> will use it only as necessary.  (Current dpkg-buildpackage doesn't even
> need the option but upstream will presumably want to maintain
> backward-compatibility.) 

Done.

> >             invoke_command "mv
> > '$temp_dir/${debian_package}-dkms_${module_version}_all.deb' '$deb_basedir'"
> > "Moving built files to $deb_basedir" ;; esac
> > 
> >     #cleanup
> >     invoke_command "rm $temp_dir -fr" "Cleaning up temporary files"
> > 
> >     #done
> >     if [ "$?" -eq 0 ]; then
> >         echo $""
> >         echo $"DKMS: mk${create_type} Completed."
> 
> Unfortunately this just tests that the rm -rf was successful.  The
> preceding build steps appear to be unchecked. 

I suppose this would need some upstream/major patching.

You can find the dsc here:

<http://mentors.debian.net/debian/pool/main/d/dkms/dkms_2.0.20.4-1.dsc>

Please, keep the list CCed in your reply.

Thank you,
David

-- 
 . ''`.  Debian maintainer | http://wiki.debian.org/DavidPaleino
 : :'  : Linuxer #334216 --|-- http://www.hanskalabs.net/
 `. `'`  GPG: 1392B174 ----|---- http://snipr.com/qa_page
   `-   2BAB C625 4E66 E7B8 450A C3E1 E6AA 9017 1392 B174
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
Url : http://lists.alioth.debian.org/pipermail/pkg-dkms-maint/attachments/20081226/4953ddc3/attachment.pgp