Merge ~athos-ribeiro/ubuntu/+source/apache2:postinst-triggered-mm into ubuntu/+source/apache2:ubuntu/mantic-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)]

I will prepare the SRU templates for this one after we agree this is an acceptable change and its noble counterpart gets uploaded.

[Athos Ribeiro (athos-ribeiro)]

  - apache2/2.4.57-2ubuntu2.4~ppa1
    + ✅ apache2 on mantic for amd64 @ 18.03.24 19:59:49 Log️ 🗒️
    + ✅ apache2 on mantic for arm64 @ 18.03.24 20:00:40 Log️ 🗒️
    + ✅ apache2 on mantic for armhf @ 18.03.24 19:15:08 Log️ 🗒️
    + ✅ apache2 on mantic for ppc64el @ 18.03.24 18:51:31 Log️ 🗒️
    + ✅ apache2 on mantic for s390x @ 18.03.24 18:49:14 Log️ 🗒️

[Sergio Durigan Junior (sergiodj)]

Thanks, Athos.

Not much to say here except what I've already mentioned in the Noble MP. I'd like to get more input from the Debian apache2 maintainers just to make sure there are no unintended consequences before adopting such change. OTOH, I know that there's an MR on salsa which has been waiting for 4 months without any replies, so let's not block ourselves here.

Code-wise, everything LGTM. The SRU text is also very good, thanks for being thorough there.

+1

review: Approve

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

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

review: Approve

[Sergio Durigan Junior (sergiodj)]

Thinking a bit more about this...

Do you think you might encounter some backlash from the SRU team here? I'm specifically worried about the "behaviour change" side of this fix. I know you did your homework on this one, so I'd like to know your opinion about this possible issue.

Thanks!

[Athos Ribeiro (athos-ribeiro)]

Thanks, Sergio.

It would be worth discussing this with with the SRU team (after an upload?). I'd be OK with waitin ga bit longer to see if we get any feedback from Debian on this.

> I'm specifically worried about the "behaviour change" side of this fix.

AFAICT, PHP should be the only package affected. If the concern is about "we are adding a feature to a stable release", I do believe that on the PHP end this should be considered a bug (which has been there for a long while) instead. However, this may indeed be seen as a feature in the apache2 side.

That said, an alternative approach could be:

- keep the noble change, adding support for triggers in apache2.
- change the php's libapache2-mod-php postinst script to reload or restart apache2 without the apache2-maintscript-helper.

Still, given that (most likely) the only known behavioral change is in php, the approach presented here could still be acceptable, although the alternative presented above looks safer from a SRU perspective.

Thoughts?

[Sergio Durigan Junior (sergiodj)]

On Wednesday, March 20 2024, Athos Ribeiro wrote:

> It would be worth discussing this with with the SRU team (after an upload?). I'd be OK with waitin ga bit longer to see if we get any feedback from Debian on this.

ACK.

>> I'm specifically worried about the "behaviour change" side of this fix.
>
> AFAICT, PHP should be the only package affected. If the concern is
> about "we are adding a feature to a stable release", I do believe that
> on the PHP end this should be considered a bug (which has been there
> for a long while) instead. However, this may indeed be seen as a
> feature in the apache2 side.

I understand where you're coming from. This is a bug on apache2 because
PHP expects apache2 to react to triggers. But it can also be said that
maybe apache2 *shouldn't* react to those triggers, because it doesn't
want to be restarted at random times.

> That said, an alternative approach could be:
>
> - keep the noble change, adding support for triggers in apache2.
> - change the php's libapache2-mod-php postinst script to reload or restart apache2 without the apache2-maintscript-helper.

You mean that you could implement either one or the other, right?

Honestly, they're equivalent and don't change the underlying problem of
restarting apache2 "behind the scenes" after installing a PHP module
(but maybe I'm misunderstanding what you're saying).

> Still, given that (most likely) the only known behavioral change is in php, the approach presented here could still be acceptable, although the alternative presented above looks safer from a SRU perspective.

Let me backtrack a little bit and ask you this: if you only reload
apache2 (instead of restarting it), does it work the same? Because if
it does, then that would be less intrusive indeed.

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

[Athos Ribeiro (athos-ribeiro)]

After discussing this within the server team, we decided to let this update sit in noble for a while, so we can understand its impact on users before we keep pushing for the SRUs here.

The reason for that is that most other packages will not restart apache2 (nor other services) after an upgrade (we do understand the benefits of the restart in case of security updates though).

FWIW, needrestart does warn/query for the restart need for the upgrade path being addressed here.

In the meanwhile, I will ping Debian again so we can be sure we are in the same page regarding the regression potential for this change.

Unmerged commits

adceb5b... by Athos Ribeiro on 2024-03-18

Update changelog

380a7a3... by Athos Ribeiro on 2024-03-18

Add support to package triggers

--CL--
    - 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)

diff --git a/debian/changelog b/debian/changelog
index 6a8ffcc..d61f2b1 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+apache2 (2.4.57-2ubuntu2.4) mantic; 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 10:39:39 -0300
+
 apache2 (2.4.57-2ubuntu2.3) mantic; urgency=medium
 
   * d/c/m/setenvif.conf, d/p/fix-dolphin-to-delete-webdav-dirs.patch: Add
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