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
diff --git a/debian/changelog b/debian/changelog
index eb44a50..3a67c1b 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,17 @@
1base-files (11ubuntu13) groovy; urgency=medium
2
3 * Fix handling of /e/d/motd-news.wasremoved (LP: #1895302):
4 - d/postinst.in: check if ubuntu-server is installed before creating
5 the .wasremoved file
6 - d/postinst.in: re-arrange the cascading conditionals around the
7 creation of the .wasremoved file for better clarity
8 - d/postinst.in: only consider creating the .wasremoved file in upgrades,
9 never fresh installs like in debootstrap for example
10 - d/motd-news-config.postinst: always remove /e/d/motd-news.wasremoved
11 if present
12
13 -- Andreas Hasenack <andreas@canonical.com> Wed, 16 Sep 2020 10:26:50 -0300
14
1base-files (11ubuntu12) groovy; urgency=medium15base-files (11ubuntu12) groovy; urgency=medium
216
3 * d/control: set priority to optional for motd-news-config to avoid17 * d/control: set priority to optional for motd-news-config to avoid
diff --git a/debian/motd-news-config.postinst b/debian/motd-news-config.postinst
index a73427f..0be04fd 100644
--- a/debian/motd-news-config.postinst
+++ b/debian/motd-news-config.postinst
@@ -33,9 +33,9 @@ case "$1" in
33 fi33 fi
34 if [ -e /etc/default/motd-news.wasremoved ] && [ -e /etc/default/motd-news ]; then34 if [ -e /etc/default/motd-news.wasremoved ] && [ -e /etc/default/motd-news ]; then
35 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-news35 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
36 rm /etc/default/motd-news.wasremoved
37 fi36 fi
38 fi37 fi
38 rm -f /etc/default/motd-news.wasremoved
39 ;;39 ;;
4040
41 abort-upgrade|abort-remove|abort-deconfigure)41 abort-upgrade|abort-remove|abort-deconfigure)
diff --git a/debian/postinst.in b/debian/postinst.in
index c347a62..055c914 100644
--- a/debian/postinst.in
+++ b/debian/postinst.in
@@ -130,12 +130,16 @@ fi
130# it does not put back the file with default contents which would130# it does not put back the file with default contents which would
131# re-enable motd-news131# re-enable motd-news
132motd_news_config="/etc/default/motd-news"132motd_news_config="/etc/default/motd-news"
133if [ ! -e ${motd_news_config} ]; then133# only in upgrades, never fresh installs like in debootstrap
134 if [ ! -e ${motd_news_config}.dpkg-remove ]; then134if [ "$2" != "" ] && \
135 if [ ! -e ${motd_news_config}.dpkg-backup ]; then135 [ ! -e ${motd_news_config} ] && \
136 touch ${motd_news_config}.wasremoved136 [ ! -e ${motd_news_config}.dpkg-remove ] && \
137 fi137 [ ! -e ${motd_news_config}.dpkg-backup ]; then
138 fi138 # The .wasremoved file only matters if ubuntu-server is installed,
139 # because that's what will pull in motd-news-config
140 if dpkg -l ubuntu-server 2>/dev/null | grep -q ^i; then
141 touch ${motd_news_config}.wasremoved
142 fi
139fi143fi
140144
141#DEBHELPER#145#DEBHELPER#

Subscribers

People subscribed via source and target branches