packaging: use templates for relevant systemd units

[morphis]

To make packaging across different distributionis a bit easier we
now use templates for some of our systemd units and replace things
which are in a different location with sed at build time.

[Conan-Kudo]

This could be slightly simplified.

1. @SNAP_BINARY@ is gratuitous and unnecessary. Currently, it is /usr/bin/snap everywhere. However, if you want to template that, then I would suggest @bindir@/snap instead.

2. Same goes for @SNAPD_BINARY@, though this change is necessary. I'd suggest @libexecdir@/snapd/snapd instead.

3. For consistency with the code, I would rename @SNAP_BASEDIR@ to @SNAP_MOUNTDIR@, to correspond to similar references in snap-confine and snapd.

[zyga]

I agree with @Conan-Kudo review

[morphis]

@Conan-Kudo @zyga Update implementation according to your comments.

[Conan-Kudo]

Looks good to me! 👍

[mvo5]

Looks good, some questions though.

[mvo5]

We might consider putting the sed details into a Makefile in data/systemd - and then call make -C data/systemd SNAPD_ENVIRONMENT_FILE=foo libexecdir=bar SNAP_MOUNTDIR=foobar here. This avoid that we duplicate the sed rules in all the packaging we do.

[morphis]

Makes sense. Will add this.

[mvo5]

We also need to add this to the packaging/ubuntu-14.04/ part of the code, no?

[morphis]

We don't have to. The packaging/ubtunu-14.04 ships its own version of the systemd units and doesn't install those from data/systemd

[Conan-Kudo]

Since we're now using a Makefile, there's some small changes I'd like to see here.

[Conan-Kudo]

If we're going to use a Makefile, I would rather see it take a DESTDIR parameter and install the generated units to the correct place.

Add an install target, and have it write the real units to SYSTEMDSYSTEMUNITDIR (matching autotools name for this). Since you're doing the Ubuntu thing by default, this would be defined as /lib/systemd/system, whereas on RPM systems, I would pass in %{_unitdir} to ensure it went to the right place.

[Conan-Kudo]

You meant to have it use ${BINDIR} here.

[Conan-Kudo]

This should probably be made non-mandatory, like we do for the Fedora units.

That is:

EnvironmentFile=-@SNAPD_ENVIRONMENT_FILE@

[Conan-Kudo]

This is new to me. This should be templated too, even if it doesn't currently apply to non-snappy systems. You never know who is going to make a snap-based system. :)

[Conan-Kudo]

Add data/systemd/snapd.system-shutdown.service to the .gitignore and I think we're all set.

[Conan-Kudo]

Looks good to me.

[mvo5]

The SYSTEMSYSTEMUNITDIR appears to be missing a D - which highlights that maybe it should use _ ? To make it easier to read and to be more consistent we could just use the same name as in the rules file: SYSTEMD_UNITS_DESTDIR ? (unless there is a good reason for the different name of course).

[morphis]

I am fine with that. @Conan-Kudo requested to use SYSTEMDSYSTEMUNITDIR to match the name autotools is using.

[morphis]

@mvo5 @zyga Can you guys have another look at this?

[mvo5]

Small comments

[mvo5]

I was thinking about if this could be made more make-ish. What do you think about something like the bits below:

SYSTEMD_UNITS := $(patsubst %.service.in,%.service,$(wildcard *.service.in))

%.service: %.service.in
	cat $< | \
		sed s:@libexecdir@:${LIBEXECDIR}:g | \
		sed s:@SNAPD_ENVIRONMENT_FILE@:${SNAPD_ENVIRONMENT_FILE}:g | \
		sed s:@bindir@:${BINDIR}:g | \
		sed s:@SNAP_MOUNTDIR@:${SNAP_MOUNTDIR}:g  | \
	cat > $@


install: $(SYSTEMD_UNITS)

?

[morphis]

Fine for me. Let me add that.

[mvo5]

A bit of extra spaces here, not sure if that is on purpose or an accident.

[mvo5]

Very nice, thanks for doing this work!

[Conan-Kudo]

Looks good to me, only one change might be worth doing.

[Conan-Kudo]

You could probably simplify this further with wildcards, so that we don't need to know the exact names of all the services, since you've done the same for generating them.

[morphis]

Done

[zyga]

Some comments, feel free to land if addressed

[zyga]

How about a one-liner:

install: $(SYSTEMD_UNITS)
    $(foreach u,$^,install -D -m 0644 ${u} ${DESTDIR}/${SYSTEMDSYSTEMUNITDIR}/${u};)

[mvo5]

or maybe just make install install it all in one go?

install: $(SYSTEMD_UNTIS)
  install -m 0644 -D -t ${DESTDIR}/${SYSTEMDSYSTEMUNITDIR} ${SYSTEMD_UNTIS}

[zyga]

Ditto:

clean:
    rm -f ${SYSTEMD_UNITS_GENERATED}

[zyga]

snap-confine's build system calls this SNAP_MOUNT_DIR

[mvo5]

I slightly prefer install -D -m 0644 -t ${DESTDIR}/${SYSTEMDSYSTEMUNITDIR} ${SYSTEMD_UNITS} (not tested but should work) - feels less arcane to me. But I will not sweat over it :)

[Conan-Kudo]

Just a small tweak, if you want. :)

[Conan-Kudo]

You could change this to be data/systemd/*.service now, so that you don't have to keep updating this..

[Conan-Kudo]

Looks excellent to me.

[niemeyer]

Super nice collaboration here, thanks all!

I stepped by just for the wine 🥂.