Code review comment for ~ahasenack/ubuntu/+source/base-files:xenial-handle-was-removed-1895302

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

> 44d97359a4f73cad77e9bf30b0c1bd21b4d4c732
> - Seems a reasonable solution.
> - I'm curious if you tried dpkg -s ubuntu-server and found that the -l
> option with grep was better?

No, I haven't tried dpkg -s. I did grep my git-ubuntu checkouts for dpkg usage in postinst, and found some "samples", but not a clear pattern:

openssl/debian/libssl1.1.postinst:
# Only get the ones that are installed, and configured
check=$(dpkg -s $check 2> /dev/null | egrep '^Package:|^Status:' | awk '{if ($1 ~ /^Package:/) { package=$2 } else if ($0 ~ /^Status: .* installed$/) { print package }}')

postfix/debian/postfix.postinst:
    nis_status=$(dpkg -l nis 2>/dev/null | sed -n '$p')
    if [ "X$nis_status" != "X${nis_status#i}" ] && [ -x /usr/bin/ypcat ] &&
        /usr/bin/ypcat mail.aliases >/dev/null 2>&1; then
        alias_maps="hash:/etc/aliases, nis:mail.aliases"

> 989427c706c868917534627390bca6b28baeeb55
> - LGTM. Much cleaner looking and likely more maintainable.
>
> 29622498abfe7ed935eacc9abefd476505e4e34c
> - Do I understand correctly that this differentiates between installs vs.
> upgrades by $2 being non-blank? If so, is it 100% guaranteed that this is
> always blank on install?

100%? :)

It's a pattern seen all over the place, and the technical description is:
postinst configure most-recently-configured-version
$0: postinst
$1: configure
$2: most-recently-configured-version
    The files contained in the package will be unpacked. All package dependencies will at least be “Unpacked”. If there are no circular dependencies involved, all package dependencies will be configured. For behavior in the case of circular dependencies, see the discussion in Binary Dependencies - Depends, Recommends, Suggests, Enhances, Pre-Depends.

>
> 52a955f23051e297070e594d78e89436b3a1c893
> - LGTM
>
> 6ca97cfce91cc40d8fbf6c5a61f487c5c2659f43
> - Changelog looks good.
>
> A couple questions just to clarify some assumptions. Otherwise, looks good.
>
> If you'd like me to run the test cases before you land this, I can do that
> tomorrow.

This would help, thanks. The groovy changes were uploaded and migrated already.

« Back to merge proposal