Re: [PATCH v2 2/6] maintenance: use packaged systemd units

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 23, 2024, 08:38 UTC
Message-ID: <CAPig+cS1YL1iOZFh5=b1+_P=KBST=DYRqp7TaD7uMVLJQjs1bA@mail.gmail.com>
In-Reply-To: <20240322221327.12204-3-mg@max.gautier.name>

On Sat, Mar 23, 2024 at 4:21 AM Max Gautier <mg@max.gautier.name> wrote:

Show 24 quoted lines

> [...]
> Package the systemd user units (timer and service) with git in
> $(prefix)/lib/systemd/user (or $XDG_DATA_HOME for $HOME installation),
> and remove code for writing and deleting the units from builtin/gc.c.
> Determine the correct git path at install time by for the service unit.
>
> Detect systemd timers support (at install time) by relying on systemctl
> presence, since we use it as the control interface for the systemd
> scheduler.
>
> Signed-off-by: Max Gautier <mg@max.gautier.name>
> ---
> Notes:
>     I'm not completely sure if it's ok to do install time templating like
>     this, but I couldn't find a similar enough example in the Makefile. Any
>     suggestion for a better way ?
>
> diff --git a/Makefile b/Makefile
> @@ -3469,6 +3469,11 @@ install: all
> +ifdef SYSTEMD_USER_UNIT_DIR
> +       $(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)' systemd/user/git-maintenance@.timer
> +       sed 's+@BINDIR@+$(bindir_SQ)+' systemd/user/git-maintenance@.service.in | \
> +               $(INSTALL) -Dm 644 /dev/stdin '$(DESTDIR_SQ)$(SYSTEMD_USER_UNIT_DIR)/git-maintenance@.service'
> +endif

This is the first use of /dev/stdin in the project and I might worry a bit about portability. Granted, a system in which systemd is installed is likely to have /dev/stdin available, but it's often a good idea to be cautious when introducing something new into the project.

I would think it would be possible to instead generate the `git-maintenance@.service` file locally from the template `git-maintenance@.service.in` as part of the normal build process, and then install the built `git-maintenance@.service` at "install" time. That seems more in line with how other resources are handled, avoids the novel use of /dev/stdin, and answers the question you ask above.

> diff --git a/config.mak.uname b/config.mak.uname
> @@ -68,6 +68,16 @@ ifeq ($(uname_S),Linux)
> +       ifeq ($(shell command -v systemctl >/dev/null ?&& echo y),y)

What is "?&&"?

> +               XDG_DATA_HOME ?= $(HOME)/.local/share
> +               # systemd user units of programm installed in the home directory
> +               # (meaning prefix == $HOME) shall go in XDG_DATA_HOME
> +               # (from man 5 systemd.unit)

s/programm/program/