[Pkg-sql-ledger-discussion] Review of ledgersmb 1.3.10-1

[Raphael Hertzog]

Hello,

here's my review of your updated package. First let's reiterate that the
long description and the copyright file needs some more work. Those are
important parts of the package and it would have been nice to see this
done before the actual upload.

On Thu, 22 Dec 2011, Raphael Hertzog wrote:
> 14/ debian/links: The symlink /etc/apache2/conf.d/ledgersmb-httpd.conf
>     is problematic. If you want to get rid of that file, your choice
>     won't be respected because it's a symlink and thus not a conffile.
>     Replace this symlink either directly with the target file (i.e. move
>     /etc/ledgersmb/ledgersmb-httpd.conf to
>     /etc/apache2/conf.d/ledgersmb-httpd.conf) or create
>     /etc/apache2/conf.d/ledgersmb-httpd.conf as a file with a single
>     "Include /etc/ledgersmb/ledgersmb-httpd.conf" directive.

This new version has a symlink /etc/ledgersmb/ledgersmb-httpd.conf
pointing to /etc/apache2/conf.d/ledgersmb-httpd.conf (which is not
installed by the package itself but by the postinst). I don't see
what this symlinks brings except extra problems (it's broken until the
package is configured and might again be broken if the admin drops
/etc/apache2/conf.d/ledgersmb-httpd.conf, which he rightfully can).

IMO you should install /etc/apache2/conf.d/ledgersmb-httpd.conf as
"conffile" managed by dpkg and be done with it.

Installing that file manually in the postinst has drawbacks because
you have to ensure that "postrm purge" also removes it, which is not
the case currently... and also the user doesn't benefit from changes in
the files even if he makes no changes compared to the original file.


The same applies to /usr/share/ledgersmb/pos.conf.pl. It should be made a
symlink (provided by the .deb itself) pointing to
/etc/ledgersmb/pos.conf.pl and you should provide a copy of the template
file as /etc/ledgersmb/pos.conf.pl.

----
Then I looked at debian/patches/ and the number of patches is a bit
ridiculous. IMO you should have merged all the similar patches together.
I don't see the upstream developers picking your patches one by one... IMO
if they want to merge fixes for those POD issues, they want all the fixes
at the same time.

Same goes for the shebang patches.

Another small detail: debian/patches/15_httpdconf.patch provides
a "ledgersmb-httpd.conf.rej" which it shouldn't.

Another weird thing to investigate: debian/patches/05_confdir.patch
doesn't modify the $templates variable yet it probably should since
you install templates in /etc/ledgersmb/templates and not in
/usr/share/ledgersmb/.

----

> 16/ dpkg-statoverride must not be used, it's a tool for the local
>     admin and not for the package maintainer. See
>     http://www.debian.org/doc/debian-policy/ch-files.html#s10.9.1
>     www-data is a statically allocated user id so you must
>     fix the ownerships at package build time and ensure that
>     the files in the .deb are directly owned by www-data.
> 
>     (you do this by adding some "chown -R" after the dh_fixperms call)

You have dropped the dpkg-statoverride but not added any chown -R
to compensate. Why?

Also I believe that you need empty /var/lib/ledgersmb/{users,spool} to be
owned by www-data. At least that was the case for sql-ledger.

Templates are also editable from the web interface AFAIK—at least they
were in sql-ledger—so they should be owned by www-data if we want the
possibility to edit them to work.

> 18/ in debian/postrm
>     dpkg-statoverride --remove /etc/ledgersmb/templates* 
>     does not do what you expect... (and you won't notice since you hide
>     all the errors and you ignore the return code)
>     (anyway this will go away since you will no longer use
>     dpkg-statoverride, you might want to move that part in postinst
>     to drop what you incorrectly set during the upgrade from an
>     old affected version)

You have kept the statoverride removal in "postrm upgrade" but this won't
work because it's the "old postrm" that is used during upgrade... and the
version passed is the new version being installed (and not the
previous version).

As I said, you should move it to the postinst.

> - The apache integration is definitely required. It's easy enough to add
>   the required a2enmod call (just ensure to call it only once during
>   initial installation, i.e. "$1" = "configure" and "$2" = ""). Make
>   it followed by an "invoke-rc.d apache2 restart || true". Protect all this
>   by a check that ensures that those commands are available.

You have put this:

        # Enable mod rewrite if it is not already enabled.
        if [ -d "/etc/apache2/mods-enabled" ]; then
          if [ ! -h "/etc/apache2/mods-enabled/rewrite.load" ]; then
            if [ -e "/usr/sbin/a2enmod" ]; then
              a2enmod rewrite
            fi
          fi
        fi

This is too many checks and also not using the usual idioms. First you
should not have to check anything in /etc/apache2, those are implementation
details of "a2enmod"... and the documentation of a2enmod says clearly
“It is not an error to enable a module which is already enabled”.

So you should only do:

    if [ -x "`which a2enmod 2>/dev/null`" ]; then
        a2enmod rewrite
    fi


To restart apache you're doing this:

        # Restart apache
        if [ -e "/etc/init.d/apache2" ]; then
          invoke-rc.d apache2 restart || true
        fi

It's also the wrong check, you want "-x" and not "-e".

BTW you should really use more than 2 characters for indentation,
4 or 8 characters are the most common indentations. 2 chars is really
too narrow.

----
Some review of debian/rules too:

	[ ! -e Build.PL ] || rm -f Build.PL

The check is useless. "rm -f Build.PL" is enough since it won't fail if the
file is not there.

	[ ! -e LedgerSMB/Assets ] || rmdir LedgerSMB/Assets
	[ ! -e LedgerSMB/Entity ] || rmdir LedgerSMB/Entity

Why not just "rm -rf LedgerSMB/Assets LedgerSMB/Entity" or "rmdir --ignore-fail-on-non-empty LedgerSMB/Assets LedgerSMB/Entity" ?

override_dh_auto_clean:
        dh_auto_clean
        rm -f ledgersmb.conf
        rm -f ledgersmb-httpd.conf

Put ledgersmb.conf & ledgersmb-httpd.conf in debian/clean and let dh_clean drop
the files for you.

----

I'm not going to upload this version of the package because of the directory
ownership issues identified (root instead of www-data). It's better to not
release something that will later require you to add code to fix the
ownership.

Cheers,
-- 
Raphaël Hertzog ◈ Debian Developer

Pre-order a copy of the Debian Administrator's Handbook and help
liberate it: http://debian-handbook.info/liberation/