1. The excessive nesting.
I agree that it is annoying to read several layers of nested ifs. As an individual issue, we don't think this is a blocker; however if we decide to rewrite this function for this SRU for the below reasons, we can use the early return like you suggest. If not, then we will simplify this for next release.
2. The check for deb-systemd-helper existing.
There's a longer story as to why we thought this check was a good idea at the time, but the end result is that we were mistaken and it is superfluous. We don't think this check hurts anything but it doesn't help either. We can remove it in our next release unless you think it blocks this one.
3. The check for systemd existing.
This is present because we were copying a pattern that deb-systemd-helper uses in its autosnippets. Whenever d-s-h appends a snippet that uses systemctl, it makes this check first. For example, a snippet that is added to our postinst by d-s-h:
# Automatically added by dh_systemd_start
if [ -d /run/systemd/system ]; then
systemctl --system daemon-reload >/dev/null || true deb-systemd-invoke start ua-timer.timer >/dev/null || true
fi
# End automatically added section
However, it turns out our use of systemctl in this case is also unnecessary. We don't need to run systemctl stop in this case because ua-timer.timer will never have been started when this chunk of code runs. Again, this shouldn't hurt anything. We can remove it in our next release unless you think it blocks this one.
4. Running deb-systemd-helper enable is confusing.
The use of enable was intentional. We attempted to explain in the comment above that line. The reason is that we want to take advantage of d-s-h's autosnippet for enabling/updating config for ua-timer.timer which appends the following to the bottom of postinst:
# Automatically added by dh_systemd_enable
# This will only remove masks created by d-s-h on package removal.
deb-systemd-helper unmask ua-timer.timer >/dev/null || true
# was-enabled defaults to true, so new installations run enable.
if deb-systemd-helper --quiet was-enabled ua-timer.timer; then
# Enables the unit on first installation, creates new
# symlinks on upgrades if the unit file has changed. deb-systemd-helper enable ua-timer.timer >/dev/null || true
else
# Update the statefile to add new symlinks (if any), which need to be
# cleaned up on purge. Also remove old symlinks. deb-systemd-helper update-state ua-timer.timer >/dev/null || true
fi
# End automatically added section
The chunk of code that we are talking about happens before this d-s-h autosnippet, and the intent is to prevent this d-s-h snippet from running "enable".
By running "enable" and then "disable" we update the d-s-h state such that "was-enabled" will fail and so that snippet will not run "enable".
We acknowledge that this solution is confusing, to put it charitably. We've evaluated a number of alternatives:
This is the same solution but more consistent in its use of d-s-h for tooling. We think this reads better but is not a significant improvement on what is currently there. We'll simplify to this solution in the next release.
4.2. Rely on d-s-h implementation details
touch /var/lib/systemd/deb-systemd-helper-enabled/timers.target.wants/ua-timer.timer
This avoids needlessly enabling and disabling, but relies on the implementation detail of d-s-h that it won't enable if this file is present. It seems more fragile than the existing solution.
4.3 Rely on incidental behavior of update-state
deb-systemd-helper update-state ua-timer.timer >/dev/null || true
This command would set the d-s-h state appropriately to avoid enabling the timer, but it is not the command's intended purpose. We think this is at least as confusing as the existing solution and could be fragile as well.
4.4 We stop relying on `dh_systemd_enable -pubuntu-advantage-tools ua-timer.timer` and instead write the d-s-h enable code ourselves in postinst (and the other operations it provides in preinst, prerm, postrm).
This shifts all of the ua-timer.timer management responsibility from d-s-h to us, the ua devs, and seems like an unnecessary burden just to make this use case more straightforward.
A note on this use case: This snippet of code is only run for users who have previously disabled ua-messaging.timer, which has only existed for ~6 months. We expect the number of users this affects to be very low. With this snippet, we are trying to carry their preference forward to the new timer, and it works as written. We're not sure it is worth too much more effort to make the solution more efficient or cleaner for this small edge case.
Overall, all of the issues you've pointed out are real, but we don't think any negatively impact the package in a significant way that is worth holding up this release. We can fix them in the next release. Please let us know if you disagree.
Thank you Steve, you raise several good points.
1. The excessive nesting.
I agree that it is annoying to read several layers of nested ifs. As an individual issue, we don't think this is a blocker; however if we decide to rewrite this function for this SRU for the below reasons, we can use the early return like you suggest. If not, then we will simplify this for next release.
2. The check for deb-systemd-helper existing.
There's a longer story as to why we thought this check was a good idea at the time, but the end result is that we were mistaken and it is superfluous. We don't think this check hurts anything but it doesn't help either. We can remove it in our next release unless you think it blocks this one.
3. The check for systemd existing.
This is present because we were copying a pattern that deb-systemd-helper uses in its autosnippets. Whenever d-s-h appends a snippet that uses systemctl, it makes this check first. For example, a snippet that is added to our postinst by d-s-h:
# Automatically added by dh_systemd_start
deb-systemd- invoke start ua-timer.timer >/dev/null || true
if [ -d /run/systemd/system ]; then
systemctl --system daemon-reload >/dev/null || true
fi
# End automatically added section
However, it turns out our use of systemctl in this case is also unnecessary. We don't need to run systemctl stop in this case because ua-timer.timer will never have been started when this chunk of code runs. Again, this shouldn't hurt anything. We can remove it in our next release unless you think it blocks this one.
4. Running deb-systemd-helper enable is confusing.
The use of enable was intentional. We attempted to explain in the comment above that line. The reason is that we want to take advantage of d-s-h's autosnippet for enabling/updating config for ua-timer.timer which appends the following to the bottom of postinst:
# Automatically added by dh_systemd_enable
# This will only remove masks created by d-s-h on package removal.
deb-systemd-helper unmask ua-timer.timer >/dev/null || true
# was-enabled defaults to true, so new installations run enable.
deb-systemd- helper enable ua-timer.timer >/dev/null || true
deb-systemd- helper update-state ua-timer.timer >/dev/null || true
if deb-systemd-helper --quiet was-enabled ua-timer.timer; then
# Enables the unit on first installation, creates new
# symlinks on upgrades if the unit file has changed.
else
# Update the statefile to add new symlinks (if any), which need to be
# cleaned up on purge. Also remove old symlinks.
fi
# End automatically added section
The chunk of code that we are talking about happens before this d-s-h autosnippet, and the intent is to prevent this d-s-h snippet from running "enable".
By running "enable" and then "disable" we update the d-s-h state such that "was-enabled" will fail and so that snippet will not run "enable".
We acknowledge that this solution is confusing, to put it charitably. We've evaluated a number of alternatives:
4.1. Straightforward simplification
deb-systemd-helper enable $UA_TIMER_NAME > /dev/null 2>&1 || true
deb-systemd-helper disable $UA_TIMER_NAME > /dev/null 2>&1 || true
This is the same solution but more consistent in its use of d-s-h for tooling. We think this reads better but is not a significant improvement on what is currently there. We'll simplify to this solution in the next release.
4.2. Rely on d-s-h implementation details systemd/ deb-systemd- helper- enabled/ timers. target. wants/ua- timer.timer
touch /var/lib/
This avoids needlessly enabling and disabling, but relies on the implementation detail of d-s-h that it won't enable if this file is present. It seems more fragile than the existing solution.
4.3 Rely on incidental behavior of update-state
deb-systemd-helper update-state ua-timer.timer >/dev/null || true
This command would set the d-s-h state appropriately to avoid enabling the timer, but it is not the command's intended purpose. We think this is at least as confusing as the existing solution and could be fragile as well.
4.4 We stop relying on `dh_systemd_enable -pubuntu- advantage- tools ua-timer.timer` and instead write the d-s-h enable code ourselves in postinst (and the other operations it provides in preinst, prerm, postrm).
This shifts all of the ua-timer.timer management responsibility from d-s-h to us, the ua devs, and seems like an unnecessary burden just to make this use case more straightforward.
A note on this use case: This snippet of code is only run for users who have previously disabled ua-messaging.timer, which has only existed for ~6 months. We expect the number of users this affects to be very low. With this snippet, we are trying to carry their preference forward to the new timer, and it works as written. We're not sure it is worth too much more effort to make the solution more efficient or cleaner for this small edge case.
Overall, all of the issues you've pointed out are real, but we don't think any negatively impact the package in a significant way that is worth holding up this release. We can fix them in the next release. Please let us know if you disagree.