Merge ~athos-ribeiro/ubuntu/+source/apache2:postinst-triggered into ubuntu/+source/apache2:ubuntu/devel

[Athos Ribeiro]

libapache2-phpX.Y-mod's postinst script has a trigger to restart apache2.

php-X.Y-common activates the trigger whenever it is upgraded, and all php extensions shipped through the phpX.Y package has a pinned required version on the php-X.Y-common it was built with it (i.e., whenever php or a php extention built from the php source is updated, the trigger is activated). External (not built from the phpX.Y sources) extensions packaged should also activate the trigger so apache2 is restarted.

The trigger relies on the apache2-maintscript-helper script shipped by apache2. However, the apache2 script does not support the "triggered" postinst parameter, which is passed to postinst when a trigger is activated.

This means that apache2 is not restarted when php or an extension is upgraded.

The proposed patch adds support for "postinst triggered" in apache2.

While PHP seems to be the only package using a trigger with the pache2-maintscript-helper, this seem to be a valid use case which should be supported.

A script to test this patch from the the PPA linked below is available at https://bugs.launchpad.net/debian/+source/apache2/+bug/2038912/comments/5

See LP: #2038912 for additional context.

PPA: https://launchpad.net/~athos-ribeiro/+archive/ubuntu/apache2-postist-triggered/+packages

[Athos Ribeiro (athos-ribeiro)]

Testing in noble should be done in a different way since we are relaying on the release pocket => updates pocket upgrade path.

One idea would be to use the package in the -proposed pocket (if still available) or to build a new package with a deb version bump to test it.

[Athos Ribeiro (athos-ribeiro)]

The autopkgtest test suite is currently broken due to dependency issues related to the time_t transition.

[Sergio Durigan Junior (sergiodj)]

Thanks, Athos.

I'm leaving a couple of minor comments below, but they're cosmetic.

Code-wise, this change is simple and correct. There's not much to comment, I believe.

Feature-wise, I'm always a bit reticent about introducing a "breaking" change like this, especially when it comes to Apache. I mean, the goal of forcing apache2 to reload the php module when something changes is a good one, and as you mentioned, it actually should have always worked like this. But given that it doesn't (and never has, apparently?), I'm wondering if it will catch any user out there by surprise. For example, when you use "a2enmod" to enable an Apache module, the command doesn't restart apache2 automatically for you; instead, it prints a message saying that you should do it. So I think I'd expect a similar behaviour when it comes to PHP modules...?

Something else that comes to mind is how docker.io works. It will ask the user whether they want the service (and the containers) to be restarted after upgrade. Maybe PHP could do the same.

Anyway, these are all thoughts that occurred to me while reviewing the changes, but not something I'd expect you to implement. And I agree with your assessment in the bug when you say that we should hear what Debian has to say about this change. OTOH, time is running out for Noble and I believe that this is the right moment to introduce such a change.

I'm approving the MP for Noble based on that. I believe it's worth mentioning this change in the release notes as well, because it may catch some users unprepared.

LGTM, +1.

review: Approve

[git-ubuntu bot (git-ubuntu-bot)]

Approvers: athos-ribeiro, sergiodj
Uploaders: athos-ribeiro, sergiodj
MP auto-approved

review: Approve

[Athos Ribeiro (athos-ribeiro)]

Thanks, Sergio.

I addressed the cosmetic comments and uploaded.

Uploaded:

Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading apache2_2.4.58-1ubuntu6.dsc: done.
  Uploading apache2_2.4.58-1ubuntu6.debian.tar.xz: done.
  Uploading apache2_2.4.58-1ubuntu6_source.buildinfo: done.
  Uploading apache2_2.4.58-1ubuntu6_source.changes: done.
Successfully uploaded packages.

The indentation nitpick is actually an issue with that script where some of the "blanks" after the "#" characters are spaces and some are tabs. Since there is no point in carrying a delta to have that fixed, I just replaced the space for a tab there (although other items in the same list will have a space instead).

I also updated all the SRU branches with the same changes.

> For example, when you use "a2enmod" to enable an Apache module, the command doesn't restart apache2 automatically for you; instead, it prints a message saying that you should do it. So I think I'd expect a similar behaviour when it comes to PHP modules...?

This is an interesting thought. I need to assess what other modules are doing. I see most users of apache2_reload are just reloading instead of restarting, but there may be other modules that are just not using the maintscript helper, so I'd need to check those as well.

FWIW, the change should enable reloading (or doing other actions) through the script as well. Currently, it mostly tell the script to skip/noop (if used properly) when called from a trigger.

I will add an entry to the PHP release notes for this. I agree this is important enough to be mentioned there.

[Andreas Hasenack (ahasenack)]

This is in noble and oracular, marking MP as merged.

diff --git a/debian/changelog b/debian/changelog
index ec578f4..4903e89 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+apache2 (2.4.58-1ubuntu6) noble; urgency=medium
+
+  * d/debhelper/apache2-maintscript-helper: Allow execution when called from a
+    postinst script through a trigger (i.e., postinst triggered).
+    Thanks to Roel van Meer. (LP: #2038912) (Closes: #1060450)
+
+ -- Athos Ribeiro <[e-mail]>  Mon, 18 Mar 2024 09:35:36 -0300
+
 apache2 (2.4.58-1ubuntu5) noble; urgency=medium
 
   * No-change rebuild against libcurl4t64
diff --git a/debian/debhelper/apache2-maintscript-helper b/debian/debhelper/apache2-maintscript-helper
index ce20fb1..f6c53f6 100644
--- a/debian/debhelper/apache2-maintscript-helper
+++ b/debian/debhelper/apache2-maintscript-helper
@@ -198,6 +198,8 @@ apache2_needs_action()
 	#	Probably the most important invokation. When invoked in configure we:
 	#	- enable the piece of configuration on fresh installs
 	#	- do nothing on upgrades UNLESS the configuration was removed automatically in the past
+	# postinst triggered:
+	#	- use package triggers to restart apache2
 	# postrm remove|purge
 	#	- disable the configuration, mark it as automatically disabled in remove
 	#	- disable the configuration, remove any trace we have on purge
@@ -223,6 +225,8 @@ apache2_needs_action()
 			[ -z "$APACHE2_MAINTSCRIPT_ARGUMENT" ] && return 0
 			# act if someone told us
 			[ -n "$APACHE2_NEED_ACTION" ] && return 0
+		elif [ "$APACHE2_MAINTSCRIPT_METHOD" = "triggered" ] ; then
+			return 0
 		fi
 		;;
 	esac