Re: Bug #583585 dpkg-maintscript-helper
Hello,
On Wed, 28 Dec 2011, Gianluca Ciccarelli wrote:
> I thought it was a good idea to share with you the patch for the first
> case for feedback, before going on.
Good idea, what would be even better is to write immediately a
(non-regression) test-case to ensure that the code you wrote work as
expected and keeps doing it in the future.
> I've created the patch according to the guidelines at the dpkg team's
> website¹. Any feedback would be much appreciated.
Great, but I think you have some problems with spaces and tabs. The
indentation looks wrong here. Ensure that a TAB is always displayed like 8
spaces in your editor and then fix the indentation.
> From 27c6be170a5544be6e8aaf6b9f549e4aba0b53c0 Mon Sep 17 00:00:00 2001
> From: Gianluca Ciccarelli (sturmer) <ciccarelli@disi.unitn.it>
> Date: Wed, 28 Dec 2011 16:46:00 +0100
> Subject: [PATCH] dpkg-maintscipt-helper: Case 1 for bug 583585
"case 1 for bug xxx" is not a very good subject, you should rather use
"implement command convert_dir_to_symlink" for example.
(and fix the typo in the dpkg-maintscript-helper)
> @@ -26,6 +26,7 @@
> ##
> ## Functions to remove an obsolete conffile during upgrade
> ##
> +# Dispatcher function
> rm_conffile() {
> local CONFFILE="$1"
> local LASTVERSION="$2"
> @@ -128,6 +129,7 @@ abort_rm_conffile() {
Those supplementary comments are useless. Drop them.
> +# Substitute a directory with a symlink
> +stash_dir() {
Please use something more explicit like
"convert_dir_to_symlink".
> + local DIRECTORY=$1
> + local LAST_VERSION=$2
> +
> + [ -d "$DIRECTORY" ] || error "a directory should be specified"
This check will fail after you have renamed your directory... (hence the
importance of testing your code with a test-case).
> + if [ "$LAST_VERSION" = "--" -o \
> + -z "$LAST_VERSION"]; then
> + error "please specify the number of the first version having a symlink"
"please specicfy the version number of the first package having a symlink"
> + # In the case statement, $1 is the preinst command, $2 the version
> + case "$DPKG_MAINTSCRIPT_NAME" in
> + preinst)
> + if [ -n "$2" ] && dpkg --compare-versions "$2" le-nl "$LASTVERSION"; then
> + create_dir_backup "$DIRECTORY"
> + fi
You have to test $1 too. You should do this only if "$1" is "upgrade".
> + postinst)
> + if [ -n "$2" ] && dpkg --compare-versions "$2" le-nl "$LASTVERSION"; then
> + rm_dir_backup "$DIRECTORY"
> + fi
Again you need to check that $1 is "configure".
> + postrm)
> + if [ -n "$1" ] && [ "$1" = "failed-upgrade" ] && \
You don't need to check that $1 is not empty if you already check that
it's equal to "failed-upgrade".
> + [ -n "$2" ] && dpkg --compare-versions "$2" le-nl "$LASTVERSION"; then
> + restore_dir_backup "$DIRECTORY"
> + fi
> + ;;
> + *)
> + # TODO substitute stash_dir with a variable
Why this TODO ?
> + debug "$0 stash_dir not required in $DPKG_MAINTSCRIPT_NAME"
> + ;;
> + esac
> + shift
> +}
> +
> +create_dir_backup() {
Please rename those helper functions to follow the same naming scheme
that has been used for other commands:
preinst -> prepare_<command>
postinst -> finish_<command>
postrm -> abort_<command>
> + # Ensure ownership of the dir by the package
> + [ -d "$DIRECTORY" -a -O "$DIRECTORY" ] || \
> + error "the package has no ownership of the directory, check permissions"
This -O check is completely wrong. "dpkg -S $DIRECTORY" is the command to
use if you want to verify who "owns" the directory as far as dpkg is
concerned. You must verify that only the package being installed owns it
and that the directory is not shared with another package.
> +restore_dir_backup() {
> + [ -d "$DIRECTORY" ] && mv "$DIRECTORY".dpkg-bak "$DIRECTORY"
> +}
You should rather get rid of "$DIRECTORY" before moving
$DIRECTORY.dpkg-bak into its original place. Testing the existence of
$DIRECTORY does not make sense.
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/
Reply to: