[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