[Pkg-sql-ledger-discussion] Uploads?

Raphael Hertzog hertzog at debian.org
Thu Dec 22 10:43:01 UTC 2011 

Hi,

On Thu, 15 Dec 2011, Robert James Clay wrote:
> On Mon, 2011-12-12 at 01:54 -0500, Raphael Hertzog wrote:
> > Hand me a .dsc of your package to be uploaded and I'll review it.
> 
>     Here's the first one:
> http://mentors.debian.net/debian/pool/main/l/ledgersmb/ledgersmb_1.3.8-1.dsc

Ok, let's start with the review. I'll dump comments in no particular
order, just as I stumble upon them. Please note that some of them
are caught by lintian if you use its -I option. That's why I put
this in ~/.devscripts:
DEBUILD_LINTIAN_OPTS="--color always -I"

Now when you run debuild, you'll get all those useful lintian hints.


1/ Please switch to debhelper compat level 8 and use "dh" to simplify
   debian/rules. That way you ensure that all importants dh_* scripts
   are run in the right order. Any change to the normal build process
   must happen throug "overrides". "man dh" for explanations if you don't
   understand what I'm referring to.

[ Reviewing debian/control ]

2/ The package is arch: all so you don't need ${shlibs:Depends} in the
   Depends field. Drop it.

3/ On the opposite you have lots of perl code installed, so adding
   ${perl:Depends} is a good idea (it's generated by dh_perl, currently
   you don't call it but if you switch to "dh", it will be automatically
   called).

4/ Please take the few minutes required to update Standards-Version to
   3.9.2. Use /usr/share/doc/debian-policy/upgrading-checklist.txt.gz to
   verify what needs to be updated (probably nothing).

5/ Replace the "mail-transport-agent" Recommends by
   "default-mta | mail-transport-agent". That way if no MTA is intalled,
   APT will pick the default one (exim) instead of a random one.

6/ Why is there a recommends on "lpr" ? I expect most printing to happen
   on the client side not on the server side. So it can probably be moved
   to Suggests.

7/ Unstable and Wheezy have postgresql-9.1. Drop references to
   "postgresql-8.3 | postgresql-8.4" and replace it by "postgresql"
   alone so that we always have the latest version of postgresql.
   Do the same for "postgresql-client-8.3 | postgresql-client-8.4",
   replace it with "postgresql-client".

8/ The long description sucks. Drop the sentence "It has been tested
   with...", it brings nothing. Say "PostgreSQL database server" because
   that's the only supported server in truth. Expand the abreviations
   AR / AP / GL. Try to further improve the description while
   following the advice of the developers-reference:
   http://www.debian.org/doc/manuals/developers-reference/best-pkging-practices.html#bpp-desc-basics

[ Review of the content of the debian/ directory ]

9/ Please be consistent and always prefix the filenames that are
   binary-package specific with the package name:
   dirs -> ledgersmb.dirs
   docs -> ledgersmb.docs
   links -> ledgersmb.links
   postinst -> ledgersmb.postinst
   postrm -> ledgersmb.postrm

10/ debian/watch: drop the useless comments. that's lintian's
    I: ledgersmb source: debian-watch-contains-dh_make-template

11/ Typo in debian/changelog caught by Lintian:
    W: ledgersmb: spelling-error-in-changelog neccessary necessary

12/ Please redo debian/copyright from scratch and follow DEP-5.
    http://dep.debian.net/deps/dep5/ 
    You don't have to list all the copyright holders, that's useless
    hard work.
    In theory this rewrite is not required but I prefer to do it because
    the resulting file is more readable and makes it easiers for the
    Debian ftpmasters to review the package (which they will have to do
    on the first upload).

13/ Drop debian/dirs. It's usually not needed. dh_install (and other
    dh_install* commands) do create any missing directory automatically.
    The only thing that you achieve is to have empty not-needed
    directories (for which you need a lintian override). If that's what
    you want fine, but then put only those directories in this file and
    put a comment explaining why you want those empty directories.

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.

[ Reviewing debian/rules ]

15/ The switch suggested in 1/ will implicitly fix many of the
    problems that I'll raise here but I'll give you them anyway.
    - you should really rely on the upstream "make install" to
      do most of the installation work for you instead of abusing
      dh_install...
    - all those chmod +x /chmod -x are not very nice, you'd better use
      something like this:
      find . -name '*.pl' -o -name '*.sh' -exec chmod +x '{}' \;
      find . -name '*.pm' -o -name '*.sql' -exec chmod -x '{}' \;
      => but it might be the upstream "make install" will put the right
      permissions and that you don't have to muck with the permissions at
      all...
    - debian/rules clean is not complete. Try to build your package
      twice in a row and you'll see that it fails... (you have to add make
      distclean and find a way to restore the original
      ledgersmb-httpd.conf (or drop it))
    - add the recommended targets build-arch / build-indep (build-indep
      doing the work, build-arch empty, build depending on build-indep)

[ Reviewing debian/postinst && debian/postrm ]

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)

17/ The same should apply to permissions of sensitive files. They must
    be correctly set in the .deb. If you need to change them for a given
    upgrade, you can add code to debian/postinst but that code should only
    execute once for this specific upgrade and not for all upgrades
    (reason: any change of rights set by the admin must be kept)

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)

[ Misc stuff ]

19/ You depend on "apache2". This is clearly the most common case but it's
    not required. You should better depend on "apache2 | httpd". And then
    you have to be careful when you try to call apache specific commands
    because you must first verify that they are installed...

Some comments about debian/TODO:

- Updating ledgersmb.conf.default with a patch is OK, but the current sed
  approach is OK too (provided that you clean up any temporary file that you
  create).

- Autoconfiguration of an initial database during initial install would be
  nice indeed.

- 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.

- Definitely fix all the lintian warnings, they are easy to fix.

- As I said for PostgreSQL you should aim to always support the current
  default version in Debian.


Back to my sponsoring offer, given the amount of comments, I prefer to not
yet upload this package. Most of those are easily fixable and I fully
expect to upload the next version that you'll submit to me. Make sure to
submit a package that is at least lintian clean (only exception allowed is
the embedded-javascript-library tag) even if you did not manage to fix
all the the other reported issues.

If you have any questions, just ask me and I'll try to help you.

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/

More information about the Pkg-sql-ledger-discussion mailing list