config-package-dev maintainer scripts don’t undivert on deconfigure

[andersk]

We currently undivert on prerm when [ "$1" = "remove" ]. This should probably be [ "$1" != "upgrade" ], so as to include the deconfigure case as well.

More generally, we should check that we handle  all these cases correctly.

[andersk]

Ooh, here are the same cases, presented in the form of  diagrams that you can actually read.

[jdreed]

This patch appears technically simple, so I assume it's not blocking on that. Is there a reason not to take it? The logic seems sound. It doesn't deal with error-unwind, but it also doesn't currently deal with that, so that's not a regression. At the very least, if we're nervous about the not-upgrade case, can we explicitly add "$1" = "deconfigure" in addition to remove? This would buy us the abilility to bump our versions with Conflicts/Breaks?, and then allow us to clean up rules files currently cluttered with DEB_UNDIVERT_FILES=stuff-from-.2009

[jdreed]

OK, clarifying from earlier, I think I'm going to advocate for explicitly undiverting on deconfigure. We can't really handle failed-upgrade well (apart from doing nothing, which we already do). And the reason I want this is because packages are getting ridiculously cluttered with undiversions and unremovals, so I'd like to be able to release newer versions that Conflict: with the old ones, and clean up our source tree.

Therefore, I think I want this patch:

Index: divert.mk
===================================================================
--- divert.mk	(revision 25759)
+++ divert.mk	(working copy)
@@ -99,7 +99,7 @@
 		fi;) \
 	    sed 's/#PACKAGE#/$(cdbs_curpkg)/g; s/#DEB_DIVERT_EXTENSION#/$(DEB_DIVERT_EXTENSION)/g' $(DEB_DIVERT_SCRIPT); \
 	    $(if $(divert_files_thispkg), \
-		echo 'if [ "$$1" = "remove" ]; then'; \
+		echo 'if [ "$$1" = "remove" ] || [ "$$1" = "deconfigure" ]; then'; \
 		$(foreach file,$(call reverse_dh_compat_5,$(divert_files)), \
 		    echo "    undivert_unlink $(call divert_files_replace_name,$(file), )";) \
 		$(foreach file,$(call reverse_dh_compat_5,$(divert_remove_files)), \

[jdreed]

I discussed this with geofft earlier this afternoon. He points out that my epoch bump idea won't really help, because we have to account for versions that predate the undivert to prevent upgrade failures. Effort is better spent on #867. That having been said, we do still want this, because deconfiguring a config package should in fact stop applying the configuration.

[geofft]

Replying to andersk:

We currently undivert on prerm when [ "$1" = "remove" ]. This should probably be [ "$1" != "upgrade" ], so as to include the deconfigure case as well.

This is fixed in config-package-dev 4.14 in unstable, uploaded to Debathena as r25800.

More generally, we should check that we handle  all these cases correctly.

This isn't, yet.

[jdreed]

This ticket has been around for 3 years and nobody has identified the other maintainer script invocations we don't handle correctly. We should identify them, or close this ticket. As far as I can tell, we're all set, modulo the error-unwind conditions, which I've never actually seen in the wild?

preinst: n/a
postinst: We deal with configure correctly.
prerm: We now deal correctly with upgrade, remove, and deconfigure.
postrm: n/a

[jdreed]

OK, I thought about the error unwind a bit, since that seems to be the only remaining condition we don't deal with.

• The only thing we do in the postinst is configure, and if configure fails, it's always an error, there's no unwind. So that's fine.
• In the prerm, we support remove, and deconfigure. Those could concievably fail (resulting in a postinst abort-remove), but we won't know whether c-p-d's snippets failed, or whether something else in the maintainer script did. (This is likely why very few maintainer script snippets from debhelper tools handle the unwind conditions. It's possible that when #867 gets fixed, we can write code that will iterate over the changes a package did/may-have made, but even then, let's say that undivert_unlink_symlink succeeds, but undivert_unlink_divert fails. That's doing to be a mess.)

That having been said, we're determined to deal with these conditions (and frankly, I'm not), we have a couple of options:

• The tex debhelper snippets rely on idempotency and simply call the same postinst code as configure for above-remove.
• zope's debhelper code has a handler for all its maintainer scripts, and it saves state in the prerm to refer to for error-unwind in the postinst
• The lisp debhelper code retries the prerm operations in the postinst abort-upgrade, which seems particularly wrong.

I see three ways forward, in rough order of preference:

• Stop caring.
• The postinst does the same thing for abort-remove as it does for configure.
• The postinst yells loudly on STDERR in above-remove to inform the user that operations may have failed and to inspect a (finite) set of files manually.

I think attempting to save state is asking for trouble.

[jdreed]

I formally propose the following. I vaguely want to add a line to inform the user (on stderr) that we're trying to recovery, but I think dpkg whining is probably good enough?

jdreed@infinite-loop:~/src/config-package-dev$ git diff master
diff --git a/dh_configpackage b/dh_configpackage
index 9d7c117..fcc3abe 100755
--- a/dh_configpackage
+++ b/dh_configpackage
@@ -399,7 +399,7 @@ foreach my $package (@{$dh{DOPACKAGES}}) {
     if (! $dh{NOSCRIPTS}) {
         if (@undisplacefiles || @unhidefiles || @displacefiles || @hidefiles) {
             my $postinst = escape_shell(join "\\n", (
-                'if [ "$1" = "configure" ]; then',
+                'if [ "$1" = "configure" ] || [ "$1" = "abort-remove" ]; then',
                 (map {"    check_undisplace_unlink " . displace_files_replace_n
                 (map {"    check_undisplace_unhide $_ " . hide_files_name($_, $
                 (map {"    displace_link " . displace_files_replace_name($packa

[jdreed]

The first half of this was fixed in config-package-dev 5.0

[jdreed]

This was fixed in 5.1