diff mbox series

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

Message ID 20240322221327.12204-3-mg@max.gautier.name (mailing list archive)
State New
Headers show
Series maintenance: use packaged systemd units | expand

Commit Message

Max Gautier March 22, 2024, 10:11 p.m. UTC
Currently, `git maintenance start`, if it uses the systemd scheduler,
will create systemd user units in $XDG_CONFIG_HOME, and overwrite the
files on each invocation.
Conversely, `git maintenance stop` will remove the unit files.

This is problematic mostly because it steps on user toes (and their
configuration, also). You can't mask the git maintenance systemd units,
because they are in $XDG_CONFIG_HOME and you can only mask "vendor"
units (see man systemctl), you can't override them either.

Additionally, it necessitates creating, modifying and removing files
from builtin/gc.c, which adds code and some avoidable complexity.

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 ?

 Makefile                                 |   5 +
 builtin/gc.c                             | 117 +----------------------
 config.mak.uname                         |  10 ++
 systemd/user/git-maintenance@.service.in |  17 ++++
 systemd/user/git-maintenance@.timer      |  12 +++
 5 files changed, 49 insertions(+), 112 deletions(-)
 create mode 100644 systemd/user/git-maintenance@.service.in
 create mode 100644 systemd/user/git-maintenance@.timer

Comments

Eric Sunshine March 23, 2024, 8:38 a.m. UTC | #1
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.

> 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/
Max Gautier March 23, 2024, 9:52 a.m. UTC | #2
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.
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 4e255c81f2..4fb015478e 100644
--- a/Makefile
+++ b/Makefile
@@ -3469,6 +3469,11 @@  install: all
 	$(INSTALL) -m 644 $(SCRIPT_LIB) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
 	$(INSTALL) $(INSTALL_STRIP) $(install_bindir_xprograms) '$(DESTDIR_SQ)$(bindir_SQ)'
 	$(INSTALL) $(BINDIR_PROGRAMS_NO_X) '$(DESTDIR_SQ)$(bindir_SQ)'
+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
 
 ifdef MSVC
 	# We DO NOT install the individual foo.o.pdb files because they
diff --git a/builtin/gc.c b/builtin/gc.c
index dac59414f0..199c8e6240 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -2303,110 +2303,6 @@  static int is_systemd_timer_available(void)
 	return real_is_systemd_timer_available();
 }
 
-static char *xdg_config_home_systemd(const char *filename)
-{
-	return xdg_config_home_for("systemd/user", filename);
-}
-
-static int systemd_timer_delete_unit_templates(void)
-{
-	int ret = 0;
-	char *filename = xdg_config_home_systemd("git-maintenance@.timer");
-	if (unlink(filename) && !is_missing_file_error(errno))
-		ret = error_errno(_("failed to delete '%s'"), filename);
-	FREE_AND_NULL(filename);
-
-	filename = xdg_config_home_systemd("git-maintenance@.service");
-	if (unlink(filename) && !is_missing_file_error(errno))
-		ret = error_errno(_("failed to delete '%s'"), filename);
-
-	free(filename);
-	return ret;
-}
-
-static int systemd_timer_write_unit_templates(const char *exec_path)
-{
-	char *filename;
-	FILE *file;
-	const char *unit;
-
-	filename = xdg_config_home_systemd("git-maintenance@.timer");
-	if (safe_create_leading_directories(filename)) {
-		error(_("failed to create directories for '%s'"), filename);
-		goto error;
-	}
-	file = fopen_or_warn(filename, "w");
-	if (!file)
-		goto error;
-
-	unit = "# This file was created and is maintained by Git.\n"
-	       "# Any edits made in this file might be replaced in the future\n"
-	       "# by a Git command.\n"
-	       "\n"
-	       "[Unit]\n"
-	       "Description=Optimize Git repositories data\n"
-	       "\n"
-	       "[Timer]\n"
-	       "OnCalendar=%i\n"
-	       "Persistent=true\n"
-	       "RandomizedDelaySecond=3540\n"
-	       "FixedRandomDelay=true\n"
-	       "\n"
-	       "[Install]\n"
-	       "WantedBy=timers.target\n";
-	if (fputs(unit, file) == EOF) {
-		error(_("failed to write to '%s'"), filename);
-		fclose(file);
-		goto error;
-	}
-	if (fclose(file) == EOF) {
-		error_errno(_("failed to flush '%s'"), filename);
-		goto error;
-	}
-	free(filename);
-
-	filename = xdg_config_home_systemd("git-maintenance@.service");
-	file = fopen_or_warn(filename, "w");
-	if (!file)
-		goto error;
-
-	unit = "# This file was created and is maintained by Git.\n"
-	       "# Any edits made in this file might be replaced in the future\n"
-	       "# by a Git command.\n"
-	       "\n"
-	       "[Unit]\n"
-	       "Description=Optimize Git repositories data\n"
-	       "\n"
-	       "[Service]\n"
-	       "Type=oneshot\n"
-	       "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n"
-	       "LockPersonality=yes\n"
-	       "MemoryDenyWriteExecute=yes\n"
-	       "NoNewPrivileges=yes\n"
-	       "RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK\n"
-	       "RestrictNamespaces=yes\n"
-	       "RestrictRealtime=yes\n"
-	       "RestrictSUIDSGID=yes\n"
-	       "SystemCallArchitectures=native\n"
-	       "SystemCallFilter=@system-service\n";
-	if (fprintf(file, unit, exec_path, exec_path) < 0) {
-		error(_("failed to write to '%s'"), filename);
-		fclose(file);
-		goto error;
-	}
-	if (fclose(file) == EOF) {
-		error_errno(_("failed to flush '%s'"), filename);
-		goto error;
-	}
-	free(filename);
-	return 0;
-
-error:
-	free(filename);
-	systemd_timer_delete_unit_templates();
-	return -1;
-}
-
 static int systemd_timer_enable_unit(int enable,
 				     enum schedule_priority schedule)
 {
@@ -2447,24 +2343,21 @@  static int systemd_timer_enable_unit(int enable,
 	return 0;
 }
 
-static int systemd_timer_delete_units(void)
+static int systemd_timer_disable_units(void)
 {
 	return systemd_timer_enable_unit(0, SCHEDULE_HOURLY) ||
 	       systemd_timer_enable_unit(0, SCHEDULE_DAILY) ||
-	       systemd_timer_enable_unit(0, SCHEDULE_WEEKLY) ||
-	       systemd_timer_delete_unit_templates();
+	       systemd_timer_enable_unit(0, SCHEDULE_WEEKLY);
 }
 
 static int systemd_timer_setup_units(void)
 {
-	const char *exec_path = git_exec_path();
 
-	int ret = systemd_timer_write_unit_templates(exec_path) ||
-		  systemd_timer_enable_unit(1, SCHEDULE_HOURLY) ||
+	int ret = systemd_timer_enable_unit(1, SCHEDULE_HOURLY) ||
 		  systemd_timer_enable_unit(1, SCHEDULE_DAILY) ||
 		  systemd_timer_enable_unit(1, SCHEDULE_WEEKLY);
 	if (ret)
-		systemd_timer_delete_units();
+		systemd_timer_disable_units();
 	return ret;
 }
 
@@ -2473,7 +2366,7 @@  static int systemd_timer_update_schedule(int run_maintenance, int fd UNUSED)
 	if (run_maintenance)
 		return systemd_timer_setup_units();
 	else
-		return systemd_timer_delete_units();
+		return systemd_timer_disable_units();
 }
 
 enum scheduler {
diff --git a/config.mak.uname b/config.mak.uname
index d0dcca2ec5..35ca236874 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -68,6 +68,16 @@  ifeq ($(uname_S),Linux)
 	ifneq ($(findstring .el7.,$(uname_R)),)
 		BASIC_CFLAGS += -std=c99
 	endif
+	ifeq ($(shell command -v systemctl >/dev/null ?&& echo y),y)
+		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)
+		SYSTEMD_USER_UNIT_DIR = $(strip $(if $(and \
+			$(findstring $(prefix),$(HOME)),\
+			$(findstring $(HOME),$(prefix))),\
+			$(XDG_DATA_HOME),$(prefix)/$(lib)))/systemd/user
+	endif
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_ALLOCA_H = YesPlease
diff --git a/systemd/user/git-maintenance@.service.in b/systemd/user/git-maintenance@.service.in
new file mode 100644
index 0000000000..649ba87e7e
--- /dev/null
+++ b/systemd/user/git-maintenance@.service.in
@@ -0,0 +1,17 @@ 
+[Unit]
+Description=Optimize Git repositories data
+Documentation=man:git-maintenance(1)
+
+[Service]
+Type=oneshot
+ExecStart=@BINDIR@/git for-each-repo --config=maintenance.repo \
+          maintenance run --schedule=%i
+LockPersonality=yes
+MemoryDenyWriteExecute=yes
+NoNewPrivileges=yes
+RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6 AF_VSOCK
+RestrictNamespaces=yes
+RestrictRealtime=yes
+RestrictSUIDSGID=yes
+SystemCallArchitectures=native
+SystemCallFilter=@system-service
diff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer
new file mode 100644
index 0000000000..2834bac365
--- /dev/null
+++ b/systemd/user/git-maintenance@.timer
@@ -0,0 +1,12 @@ 
+[Unit]
+Description=Optimize Git repositories data
+Documentation=man:git-maintenance(1)
+
+[Timer]
+OnCalendar=%i
+Persistent=true
+RandomizedDelaySec=3540
+FixedRandomDelay=true
+
+[Install]
+WantedBy=timers.target