Michael Stapelberg wrote: > The raison d'être for dh_installsystemd is the fact that some packages > ship multiple service files but only a single SysV init script, i.e. > there is no 1:1 correlation between systemd unit files and SysV init > scripts. As an example, samba consists of smbd and nmbd, but both are > started using /etc/init.d/samba, whereas with systemd, one might use > smbd.service and nmbd.service instead. For such cases it makes sense to > have a separate helper instead of trying to cram the functionality into > dh_installinit.
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.
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.)
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.
> +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..
> +B<dh_installsystemd> [S<I<debhelper options>>] [B<--no-enable>]
> [B<--restart-after-upgrade>] [B<--no-restart-on-upgrade>]
> [B<--assume-sysv-present>] [S<I<unit file> ...>]
The fact that we have here some of the same options dh_installinit has
to control when daemons get restarted is another red flag.
> +=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?
> +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.
> +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.
> +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.
> +Note that B<dh_installsystemd> should be run after B<dh_installinit> so that
> it
> +can detect corresponding SysV init scripts. The default sequence in B<dh>
> does
> +the right thing, this note is only relevant when you are calling
> +B<dh_installsystemd> manually.
The need to repeatedly mention this, and this rather tight binding
between the commands again makes me wonder if this should be part of
dh_installinit.
> + "no-also" => \$dh{NO_ALSO},
This option is not documented, and I don't do hidden undocumented options.
> +# Extracts the Also= or Alias= line(s) from a unit file.
> +# In case this produces horribly wrong results, you can pass --no-also, but
> +# that should really not be necessary. Please report bugs to
> +# pkg-systemd-maintainers.
> +sub extract_key {
> + my ($unit_path, $key) = @_;
> + my @values;
> + my $fh;
> +
> + if ($dh{NO_ALSO}) {
> + return @values;
> + }
> +
> + if (!open($fh, '<', $unit_path)) {
> + warning("Cannot open($unit_path) for extracting the Also=
> line(s)");
> + return;
> + }
> + while (my $line = <$fh>) {
> + chomp($line);
> +
> + if ($line =~ /^\s*$key=(.+)$/i) {
> + @values = (@values, shellwords($1));
> + }
> + }
> + close($fh);
> + return @values;
> +}
> + # Skip template service files like e.g. getty@.service.
> + # Enabling, disabling, starting or stopping those services
> + # without specifying the instance (e.g. getty@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.
This suggests that you are relying on a debhelper command to handle
things that your package's infrastructure should be handling. Ideally
with triggers so the debhelper command isn't necessary at all, but at
least by providing an interface that can be used by the debhelper
command, as well as by packagers who do not use debhelper.
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.
> + # 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.
> + } 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.
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)
--
see shy jo
signature.asc
Description: Digital signature
