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

From: Max Gautier <mg@max.gautier.name>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: git@vger.kernel.org, "Lénaïc Huard" <lenaic@lhuard.fr>,
	"Derrick Stolee" <stolee@gmail.com>,
	"Patrick Steinhardt" <ps@pks.im>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Junio C Hamano" <gitster@pobox.com>
Subject: Re: [PATCH v2 2/6] maintenance: use packaged systemd units
Date: Sat, 23 Mar 2024 10:52:48 +0100	[thread overview]
Message-ID: <Zf6mcBhd31WYkwob@framework> (raw)
In-Reply-To: <CAPig+cS1YL1iOZFh5=b1+_P=KBST=DYRqp7TaD7uMVLJQjs1bA@mail.gmail.com>

On Sat, Mar 23, 2024 at 04:38:44AM -0400, Eric Sunshine wrote:
> On Sat, Mar 23, 2024 at 4:21 AM Max Gautier <mg@max.gautier.name> wrote:
> > [...]
> > 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.

Ok.  It's not completely obvious to me how the "Detect prefix change
logic" works, but using other rules as a model, I think I can do
something like that:
systemd/user/git-maintenance@.service: systemd/user/git-maintenance@.service.in GIT-PREFIX
    sed 's+@BINDIR@+$(bindir_SQ)+' $< > $@

and depending on GIT-PREFIX should regenerate the service when changing
the prefix, correct ?
I'll need to add that to .gitignore as well.

> 
> > 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 "?&&"?
> 

Hum, a typo, sorry that slipped through. It apparently works regardless,
because shell expansion does something with it I guess. Curious. I'll
clean that up as well.

> > +               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/
Ack.
That should even be 'programs' I think, that's a general rule.

-- 
Max Gautier