[Pkg-systemd-maintainers] Bug#709842: [PATCH] Add new helper dh_installsystemd

[Michael Stapelberg]

Hi Joey,

Thank you for your feedback. Comments inline:

Joey Hess <joeyh at debian.org> writes:
> It seems that despite the "install" in its name, dh_installsystemd does
> not install anything. It does some registration of systemd services
> files already installed by the package's Makefile or by dh_installinit.
> So, this needs to be renamed to dh_systemd.
Done.

> I'm not at all sure that this has a reason to be separate from
> dh_installinit. (Other than the problem that Debian's terminal
> indecision about init systems is turning dh_installinit into a hot
> mess.)
Well, dh_* scripts have a maximum SLOC length, enforced by t/size, also
having a different helper for a different init system is much easier to
understand as you mentioned :).

Another important reason is that packages might only ship a service file
without actually shipping a sysvinit script. This affects not so much
actual daemons which are mandated by policy to ship a sysvinit script,
but for example dbus-activated services.

I also suggest this further down in this email, but if you feel strongly
about extending dh_installinit to cover all these cases, we can
definitely talk about that.

> Your motivating example of multiple service files is not very
> convincing. A package can also, in theory contain multiple init scripts
> or, I suppose, multiple upstart jobs. dh_installinit seems to handle
> this just about as well as dh_installsystemd would, which is to say the
> user has to call the program multiple times with different parameters to
> select different files.
While a package can have multiple init scripts, my point is that there
is no 1:1 relationship between an init script and a systemd unit file.

>> +deb-systemd-helper enable #UNITFILES# >/dev/null || true
>> +deb-systemd-helper enable #UNITFILES# >/dev/null || true
>> +		deb-systemd-helper disable #UNITFILES# >/dev/null
>
> When I saw this, I immediately wondered why it was not using triggers
> for this..
Triggers work well when all files are equal. In our case, service files
have different requirements. Some of them don’t need to be enabled at
all (e.g. static service files, or services which currently in sysvinit
use $service_DISABLED=true in /etc/default/$service).

Is it guaranteed that our trigger ran before postinst is executed? See
also the next paragraph:

>> +=item B<--assume-sysv-present>
>> +
>> +When running B<dh_installsystemd> before B<dh_installinit>, init scripts might
>> +not be installed yet and thus cannot be found by B<dh_installsystemd>. By
>> +specifying B<--assume-sysv-present>, start/stop/restart will be done through
>> +invoke-rc.d, i.e. no systemd-specific code will be generated.
>
> Why would anyone ever want to reorder the commands and use this
> option?
There are cases which require an alias (= symlink) to be present. This
symlink is created by deb-systemd-enable. Take NetworkManager as an
example: its upstream service file is called NetworkManager.service,
whereas the init script is called
/etc/init.d/network-manager. Therefore, using “invoke-rc.d
network-manager start” (note the hyphen and capitalization) will not
work, unless deb-systemd-enable is invoked _before_ invoke-rc.d.

This might also happen with other projects, so we should be able to
handle it.

>> +Note that B<dh> calls B<dh_installsystemd> by default, so in most cases you do
>> +not need to change anything at all.
>
> I don't feel that debhelper commands need to talk about what dh does.
Removed.

>> +Here is an example make target to use in debian/rules:
>> +
>> +    override_dh_installsystemd:
>> +        # The user will enable smartd.service if she wants to run it.
>> +	# Call dh_installsystemd to stop/start the service on upgrades.
>> +        dh_installsystemd --no-enable smartd.service
>
> I don't use dh-specific examples in debhelper command documentation.
Removed.

>> +Note that this command is not idempotent. L<dh_prep(1)> should be called
>> +between invocations of this command. Otherwise, it may cause multiple
>> +instances of the same text to be added to maintainer scripts.
>
> This is sorta in conflict with other parts of the man page that talk
> about running the command multiple times.
I’ve changed this to:

Note that this command is not idempotent. L<dh_prep(1)> should be called
between invocations of this command (with the same arguments). Otherwise, it
may cause multiple instances of the same text to be added to maintainer
scripts.

This whole paragraph was copy-pasted from dh_installinit, so I thought
it was important to mention that the command — when run with the same
arguments — appends the same text multiple times. In case you think it
adds more confusion than it clarifies, I’m happy to remove it entirely.


>> +	"no-also" => \$dh{NO_ALSO},
>
> This option is not documented, and I don't do hidden undocumented
> options.
Added a comment. Note that this is really only as a safe-guard and
programmed very defensively. I don’t expect this to be used, ever.

Again, if you think this shouldn’t be in there, let me know, and I’ll
remove it.

>> +		# Skip template service files like e.g. getty at .service.
>> +		# Enabling, disabling, starting or stopping those services
>> +		# without specifying the instance (e.g. getty at ttyS0.service) is
>> +		# not useful.
>> +		if ($name =~ /\@/) {
>> +			return;
>> +		}
>> +
>> +		# Handle all unit files specified via Also= explicitly.
>> +		# This is not necessary for enabling, but for disabling, as we
>> +		# cannot read the unit file when disabling (it was already
>> +		# deleted).
>> +		my @also = grep { !exists($seen{$_}) } extract_key($name, 'Also');
>> +		$seen{$_} = 1 for @also;
>> +		@args = (@args, @also);
>
> All the above-quoted stuff is really ugly to put into debhelper. Do I
> really want debhelper to need to play catch-up to format changes in
> systemd files? To encode heuristics about getty? I don't think that's
> appropriate.
Note that the unit file format is covered by systemd’s interface
stability promise:
http://www.freedesktop.org/wiki/Software/systemd/InterfaceStabilityPromise/

Therefore I consider it extremely unlikely that this will
break. Unfortunately, there is no better solution than re-implementing
this, because systemd’s own tools rely on already having a running
systemd available

If you feel really strongly about this and we should put dh_systemd into
a separate package, please let us know. We thought it’d be nice for
package maintainers to make things just work™, without modifications to
their packages :).

Also, I think you have misread the comment about getty at .service —
getty is really only an example here. The code deals with template
files, i.e. all files containing an “@” character in their filename.
See also
http://www.freedesktop.org/software/systemd/man/systemd.unit.html
starting from “Optionally, units may be instantiated from a template
file at runtime.”

Therefore, there’s really no heuristics in that code :).

> I'll bet there's a way to handle disabling unit files that have already
> been deleted. Their Also= values could probably be cached, for
> example.
Yeah, but then we’d need to keep even more state on every user’s machine
and keeping state is really ugly. We would like to avoid that.

>> +		# Try to make the path absolute, so that the user can call
>> +		# dh_installsystemd bacula-fd.service
>> +		if ($base eq $name) {
>> +			# NB: This works because @installed_units contains
>> +			# files from precisely one directory.
>> +			my ($full) = grep { basename($_) eq $base } @installed_units;
>> +			if (defined($full)) {
>> +				$name = $full;
>> +			} else {
>> +				warning(qq|Could not find "$name" in the /lib/systemd/system of $package.| .
>> +					qq|This could be a typo, or using Also= with a service file from another package.| .
>> +					qq|Please check carefully that this message is harmless.|);
>
> This is not an appropriate error message for a debhelper command to
> output. The whole use of basenames of service files is unlike how all
> other debhelper commands refer to files in the package build
> directory.
Removed.

>
>> +			} elsif (!$sysv_present) {
>> +				# We need to stop/start before/after the upgrade.
>> +				# The fact that we also try start the service
>> +				# once after installing is just a minor
>> +				# inconvenience (for now).
>
> Huh? Packages should start services when installed unless the policy
> layer of invoke-rc.d says otherwise.
Removed the comment.

> Finally, I notice that this program is missing the new dh PROMISE code,
> so dh will run it for *every* package, even those without any systemd
> service files. That is unlikely to win systemd new friends..
> This might be right, but I'm not sure:
>
> # PROMISE: DH NOOP WITHOUT tmp(etc/systemd)
Added, thanks for the hint.

The new version of the patch is attached.

-- 
Best regards,
Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-add-new-helper-dh_systemd.patch
Type: text/x-diff
Size: 16266 bytes
Desc: not available
URL: <http://alioth-lists.debian.net/pipermail/pkg-systemd-maintainers/attachments/20130610/c3e5ab87/attachment-0001.patch>