Merge ~bryce/ubuntu/+source/php7.4:fix-lp1865218-mod-php-upgrade-groovy into ubuntu/+source/php7.4:ubuntu/devel

Proposed by Bryce Harrington
Status: Merged
Approved by: Bryce Harrington
Approved revision: 119afc2448d751608bc3045cc03ff4f971dc1629
Merge reported by: Bryce Harrington
Merged at revision: 119afc2448d751608bc3045cc03ff4f971dc1629
Proposed branch: ~bryce/ubuntu/+source/php7.4:fix-lp1865218-mod-php-upgrade-groovy
Merge into: ubuntu/+source/php7.4:ubuntu/devel
Diff against target: 38 lines (+16/-3)
2 files modified
debian/changelog (+8/-0)
debian/libapache2-mod-php.postinst.extra (+8/-3)
Reviewer Review Type Date Requested Status
Christian Ehrhardt  (community) Approve
Canonical Server Pending
Review via email: mp+383848@code.staging.launchpad.net

Description of the change

SRU to fix failure when upgrading with mod-php enabled.

I've verified this works using the test case from the bug #1865218, using this PPA:

PPA: https://launchpad.net/~bryce/+archive/ubuntu/php-defaults-fix-lp1865218-mod-php-upgrade

To post a comment you must log in.
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Generally I like the new approach, to disable the old one to avoid the latter conflict.
By not existing early the new one would be enabled as intended.

A few questions:

Improved wording
"Disabling $PHP_MODULE" -> "Disabling old $PHP_MODULE"
If you insist on being perfect since this would also trigger on downgrades you can version-compare and say "old/new" but downgrades are nonono anyway, so a static "old" will do as well.

  "failed to enable $PHP_MODULE"
That should now better be something like:
  "failed to disable old $PHP_MODULE module"

In some of the SRUs backports the php version in the message needs to change at "favor of using PHP 7.4".
If you want you might use @PHP_VERSION@ to get code that is the same for all releases.

review: Needs Fixing
Revision history for this message
Bryce Harrington (bryce) wrote :

Suggested changes applied, ready for re-review.

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

thanks, I was going for a practical test, but the PPA is odd, it has 2 bionic and 2 eoan versions (two php versions each).
But no Focal/Groovy builds at all.

I'd have expected builds as proposed here in the PPA.

I tried to setup a system that would trigger the upgrade path as needed, but it gets more and more (like php-common >73) and so on.

Therefore I have manually done what the Maintscripts would do.
That worked well, but let us make sure that on the actual SRU we really test the real builds.

Finally, I know Debian does handle php versions differently but is this in some way upstreamable to them still or will we have to carry it forever? If we have to, please add a bunch of extra text to the groovy-commit so we can all remember.

review: Approve
Revision history for this message
Bryce Harrington (bryce) wrote :

On Thu, May 14, 2020 at 11:52:37AM -0000, Christian Ehrhardt  wrote:
> Review: Approve
>
> thanks, I was going for a practical test, but the PPA is odd, it has 2 bionic and 2 eoan versions (two php versions each).
> But no Focal/Groovy builds at all.
>
> I'd have expected builds as proposed here in the PPA.

I've uploaded the updated binary builds for the PPA as well.

> I tried to setup a system that would trigger the upgrade path as needed, but it gets more and more (like php-common >73) and so on.
>

Interesting, I am thinking the steps to trigger the condition should be
straightforward, but maybe easier with the updated ppa.

> Therefore I have manually done what the Maintscripts would do.
> That worked well, but let us make sure that on the actual SRU we really test the real builds.
>
> Finally, I know Debian does handle php versions differently but is this in some way upstreamable to them still or will we have to carry it forever? If we have to, please add a bunch of extra text to the groovy-commit so we can all remember.

That's a good point to provide comments in the groovy-commit, however
this package isn't (yet) in git-ubuntu so the commit messages will be
lost. Hopefully the commits in the bug report will be sufficient as
institutional memory.

Regarding sending this to debian, I've thought a bit about this. I
think we have a bit of a philosophical difference in that Ubuntu accepts
a restriction of no more than one PHP version in a given Ubuntu release
with a goal of simplified user experience, whereas Debian strives for
allowing a more permissive multi-PHP version experience at the expense
of a more complex user experience. So, I'm not sure this patch - at
least as I've coded it - is going to meet their needs.

What I'd like to do is land it for us and make sure it meets our own
userbase's needs, let Debian have a chance to review and adjust if they
wish, and then perhaps for Ubuntu h-series or i-series see if we can
find a common solution with Debian.

Bryce

Revision history for this message
Bryce Harrington (bryce) wrote :

I went ahead and mentioned the fix on the associated Debian bug. Again, I doubt they would want to take this patch as-is but might give them a starting point for a better fix.

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches