Merge ~ahasenack/ubuntu/+source/base-files:groovy-motd-news-config-split into ubuntu/+source/base-files:ubuntu/devel
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Andreas Hasenack on 2020-08-13 | ||||
Approved revision: | de0f896184fa1b0569ccb57207748e7a24fc6b85 | ||||
Merged at revision: | de0f896184fa1b0569ccb57207748e7a24fc6b85 | ||||
Proposed branch: | ~ahasenack/ubuntu/+source/base-files:groovy-motd-news-config-split | ||||
Merge into: | ubuntu/+source/base-files:ubuntu/devel | ||||
Diff against target: |
169 lines (+106/-2) 7 files modified
debian/base-files.maintscript (+1/-0) debian/changelog (+23/-0) debian/control (+13/-1) debian/motd-news-config.install (+1/-0) debian/motd-news-config.postinst (+55/-0) debian/postinst.in (+13/-0) debian/rules (+0/-1) |
||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Christian Ehrhardt | 2020-08-06 | Approve on 2020-08-12 | |
Canonical Server Core Reviewers | 2020-08-06 | Pending | |
Review via email:
|
Description of the change
Another attempt at this motd-news-config split, after vorlon's feedback to make motd-news-config a base-files package instead of a new source.
PPA with test builds (no ~ppaN suffix because of the precise breaks/replaces I have all over the place): https:/
The overall plan is to have motd-news only enabled on server systems after this upgrade. What it does:
- the /etc/default/
- ubuntu-server has a dependency on motd-news-config
- new base-files breaks old ubuntu-server, forcing an upgrade of ubuntu-server if it is installed
The versions used in these breaks/replaces have to be very precise for this to work, so between now and whenever these are uploaded, I'll check again.
For now in groovy we have:
base-files 11ubuntu10
ubuntu-meta 1.452
Therefore:
ubuntu-server 1.453:
Depends: motd-news-config
base-files 11ubuntu11:
Breaks/Replaces: ubuntu-server (<< 1.453)
rm_conffile /etc/default/
motd-news-config 11ubuntu11:
Breaks/Replaces: base-files (<< 11ubuntu11)
For the PPA ubuntu-meta package, I committed directly to src:ubuntu-meta instead of running its ./update script, as I couldn't convince that script to use a branch of mine instead of the official ubuntu seeds repository. The seeds change is in https:/
Here are the tests I've been running:
a) base-files installed, ubuntu-server installed, unmodified /e/d/motd-news
apt install base-files
- upgrades ubuntu-server
- installs motd-news-config
- /e/d/motd-news remains, motd-news remains enabled
b) base-files installed, ubuntu-server installed, modified /e/d/motd-news
apt install base-files
- upgrades ubuntu-server
- installs motd-news-config
- /e/d/motd-news remains with the original modification
c) base-files installed, ubuntu-server not installed, unmodified /e/d/motd-news
apt install base-files
- upgrades base-files
- removes /e/d/motd-news
- motd-news is disabled
d) base-files installed, ubuntu-server not installed, modified /e/d/motd-news
apt install base-files
- upgrades base-files
- /e/d/motd-news gets renamed to backup
- motd-news is disabled
e) removing motd-news-config will also remove ubuntu-server (since it's a depends, and not a recommends)
f) upgrading just ubuntu-server should pull motd-news-config in, and force-upgrade base-files
g) Removing motd-news-server leaves /e/d/motd-news around; purging motd-news-server removes the /e/d/motd-news config file
Andreas Hasenack (ahasenack) wrote : | # |
> What do you think, does this require a bit more maintscript magic to be covered as well?
Heh, the config file is coming back because now it belongs to another package, which was never installed before (motd-news-config).
Do you have any idea what this maintscript magic would look like?
Is there a sane way motd-news-config could tell that the file was supposed to be there, but isn't? And then what would it do, place the new one in there with ENABLED set to 0?
Christian Ehrhardt (paelzer) wrote : | # |
How to do it:
What to do:
I wondered about that myself - there will be no perfect answer - but I think after placing the default file on install AND detecting this case of a formerly removed config-file the following should do it.
motd-news-
if [ -e /etc/default/
sed -e 's/ENABLED=
rm /etc/default/
fi
You'd need to check if that orders right to have motd-news.postinst (the one of the new version) executed before motd-news-
base-files.
# user removed the full file, mark that to keep service disabled on conffile take over
if [ -e /etc/default/
touch /etc/default/
fi
The above is untested, but this is what I'd have thought might help to avoid this issue.
Did you get any other feedback if/how to handle that situation?
Christian Ehrhardt (paelzer) wrote : | # |
what I meant for ordering: The new base-files.postinst will be called with "configure $oldver" before the install fully completes - just if this happens before moted-news.
Andreas Hasenack (ahasenack) wrote : | # |
> Did you get any other feedback if/how to handle that situation?
xnox said "it's hard", steve didn't comment yet. Will ping again today.
Andreas Hasenack (ahasenack) wrote : | # |
> base-files.
> # user removed the full file, mark that to keep service disabled on conffile take over
> if [ -e /etc/default/
> touch /etc/default/
> fi
I'm assuming you meant "! -e" in the check above, i.e.:
- before rm_conffile kicks in (#DEBHELPER#)
- if /e/d/motd-news isn't there, then it's because the user removed it
I have this in debian/postinst.in:
(...)
# special case of having /etc/default/
# signal the motd-news-config package that this happened, so that
# it does not put back the file with default contents which would
# re-enable motd-news
motd_config=
ls -la "$motd_config" || :
if [ ! -e /etc/default/
touch /etc/default/
fi
#DEBHELPER#
The "ls -la" is a debug statement.
I start with only base-files 11ubuntu10 installed (the current groovy version), and an unmodified /etc/default/
$ sudo dpkg -i ../*.deb 2>&1 | tee ../dpkg-i.log
(Reading database ... 36078 files and directories currently installed.)
Preparing to unpack .../base-
Warning: Stopping motd-news.service, but it can still be activated by:
motd-news.timer
Unpacking base-files (11ubuntu11) over (11ubuntu10) ...
Selecting previously unselected package motd-news-config.
Preparing to unpack .../motd-
Unpacking motd-news-config (11ubuntu11) ...
Setting up base-files (11ubuntu11) ...
+ [ ! -e /etc/dpkg/
+ [ configure = configure ]
+ [ 11ubuntu10 = ]
+ [ ! -d /var/lib/dpkg ]
+ [ ! -f /var/lib/
+ [ ! -f /usr/info/dir ]
+ [ ! -f /usr/share/info/dir ]
+ [ configure = configure ]
+ [ 11ubuntu10 != ]
+ rm -f /var/lib/
+ [ -x /usr/lib/
+ update_
+ [ -f /etc/profile ]
+ md5sum /etc/profile
+ cut -f 1 -d
+ md5=9d5ee341492
+ grep -q 9d5ee3414928702
+ cmp -s /usr/share/
+ update_
+ [ -f /root/.profile ]
+ md5sum /root/.profile
+ cut -f 1 -d
+ md5=d68ce7c7d7d
+ grep -q d68ce7c7d7d2bb7
+ dpkg --compare-versions 11ubuntu10 lt-nl 7.7
+ motd_config=
+ ls -la /etc/default/
ls: cannot access '/etc/default/
+ :
+ [ ! -e /etc/default/
+ touch /etc/default/
+ [ configure = configure ]
+ deb-systemd-helper unmask motd-news.timer
+ deb-systemd-helper --quiet was-enabled motd-news.timer
+ deb-systemd-helper enable motd-news.timer
+ [ configure = configure ]
+ [ -d /run/systemd/system ]
+ systemctl --system daemon-reload
+ deb-systemd-invoke start motd-news.timer
+ [ configure = configure ]
+ [ -d /run/systemd/system ]
+ systemctl --system daemon-reload
+ deb-systemd-invoke start motd-news.service
motd-news.service is a disabled or a static uni...
Andreas Hasenack (ahasenack) wrote : | # |
Ok, handling those two cases as well now (.dpkg-remove and .dpkg-bak), pushed. I'll have to create a new ppa (guess: zomg-3), as I don't want to complicate testing by using ~ppaN suffixes, given all the exact breaks/replaces I have in these packages.
Andreas Hasenack (ahasenack) wrote : | # |
Had a typo in postinst, preparing zomg-4...
Andreas Hasenack (ahasenack) wrote : | # |
I added these two extra tests:
h) base-files installed, ubuntu-server installed, removed /e/d/motd-news
- apt install base-files
- upgrades base-files, upgrades ubuntu-server, installs motd-news-config
- /e/d/motd-news is installed with ENABLED=0
i) base-files installed, ubuntu-server NOT installed, removed e/d/motd-news
- apt install base-files
- base-files is upgraded
- no /e/d/motd-news is installed, motd-news remains disabled
Packages from zomg-4 passed all, but (i) left a lingering /etc/default/
Andreas Hasenack (ahasenack) wrote : | # |
Ok, it's because the removal is done in motd-news-config's postinst, but since there was no motd-news-config package installed (because there was no ubuntu-server installed before), nothing removed the .wasremoved file :/
Andreas Hasenack (ahasenack) wrote : | # |
Should I add a check to see if ubuntu-server is installed before touching that .wasremoved file? Like dpkg -s ubuntu-server, and only if it's installed and the other conditions match, create the .wasremoved file? This is getting complicated...
Andreas Hasenack (ahasenack) wrote : | # |
Ok, Steve came back and said:
<vorlon> ahasenack: hi, managing to reply before you drop off IRC for the day; I think the corner case in question is acceptable to not address, but needs to be called out in the SRU
<vorlon> (as "regression potential")
I then mentioned the attempt above at fixing this corner case, and he analyzed the changes, and said:
<vorlon> ahasenack: yeah, so I'm vaguely inclined to leave this corner case unfixed but called out in the SRU, rather than add additional complicated maintscript handling; but I wouldn't block the SRU if you do add the maintscript stuff
I fear that this complicated handling in maintainerscripts might introduce more bugs. Let's try without it, and call out the corner case you found in the bug?
Andreas Hasenack (ahasenack) wrote : | # |
I pushed 6aaff4e6f2930bb
Christian Ehrhardt (paelzer) wrote : | # |
While it isn't common dpkg -l should be safe as it is "essential: Yes".
The line you added isn't super complex and should get the issue of the lingering file solved.
The only thing I'm wondering is - would at this stage the first entry of dpkg -l being "Desired state" already be "i" for motd-news-config itself.
If it isn't then keep things as-is and at least I'm +1 on it.
But if it is "i" then checking directly for motd-news-config ready more logically and would cover any awkward dependency messing that prevents it from being installed despite ubuntu-server being present (none come to mind right now, but you know to be on the safe side).
Christian Ehrhardt (paelzer) wrote : | # |
To be clear - (if you prefer that) I'd also be ok to leave "6aaff4e6" out and list it in regression risk. But I see no big risk in keeping "6aaff4e6" either.
Andreas Hasenack (ahasenack) wrote : | # |
I feel like walking a fine line and risking introducing worse regressions than the ones I'm trying to handle :)
About dpkg -l, I've seen at least two ways to check if a package is installed. In postfix[1], we have:
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_
In openssl[2], dpkg -s is used:
dpkg -s <list-of-
While testing the "dpkg -l ubuntu-server" idea, I was at first grepping for ^ii, but turns out ubuntu-server is in a iU state (installed, unpacked), so I switched to ^i. I haven't checked the motd-news-config state yet.
The other thing that bothers me in this fine line I'm walking is that this latest testing was on groovy, but I have to do this all the way back to xenial, and in xenial debhelper isn't even used.
1. https:/
2. https:/
Andreas Hasenack (ahasenack) wrote : | # |
I filled in the SRU template in the base-files bug, and slightly changed the wording we add to /e/d/motd-news when the config file isn't there during the upgrade. I also added an extra check for the presence of the file before sedding it, just to be safe.
Andreas Hasenack (ahasenack) wrote : | # |
Am going to test this one last time in zomg-5
Andreas Hasenack (ahasenack) wrote : | # |
Repeated the groovy tests with the zomg-5 ppa, all worked as expected from (a) to (i), where in (i) the /etc/default/
Andreas Hasenack (ahasenack) wrote : | # |
Tagging and uploading de0f896184fa1b0
$ git push pkg upload/11ubuntu11
Enumerating objects: 45, done.
Counting objects: 100% (45/45), done.
Delta compression using up to 4 threads
Compressing objects: 100% (38/38), done.
Writing objects: 100% (39/39), 5.97 KiB | 381.00 KiB/s, done.
Total 39 (delta 26), reused 0 (delta 0)
To ssh://git.
* [new tag] upload/11ubuntu11 -> upload/11ubuntu11
$ dput ubuntu ../base-
Checking signature on .changes
gpg: ../base-
Checking signature on .dsc
gpg: ../base-
Uploading to ubuntu (via ftp to upload.ubuntu.com):
Uploading base-files_
Uploading base-files_
Uploading base-files_
Uploading base-files_
Successfully uploaded packages.
Great testing as always. motd-news which at first achieves the same thing (motd news is off)
I was not looking at re-running your tests, instead I wondered "what could happen" and a less surgical blunt user came to mind that just deleted /etc/default/
I tested that on ubuntu-server and without ubuntu-server.
Without Ubuntu-server it is ok, the file does not come back and stays disabled.
But with Ubuntu-server installed there is nothing rm_conffile of base-files does.
Therefore no artifact is left, and due to that the default file is placed (again) and thereby re-enabling the service :-/
# Working (default)
root@g2:~# bash -x /etc/update- motd.d/ 50-motd- news motd-news ']' motd-news /motd.ubuntu. com /motd.ubuntu. com ']' var/cache/ motd-news motd-news ']'
+ '[' -r /etc/default/
+ . /etc/default/
++ ENABLED=1
++ URLS=https:/
++ WAIT=5
+ '[' 1 = 1 ']'
+ '[' -n https:/
+ '[' -n 5 ']'
+ '[' -n '' ']'
+ CACHE=/
+ '[' '' = --force ']'
+ '[' '' '!=' 1 ']'
+ '[' -r /var/cache/
++ id -u
+ '[' 0 -eq 0 ']'
+ :
+ exit 0
# Remove file (disabled)
root@g2:~# rm /etc/default/ motd-news motd.d/ 50-motd- news motd-news ']'
root@g2:~# bash -x /etc/update-
+ '[' -r /etc/default/
+ '[' '' = 1 ']'
+ exit 0
# Upgrade re-enables
root@g2:~# add-apt-repository ppa:ahasenack/ zomg-2
More info: https:/ /launchpad. net/~ahasenack/ +archive/ ubuntu/ zomg-2
Press [ENTER] to continue or Ctrl-c to cancel adding it.
Hit:1 http:// archive. ubuntu. com/ubuntu groovy InRelease archive. ubuntu. com/ubuntu groovy-updates InRelease archive. ubuntu. com/ubuntu groovy-backports InRelease ppa.launchpad. net/ahasenack/ zomg-2/ ubuntu groovy InRelease [17.5 kB] security. ubuntu. com/ubuntu groovy-security InRelease ppa.launchpad. net/ahasenack/ zomg-2/ ubuntu groovy/main amd64 Packages [3772 B] ppa.launchpad. net/ahasenack/ zomg-2/ ubuntu groovy/main Translation-en [1056 B] ppa.launchpad. net/ahasenack/ zomg-2/ ubuntu groovy/main amd64 ubuntu-server amd64 1.453 [46.8 kB] ppa.launchpad. net/ahasenack/ zomg-2/ ubuntu groovy/main amd64 base-files amd64 11ubuntu11 [88.6 kB] ppa.launchpad. net/ahasenack/ zomg-2/ ubuntu groovy/main amd64 motd-news-config all 11ubuntu11 [32.5 kB] ppa.launchpad. net/ahasenack/ zomg-2/ ubuntu groovy/main amd64 ubuntu-minimal amd64 1.453 [46.9 ...
Hit:2 http://
Hit:3 http://
Get:4 http://
Hit:5 http://
Get:6 http://
Get:7 http://
Fetched 22.3 kB in 1s (27.7 kB/s)
Reading package lists... Done
root@g2:~#
root@g2:~#
root@g2:~# apt upgrade
Reading package lists... Done
Building dependency tree
Reading state information... Done
Calculating upgrade... Done
The following package was automatically installed and is no longer required:
libfreetype6
Use 'apt autoremove' to remove it.
The following NEW packages will be installed:
motd-news-config
The following packages will be upgraded:
base-files ubuntu-minimal ubuntu-server ubuntu-standard
4 upgraded, 1 newly installed, 0 to remove and 0 not upgraded.
Need to get 262 kB of archives.
After this operation, 45.1 kB of additional disk space will be used.
Do you want to continue? [Y/n] Y
Get:1 http://
Get:2 http://
Get:3 http://
Get:4 http://