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

[Andreas Hasenack]

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 :)

[Christian Ehrhardt  (paelzer)]

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)]

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)]

Doing a final round of testing before uploading

[Andreas Hasenack (ahasenack)]

Verified debootstrap on groovy is fine now

[Andreas Hasenack (ahasenack)]

Focal test was also good. Preparing the groovy upload.

[Andreas Hasenack (ahasenack)]

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.

diff --git a/debian/changelog b/debian/changelog
index eb44a50..3a67c1b 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,17 @@
+base-files (11ubuntu13) groovy; urgency=medium
+
+  * Fix handling of /e/d/motd-news.wasremoved (LP: #1895302):
+    - d/postinst.in: check if ubuntu-server is installed before creating
+      the .wasremoved file
+    - d/postinst.in: re-arrange the cascading conditionals around the
+      creation of the .wasremoved file for better clarity
+    - d/postinst.in: only consider creating the .wasremoved file in upgrades,
+      never fresh installs like in debootstrap for example
+    - d/motd-news-config.postinst: always remove /e/d/motd-news.wasremoved
+      if present
+
+ -- Andreas Hasenack <[e-mail]>  Wed, 16 Sep 2020 10:26:50 -0300
+
 base-files (11ubuntu12) groovy; urgency=medium
 
   * 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
           fi
           if [ -e /etc/default/motd-news.wasremoved ] && [ -e /etc/default/motd-news ]; then
               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
-              rm /etc/default/motd-news.wasremoved
           fi
       fi
+      rm -f /etc/default/motd-news.wasremoved
     ;;
 
     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
 # it does not put back the file with default contents which would
 # re-enable motd-news
 motd_news_config="/etc/default/motd-news"
-if [ ! -e ${motd_news_config} ]; then
-  if [ ! -e ${motd_news_config}.dpkg-remove ]; then
-    if [ ! -e ${motd_news_config}.dpkg-backup ]; then
-      touch ${motd_news_config}.wasremoved
-    fi
-  fi
+# only in upgrades, never fresh installs like in debootstrap
+if [ "$2" != "" ] && \
+   [ ! -e ${motd_news_config} ] && \
+   [ ! -e ${motd_news_config}.dpkg-remove ] && \
+   [ ! -e ${motd_news_config}.dpkg-backup ]; then
+      # The .wasremoved file only matters if ubuntu-server is installed,
+      # because that's what will pull in motd-news-config
+      if dpkg -l ubuntu-server 2>/dev/null | grep -q ^i; then
+        touch ${motd_news_config}.wasremoved
+      fi
 fi
 
 #DEBHELPER#