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

Proposed by Andreas Hasenack on 2020-09-15
Status: Merged
Approved by: Andreas Hasenack on 2020-09-16
Approved revision: c7a8fc156ef1cf3a752e7ab383d7ffc2fbd2e60d
Merged at revision: c7a8fc156ef1cf3a752e7ab383d7ffc2fbd2e60d
Proposed branch: ~ahasenack/ubuntu/+source/base-files:groovy-handle-was-removed-1895302
Merge into: ubuntu/+source/base-files:ubuntu/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
Christian Ehrhardt  2020-09-15 Pending
Canonical Server Team 2020-09-15 Pending
Review via email: mp+390766@code.launchpad.net

This proposal supersedes a proposal from 2020-09-15.

Description of the change

Handle the creation and removal of the /etc/default/motd-news.wasremoved file in maintainer scripts.

I have two choices here:
a) handle the creation and removal of that file (this MP)
b) give up trying to handle that file, and just cleanup: https://git.launchpad.net/~ahasenack/ubuntu/+source/base-files/commit/?h=groovy-drop-wasremoved&id=1c8983d04fdadf831976cbe42259a4b9b5b79d0b

To recap, the idea behind /e/d/motd-news.wasremoved was to handle the case where:
- user had base-files and ubuntu-server from before the split, meaning /e/d/motd-news is a config file part of base-files
- motd-news is enabled (ENABLED=1 in /e/d/motd-news)
- in an erroneous attempt to disable the service, the user removes the /e/d/motd-news file. ENABLED doesn't get set, and the motd-news script just exits

In the above scenario, without the .wasremoved feature, if the user upgraded to the updated packages with the spit, then /e/d/motd-news would be reinstated because it's now part of the motd-news-config package. This would re-enable the motd-news feature, something we want to be careful about.

The implementation suffers from some problems, though:
- a fresh install of base-files, like done via debootstrap, will create the .wasremoved file. If you then install motd-news-config (via apt install ubuntu-server, for example), the motd-news-config postinst fill find the .wasremoved file, see it's a fresh install of motd-news-config, and disable motd-news. This is the current situation in the groovy lxd images, for example
- if you just have base-files installed, no motd-news-config, and then reinstall or upgrade base-files, the .wasremoved will be created

To address those, this MP (choice (a)) does:
- motd-news-config postinst: always remove the .wasremoved file in configure if found, regardless if /etc/default/motd-news was sed'ed or not, or if we are upgrading or on a first install
- base-files postinst: guard the creation of .wasremoved with:
  - Only during an upgrade
  - Only if ubuntu-server is installed (via a dpkg -l check)

If a user again decides to erroneously disable motd-news by removing /e/d/motd-news, then an upgrade of base-files, motd-news-config and ubuntu-server will go through the creation and removal of the .wasremoved file, but no new /e/d/motd-news config file will be created or changed, and motd-news will at least remain disabled. But it's extra unnecessary churn, as the corner case it handles only happens when upgrading from before the motd-news-config split.

The dpkg -l check for ubuntu-server relies on ubuntu-server depending (and not just recommending) on motd-news-config. I'm a bit afraid of the dpkg -l call in base-files' postinst:
- Locking issues?
- I'm checking for ^i, should I be looking for other cases? Other flags?
- At least dpkg is already installed, as I see other calls to it in the same postinst

If we find other issues in these maintainer scripts regarding the motd-news-config split, or introduce new ones with these changes, it will get harder and harder to fix each time. That's why I also have to consider simply giving up and living with the corner case we tried to handle in the beginning, and that is this diff: https://git.launchpad.net/~ahasenack/ubuntu/+source/base-files/commit/?h=groovy-drop-wasremoved&id=1c8983d04fdadf831976cbe42259a4b9b5b79d0b Just note that in this case, the corner case scenario will leave motd-news enabled when it was supposed to be disabled.

I have PPAs for both scenarios, with groovy and focal packages (just not yet bionic or xenial):

Give up handling of .wasremoved: https://launchpad.net/~ahasenack/+archive/ubuntu/motd-news-drop-wasremoved/

Try to fix handling of .wasremoved: https://launchpad.net/~ahasenack/+archive/ubuntu/motd-news-wasremoved-handling/

May the best one win :)

To post a comment you must log in.
Christian Ehrhardt  (paelzer) wrote :

I'm for handling it properly and like the approach.

There will always be one issue more with it, but at least the known ones are solved if we do it this way .. :-/

The former had two inline comments that would be useful to address.

Andreas Hasenack (ahasenack) wrote :

Thanks for the comments. I addressed them, but had to force push because I reconstruct my changelog from commit messages, and I also noticed I had forgotten to link the LP bug

Andreas Hasenack (ahasenack) wrote :

Doing a final round of testing before uploading

Andreas Hasenack (ahasenack) wrote :

Verified debootstrap on groovy is fine now

Andreas Hasenack (ahasenack) wrote :

Focal test was also good. Preparing the groovy upload.

Andreas Hasenack (ahasenack) wrote :

Tagging and uploading c7a8fc156ef1cf3a752e7ab383d7ffc2fbd2e60d

$ git push pkg upload/11ubuntu13
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.82 KiB | 577.00 KiB/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/11ubuntu13 -> upload/11ubuntu13

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

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 eb44a50..3a67c1b 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,17 @@
6+base-files (11ubuntu13) groovy; 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 10:26:50 -0300
19+
20 base-files (11ubuntu12) groovy; urgency=medium
21
22 * d/control: set priority to optional for motd-news-config to avoid
23diff --git a/debian/motd-news-config.postinst b/debian/motd-news-config.postinst
24index a73427f..0be04fd 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 c347a62..055c914 100644
40--- a/debian/postinst.in
41+++ b/debian/postinst.in
42@@ -130,12 +130,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 #DEBHELPER#

Subscribers

People subscribed via source and target branches