Merge ~ahasenack/ubuntu/+source/base-files:xenial-handle-was-removed-1895302 into ubuntu/+source/base-files:ubuntu/xenial-devel

Proposed by Andreas Hasenack
Status: Approved
Approved by: Andreas Hasenack
Approved revision: 6ca97cfce91cc40d8fbf6c5a61f487c5c2659f43
Proposed branch: ~ahasenack/ubuntu/+source/base-files:xenial-handle-was-removed-1895302
Merge into: ubuntu/+source/base-files:ubuntu/xenial-devel
Diff against target: 64 lines (+25/-7)
3 files modified
debian/changelog (+14/-0)
debian/motd-news-config.postinst (+1/-1)
debian/postinst.in (+10/-6)
Reviewer Review Type Date Requested Status
Bryce Harrington (community) Approve
Canonical Server Core Reviewers Pending
Review via email: mp+390865@code.launchpad.net

Description of the change

To post a comment you must log in.
Revision history for this message
Bryce Harrington (bryce) 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?

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?

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.

review: Approve
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.

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

I've run the test case described in the SRU bug in a xenial LXC container without ubuntu-server installed, and verified without the PPA package it creates the wasremoved file, but if upgrading instead to the PPA version of the package, no wasremoved file was created if there was not one there already.

I noticed though that if the wasremoved file was already present, upgrading to the PPA does not remove it. I'm not sure whether that's intentional, but I removed everything and re-ran the test cases (upgrading from xenial's base-file, to xenial-updates, then to the PPA) and verified this is indeed the behavior. The last sub-bullet in the changelog suggests the file should not be present at all; if this is expected perhaps that needs clarification?

I also ran all the same testing in parallel on bionic and focal, and saw identical behaviors.

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

I mentioned in the sru template that no attempts are made to fix the case
where someone already has the wasremoved file and just installs the updated
base-files. The wasremoved file is only removed in motd-news-config, and I
don't know how to detect correctly the case to safely remove it in
base-files.

On Thu, Sep 17, 2020, 19:31 Bryce Harrington <email address hidden>
wrote:

> I've run the test case described in the SRU bug in a xenial LXC container
> without ubuntu-server installed, and verified without the PPA package it
> creates the wasremoved file, but if upgrading instead to the PPA version of
> the package, no wasremoved file was created if there was not one there
> already.
>
> I noticed though that if the wasremoved file was already present,
> upgrading to the PPA does not remove it. I'm not sure whether that's
> intentional, but I removed everything and re-ran the test cases (upgrading
> from xenial's base-file, to xenial-updates, then to the PPA) and verified
> this is indeed the behavior. The last sub-bullet in the changelog suggests
> the file should not be present at all; if this is expected perhaps that
> needs clarification?
>
> I also ran all the same testing in parallel on bionic and focal, and saw
> identical behaviors.
>
>
> --
>
> https://code.launchpad.net/~ahasenack/ubuntu/+source/base-files/+git/base-files/+merge/390865
> You are the owner of
> ~ahasenack/ubuntu/+source/base-files:xenial-handle-was-removed-1895302.
>
> Launchpad-Message-Rationale: Owner
> Launchpad-Message-For: ahasenack
> Launchpad-Notification-Type: code-review
> Launchpad-Branch:
> ~ahasenack/ubuntu/+source/base-files/+git/base-files:xenial-handle-was-removed-1895302
>

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

s/rio attempts/no attempts/

phone autocorrect...

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

About the d/changelog mention of removing .wasremoved, see it's about motd-news-config, not base-files:

    - d/motd-news-config.postinst: always remove /e/d/motd-news.wasremoved
      if present

I don't know if there is a way to have base-files safely remove .wasremoved if it was placed there unintentionally by the previous update. Unless I perhaps use another filename from now on (.wasremoved-new ick), and always delete the .wasremoved one, but this is getting a rabbit hole

Revision history for this message
Bryce Harrington (bryce) wrote :
Download full text (5.6 KiB)

So, installing motd-news-config (e.g. after or independently from upgrading base-files) should always remove /e/d/motd-news.wasremoved if it is present? I don't think I'm seeing that though:

root@triage-bionic+18.04:~# ll /etc/default/motd*
-rw-r--r-- 1 root root 0 Sep 17 22:26 /etc/default/motd-news.wasremoved
root@triage-bionic+18.04:~# sudo add-apt-repository ppa:ahasenack/motd-news-wasremoved-handling

 More info: https://launchpad.net/~ahasenack/+archive/ubuntu/motd-news-wasremoved-handling
Press [ENTER] to continue or Ctrl-c to cancel adding it.

Hit:1 http://security.ubuntu.com/ubuntu bionic-security InRelease
Hit:2 http://archive.ubuntu.com/ubuntu bionic InRelease
Get:3 http://ppa.launchpad.net/ahasenack/motd-news-wasremoved-handling/ubuntu bionic InRelease [15.4 kB]
Hit:4 http://archive.ubuntu.com/ubuntu bionic-updates InRelease
Hit:5 http://archive.ubuntu.com/ubuntu bionic-backports InRelease
Get:6 http://ppa.launchpad.net/ahasenack/motd-news-wasremoved-handling/ubuntu bionic/main amd64 Packages [852 B]
Get:7 http://ppa.launchpad.net/ahasenack/motd-news-wasremoved-handling/ubuntu bionic/main Translation-en [516 B]
Fetched 16.8 kB in 1s (13.7 kB/s)
Reading package lists... Done
root@triage-bionic+18.04:~# apt-get update
Hit:1 http://archive.ubuntu.com/ubuntu bionic InRelease
Hit:2 http://security.ubuntu.com/ubuntu bionic-security InRelease
Hit:3 http://ppa.launchpad.net/ahasenack/motd-news-wasremoved-handling/ubuntu bionic InRelease
Hit:4 http://archive.ubuntu.com/ubuntu bionic-updates InRelease
Hit:5 http://archive.ubuntu.com/ubuntu bionic-backports InRelease
Reading package lists... Done
root@triage-bionic+18.04:~# apt-get install base-files
Reading package lists... Done
Building dependency tree
Reading state information... Done
The following packages were automatically installed and are no longer required:
  acpid bc bcache-tools btrfs-progs btrfs-tools byobu cloud-initramfs-copymods cloud-initramfs-dyn-netconf
  cryptsetup cryptsetup-bin dmeventd ethtool fonts-ubuntu-console htop landscape-common libdevmapper-event1.02.1
  libevent-2.1-6 libisns0 liblvm2app2.2 liblvm2cmd2.02 libmspack0 libreadline5 libutempter0 libxmlsec1
  libxmlsec1-openssl lvm2 mdadm open-iscsi open-vm-tools overlayroot pollinate python3-attr python3-automat
  python3-constantly python3-debconf python3-hyperlink python3-incremental python3-newt python3-pam
  python3-pyasn1-modules python3-service-identity python3-twisted python3-twisted-bin run-one screen sosreport tmux
  update-notifier-common xfsprogs zerofree
Use 'apt autoremove' to remove them.
The following packages will be upgraded:
  base-files
1 upgraded, 0 newly installed, 0 to remove and 0 not upgraded.
Need to get 87.9 kB of archives.
After this operation, 0 B of additional disk space will be used.
Get:1 http://ppa.launchpad.net/ahasenack/motd-news-wasremoved-handling/ubuntu bionic/main amd64 base-files amd64 10.1ubuntu2.11~ppa1 [87.9 kB]
Fetched 87.9 kB in 1s (128 kB/s)
(Reading database ... 71173 files and directories currently installed.)
Preparing to unpack .../base-files_10.1ubuntu2.11~ppa1_am...

Read more...

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

> So, installing motd-news-config (e.g. after or independently from upgrading
> base-files) should always remove /e/d/motd-news.wasremoved if it is present?
> I don't think I'm seeing that though:
>
> root@triage-bionic+18.04:~# ll /etc/default/motd*
> -rw-r--r-- 1 root root 0 Sep 17 22:26 /etc/default/motd-news.wasremoved
...
(install base-files)
...
> root@triage-bionic+18.04:~# !ll
> ll /etc/default/motd*
> -rw-r--r-- 1 root root 0 Sep 17 22:26 /etc/default/motd-news.wasremoved
...
(install motd-news-config)
...
> root@triage-bionic+18.04:~# apt-get install motd-news-config
> Reading package lists... Done
...
> root@triage-bionic+18.04:~# ll /etc/default/motd*
> -rw-r--r-- 1 root root 779 Sep 18 16:57 /etc/default/motd-news
> root@triage-bionic+18.04:~#

The .wasremoved file is gone, did you misread /etc/default/motd-news as /etc/default/motd-news.wasremoved?

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

What did happen there though is that /etc/default/motd-news was installed with ENABLED=0, because the .wasremoved file was there.

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

Any other comments?

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

> The .wasremoved file is gone, did you misread /etc/default/motd-news as /etc/default/motd-news.wasremoved?

As discussed on IRC, yes you are right. I was scanning for a "file not found" error.

It seems a bit surprising to have motd-news appear, but from discussion sounds like that is expected behavior.

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

> It seems a bit surprising to have motd-news appear, but from discussion sounds
> like that is expected behavior.

I understand how confusing this whole situation can be, believe me :)

motd-news appearing is the desired outcome when motd-news-config is installed(*), that's there this file belongs now. It was moved from base-files to motd-news-config, and all this maintainer script complication is to handle this move.

(*) further complicated by some scenarios: a) the user disabled motd-news before, in which case we want to preserve that modification. Or b) if the user removed the file before, in which case we want to install a disabled version of it.

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

Ok, that makes more sense now.

Someone's going to have to draw up a flow chart if we make any more alterations to this package. ;-)

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

Tagging and uploading 6ca97cfce91cc40d8fbf6c5a61f487c5c2659f43:

$ git push pkg upload/9.4ubuntu4.14
Enumerating objects: 26, done.
Counting objects: 100% (26/26), done.
Delta compression using up to 4 threads
Compressing objects: 100% (21/21), done.
Writing objects: 100% (21/21), 2.76 KiB | 1.38 MiB/s, done.
Total 21 (delta 15), reused 0 (delta 0)
To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/base-files
 * [new tag] upload/9.4ubuntu4.14 -> upload/9.4ubuntu4.14

$ dput ubuntu ../base-files_9.4ubuntu4.14_source.changes
Checking signature on .changes
gpg: ../base-files_9.4ubuntu4.14_source.changes: Valid signature from AC983EB5BF6BCBA9
Checking signature on .dsc
gpg: ../base-files_9.4ubuntu4.14.dsc: Valid signature from AC983EB5BF6BCBA9
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading base-files_9.4ubuntu4.14.dsc: done.
  Uploading base-files_9.4ubuntu4.14.tar.xz: done.
  Uploading base-files_9.4ubuntu4.14_source.buildinfo: done.
  Uploading base-files_9.4ubuntu4.14_source.changes: done.
Successfully uploaded packages.

Unmerged commits

6ca97cf... by Andreas Hasenack

changelog

52a955f... by Andreas Hasenack

    - d/motd-news-config.postinst: always remove /e/d/motd-news.wasremoved
      if present

2962249... by Andreas Hasenack

    - d/postinst.in: only consider creating the .wasremoved file in upgrades,
      never fresh installs like in debootstrap for example

989427c... by Andreas Hasenack

    - d/postinst.in: re-arrange the cascading conditionals around the
      creation of the .wasremoved file for better clarity

44d9735... by Andreas Hasenack

  * Fix handling of /e/d/motd-news.wasremoved (LP: #1895302):
    - d/postinst.in: check if ubuntu-server is installed before creating
      the .wasremoved file

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index cf951ad..ef21306 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,17 @@
6+base-files (9.4ubuntu4.14) xenial; urgency=medium
7+
8+ * Fix handling of /e/d/motd-news.wasremoved (LP: #1895302):
9+ - d/postinst.in: check if ubuntu-server is installed before creating
10+ the .wasremoved file
11+ - d/postinst.in: re-arrange the cascading conditionals around the
12+ creation of the .wasremoved file for better clarity
13+ - d/postinst.in: only consider creating the .wasremoved file in upgrades,
14+ never fresh installs like in debootstrap for example
15+ - d/motd-news-config.postinst: always remove /e/d/motd-news.wasremoved
16+ if present
17+
18+ -- Andreas Hasenack <andreas@canonical.com> Wed, 16 Sep 2020 11:03:13 -0300
19+
20 base-files (9.4ubuntu4.13) xenial; urgency=medium
21
22 [ Andreas Hasenack ]
23diff --git a/debian/motd-news-config.postinst b/debian/motd-news-config.postinst
24index 419d326..4e7067f 100644
25--- a/debian/motd-news-config.postinst
26+++ b/debian/motd-news-config.postinst
27@@ -33,9 +33,9 @@ case "$1" in
28 fi
29 if [ -e /etc/default/motd-news.wasremoved ] && [ -e /etc/default/motd-news ]; then
30 sed -i -e 's/^ENABLED=1/# Changed by motd-news-config.postinst:\n# config file was manually removed - disable the service\nENABLED=0/' /etc/default/motd-news
31- rm /etc/default/motd-news.wasremoved
32 fi
33 fi
34+ rm -f /etc/default/motd-news.wasremoved
35 ;;
36
37 abort-upgrade|abort-remove|abort-deconfigure)
38diff --git a/debian/postinst.in b/debian/postinst.in
39index 89ddd29..93e7232 100644
40--- a/debian/postinst.in
41+++ b/debian/postinst.in
42@@ -132,12 +132,16 @@ fi
43 # it does not put back the file with default contents which would
44 # re-enable motd-news
45 motd_news_config="/etc/default/motd-news"
46-if [ ! -e ${motd_news_config} ]; then
47- if [ ! -e ${motd_news_config}.dpkg-remove ]; then
48- if [ ! -e ${motd_news_config}.dpkg-backup ]; then
49- touch ${motd_news_config}.wasremoved
50- fi
51- fi
52+# only in upgrades, never fresh installs like in debootstrap
53+if [ "$2" != "" ] && \
54+ [ ! -e ${motd_news_config} ] && \
55+ [ ! -e ${motd_news_config}.dpkg-remove ] && \
56+ [ ! -e ${motd_news_config}.dpkg-backup ]; then
57+ # The .wasremoved file only matters if ubuntu-server is installed,
58+ # because that's what will pull in motd-news-config
59+ if dpkg -l ubuntu-server 2>/dev/null | grep -q ^i; then
60+ touch ${motd_news_config}.wasremoved
61+ fi
62 fi
63
64 # Manually inject expected maintainer script contents based on dh_systemd_*

Subscribers

People subscribed via source and target branches