[Pkg-libvirt-maintainers] Bug#1064126: Bug#1064126: libvirt: install NSS modules into /usr

[Andrea Bolognani]

On Wed, Aug 28, 2024 at 09:53:29AM +0200, Helmut Grohne wrote:
> Let me do the review part here given that you already did most of the
> work and given that a number of remarks have multiple options and I
> don't happen to know your preference.

Thanks!

> If you happen to enjoy merge requests originating from the Debian
> Janitor, you may wrap all the added code like this:
> 
> # begin-remove-after: released:trixie
> #DELETE_PROTECTIVE_DIVERSION#
> # end-remove-after
> 
> This serves both as a human-readable reminder for removing the code
> eventually, but it also is machine-readable and will make the Debian
> Janitor propose a MR for removing this code at the desired time. I'll
> leave it up to you whether you want to add support for this or not. If
> you do, please wrap all relevant code to avoid a partial removal.

We've generally strived to maintain wider compatibility than just one
Debian release, for example with the most recent Ubuntu LTS at the
very least and possibly some more. So this suggestion, albeit really
useful, doesn't really apply to libvirt.

> > +        for unit in $VIRTLOGD_UNITS; do
> > +            delete_protective_diversion \
> > +                "/lib/systemd/system/$unit" \
> > +                "10.6.0-3~" \
> > +                -- \
> > +                "$@"
> > +        done
> 
> I suggest running this snippet also in the abort-upgrade case.

For preinst, abort-upgrade is called against old-preinst, so this
doesn't seem very useful. Older versions of the package, by
definition, will not know about the diversion so there won't be
anything to clean up in those cases.

> Also add these calls to postrm matching the arguments failed-upgrade,
> abort-install, and abort-upgrade.

This, on the other hand, makes perfect sense.

> You have handled the libvirt-daemon-log units. The dumat report
> mentioned three other packages libvirt-daemon, libvirt-daemon-common,
> and libvirt-daemon-lock. Will you repeat the maintainer scripts for them
> and their mentioned units in the dumat report? I hope it is fairly
> obvious how to apply this.

Yes, I've just used virtlogd as an example. The other services will
be handled as well.

> > +create_protective_diversion() {
> > +    local usrfile="$1"
> > +    local firstver="$2"
> > +
> > +    if [ "$2" != "--" ]; then
> 
> Do you mean $3 here? I think $2 is firstver.

Yes, I've mentioned the fact that tis was an oversight in a follow-up
message. I guess you must have missed it :)

> > +    if [ -z "$2" ] || dpkg --compare-versions -- "$2" gt "$lastver"; then
> > +        return 0
> > +    fi
> 
> Please remove this check. For instance, libvirt-daemon-common does not
> exist in bookworm. Hence $2 will be empty and you'd skip the addition of
> these diversions when installing libvirt-daemon-common, but they're
> really needed on such a first installation. What you could check here is
> the version of libvirt-daemon-system (the package we are moving the
> units from), but the added value in doing so does not warrant the added
> complexity in my opinion.

Right, this is incorrect. Another consequence of very quickly
sketching out a draft implementation :)

I disagree on the check being useless in general though. The
diversion should only be created when upgrading from a version of
libvirt that was not usr-merged to one that is; when upgrading from a
version that is already usr-merged, we should do nothing.

So the correct check should be

  if [ -n "$2" ] && dpkg --compare-versions -- "$2" gt "$lastver"; then
      return 0
  fi

That is, perform an early exit if we are upgrading from a version of
the package that is newer than the one where usr-merge was
implemented, and create the diversion otherwise.

> > +    if [ ! -e "$usrfile.usr-is-merged" ]; then
> > +        return 0
> > +    fi
> 
> Please remove this check. The diverted file is expected to never exist.
> The sole point of the diversion is to redirect the removal of the file
> to a harmless location. This check would result in the diversions never
> getting removed. If you feel like you need to add a condition, consider
> using this:
> 
> if [ -z "$(dpkg-divert --list "$usrfile")" ]; then
>     return 0
> fi

Got it. I'll use something along the lines of what you suggested
instead.

> Does your salsa-ci pipeline run piuparts? If not, consider running it
> locally before uploading to experimental. It should catch the worst of
> issues.

The pipeline is set up to run piuparts, but the information reported
by the job is unfortunately not really useful at the moment. See

  https://salsa.debian.org/libvirt-team/libvirt/-/merge_requests/231#note_521119

for some additional information about this.

I'm not familiar with running piuparts locally. If you can share some
brief instructions on how to do that, I'll happily try things out
before creating the MR.


I'll work on implementing and testing this over the weekend. I'll
update the bug as appropriate.

-- 
Andrea Bolognani <eof at kiyuko.org>
Resistance is futile, you will be garbage collected.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://alioth-lists.debian.net/pipermail/pkg-libvirt-maintainers/attachments/20240830/0804455c/attachment.sig>