Re: Bug #583585 dpkg-maintscript-helper
Hello Gianluca,
On Sun, 15 Jan 2012, Gianluca Ciccarelli wrote:
> I've tried to follow the suggestions Raphael gave me in the previous
> mail, and produced a new patch (indents as an 8-space-wide tab, please
> tell me if I got this right).
The spacing looks sane this time.
> I also created an initial test within pkg-tests, that I've called
> t-convert-dir-to-symlink-upgrade. It doesn't cover all cases produced by
> the command I've introduced in dpkg-maintscript-helper.sh, but I wanted
> to get some feedback before going on.
I have not tested your code but here's some feedback anyway. It looks much
better but there are still some issues.
> +# Substitute a directory with a symlink
> +convert_dir_to_symlink() {
> + local DIRECTORY="$1"
> + local LAST_VERSION="$2"
Be consistent with the rest of the file where we used LASTVERSION and not
LAST_VERSION.
> +
> + [ -d "$DIRECTORY" -o -L "$DIRECTORY" ] || \
> + error "a directory should be specified"
You shouldn't error out when there's no symlink/directory. You should
just adapt your behaviour (i.e. there's nothing to backup). It can also
mean that the upgrade failed and it's now retried while you have already
made the backup...
The only thing that you might to check is that $DIRECTORY is not an empty
string.
> + if [ "$LAST_VERSION" = "--" -o -z "$LAST_VERSION" ]; then
> + error "please specify the version number of the first package "\
> + "having a symlink"
> + fi
> +
> + while [ "$1" != "--" -a $# -gt 0 ]; do shift; done
> + [ $# -gt 0 ] || badusage
> + [ -n "$DPKG_MAINTSCRIPT_NAME" ] || \
> + error "could not determine the launching maint script"
> + shift
Be consistent with the other similar code and put the check of
DPKG_MAINTSCRIPT_NAME after the parameter processing code (and use the same
error string):
while [ "$1" != "--" -a $# -gt 0 ]; do shift; done
[ $# -gt 0 ] || badusage
shift
[ -n "$1" ] || error "maintainer script parameters are missing"
[ -n "$DPKG_MAINTSCRIPT_NAME" ] || \
error "environment variable DPKG_MAINTSCRIPT_NAME is required"
> + # In the case statement, $1 is the name of the maint script, $2 the
> + # package version
> + case "$DPKG_MAINTSCRIPT_NAME" in
> + preinst)
> + if [ "$1" = "upgrade" ] && [ -n "$2" ] && \
> + dpkg --compare-versions "$2" le-nl "$LASTVERSION"; then
> + debug "preinst upgrade called"
Don't forget to drop those useless debug statements at the end before
submitting your final patch.
The [ -n "$2" ] test is not strictly required here since we always have a
version (and le-nl already considers an empty string as later than any other
version). But it's not a problem.
[...]
> +prepare_convert_dir_to_symlink() {
> + local DIRECTORY="$1"
> +
> + # Ensure ownership of the dir by the package ("dpkg -S $DIRECTORY"
> + # lists only one result before the dir name)
> + [ -d "$DIRECTORY" ] && \
> + [ "$(dpkg -S "$DIRECTORY" | awk '{printf NF-1}')" -eq 1 ] || \
> + error "the package has no exclusive ownership of the directory; "\
> + "please check permissions"
> +
> + mv "$DIRECTORY" "$DIRECTORY".dpkg-bak
> +}
This check is still entirely wrong:
- if the file/directory contains a space, it will break because the
counting of fields in the output will be inaccurate
- it doesn't verify that the only package owning the directory
is the one being upgraded, but just ensures that a single
package is owning it
- it doesn't ensure that the directory doesn't contain
non-packaged files
For the last point, it might be worth trying to not fail but to
just have it keep those files at some standard place. You could do
something similar to the other scripts: use $DIRECTORY.dpkg-remove for
the full backup that will be removed but extract the non-packaged files
and keep them in $DIRECTORY.dpkg-backup. And in the postinst rename
DIRECTORY.dpkg-backup to DIRECTORY.dpkg-bak and drop
$DIRECTORY.dpkg-remove. Obviously output a message explaining that you
have kept non-packaged files on the indicated place.
> +finish_convert_dir_to_symlink() {
> + local DIRECTORY="$1"
> +
> + debug "Removing '$DIRECTORY.dpkg-bak'"
> + rm -rf "$DIRECTORY".dpkg-bak
Nitpick: I prefer the "$DIRECTORY.dpkg-bak" way of writing it. Having quotes in
the middle of the string always seems weird.
> +}
> +
> +abort_convert_dir_to_symlink() {
> + local DIRECTORY="$1"
> +
> + unlink "$DIRECTORY"
Why are you using unlink ? "rm -f $DIRECTORY" ought to be enough.
> + mv "$DIRECTORY".dpkg-bak "$DIRECTORY"
Please test its existence before doing it blindly. All scripts must be
idempotent, it means we should be able to run them twice and it should
still work.
> +}
Also it would be nice to add some echo statements so that logs briefly
indicate that those operations have been tried.
Now your test case. First git can't store empty directories so you should
make sure that your pkg-dir-to-symlink-0 does contain some files in its
/test-dir... it also makes for a more realistic use case. Real directories
tend to contain files...
> diff --git a/t-convert-dir-to-symlink-upgrade/Makefile b/t-convert-dir-to-symlink-upgrade/Makefile
> new file mode 100644
> index 0000000..839cc7e
> --- /dev/null
> +++ b/t-convert-dir-to-symlink-upgrade/Makefile
> @@ -0,0 +1,17 @@
> +TESTS_DEB := pkg-dir-to-symlink-0 pkg-dir-to-symlink-1
> +
> +include ../Test.mk
> +
> +define VERIFY
> +test "`$(PKG_STATUS) pkg-dir-to-symlink`" = "install ok installed"
> +endef
There's already a macro for this:
$(call pkg_is_installed,pkg-dir-to-symlink)
> +test-clean:
> + $(DPKG_PURGE) pkg-dir-to-symlink
Ensure it doesn't fail when the package is not installed.
> +++ b/t-convert-dir-to-symlink-upgrade/pkg-dir-to-symlink-0/DEBIAN/postrm
> @@ -0,0 +1,17 @@
> +#!/bin/sh
> +
> +set -e
> +case $1 in
> +upgrade)
> + echo "DEBUG: Called postrm upgrade" >&2
> + ;;
> +failed-upgrade)
> + if dpkg-maintscript-helper supports convert_dir_to_symlink; then
> + export DPKG_DEBUG=1
> + dpkg-maintscript-helper convert_dir_to_symlink "/test-dir" 0 -- "$@"
> + fi
> + ;;
> +*)
> + ;;
> +esac
This is not how dpkg-maintscript-helper is supposed to be used. It must be
unconditionnally called and the script itself decides if it has something
to do.
See t-conffile-obsolete/pkg-conff-obsolete-2/DEBIAN/* for example.
> --- /dev/null
> +++ b/t-convert-dir-to-symlink-upgrade/pkg-dir-to-symlink-1/DEBIAN/postinst
Same here.
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: