Re: [RFC PATCH 1/5] maintenance: package systemd units

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 21, 2024, 12:37 UTC
Message-ID: <ZfwqCv889UdI0mU6@tanuki>
In-Reply-To: <20240318153257.27451-2-mg@max.gautier.name>

On Mon, Mar 18, 2024 at 04:31:15PM +0100, Max Gautier wrote:

It would be great to document _why_ we want to package the systemd units alongside with Git.

Show 35 quoted lines

> Signed-off-by: Max Gautier <mg@max.gautier.name>
> ---
>  Makefile                              |  4 ++++
>  systemd/user/git-maintenance@.service | 16 ++++++++++++++++
>  systemd/user/git-maintenance@.timer   |  9 +++++++++
>  3 files changed, 29 insertions(+)
>  create mode 100644 systemd/user/git-maintenance@.service
>  create mode 100644 systemd/user/git-maintenance@.timer
> 
> diff --git a/Makefile b/Makefile
> index 4e255c81f2..276b4373c6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -619,6 +619,7 @@ htmldir = $(prefix)/share/doc/git-doc
>  ETC_GITCONFIG = $(sysconfdir)/gitconfig
>  ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
>  lib = lib
> +libdir = $(prefix)/lib
>  # DESTDIR =
>  pathsep = :
>  
> @@ -1328,6 +1329,8 @@ BUILTIN_OBJS += builtin/verify-tag.o
>  BUILTIN_OBJS += builtin/worktree.o
>  BUILTIN_OBJS += builtin/write-tree.o
>  
> +SYSTEMD_USER_UNITS := $(wildcard systemd/user/*)
> +
>  # THIRD_PARTY_SOURCES is a list of patterns compatible with the
>  # $(filter) and $(filter-out) family of functions. They specify source
>  # files which are taken from some third-party source where we want to be
> @@ -3469,6 +3472,7 @@ 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)'
> +	$(INSTALL) -Dm 644 -t '$(DESTDIR_SQ)$(libdir)/systemd/user' $(SYSTEMD_USER_UNITS)

I wonder whether we want to unconditionally install those units. Many of the platforms that we support don't even have systemd available, so certainly it wouldn't make any sense to install it on those platforms.

Assuming that this is something we want in the first place I thus think that we should at least make this conditional and add some platform specific quirk to "config.mak.uname".

Show 24 quoted lines

>  ifdef MSVC
>  	# We DO NOT install the individual foo.o.pdb files because they
> diff --git a/systemd/user/git-maintenance@.service b/systemd/user/git-maintenance@.service
> new file mode 100644
> index 0000000000..87ac0c86e6
> --- /dev/null
> +++ b/systemd/user/git-maintenance@.service
> @@ -0,0 +1,16 @@
> +[Unit]
> +Description=Optimize Git repositories data
> +
> +[Service]
> +Type=oneshot
> +ExecStart=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

Curious, but how did you arrive at these particular restrictions for the unit? Might be something to explain in the commit message, as well.

Patrick

Show 19 quoted lines

> diff --git a/systemd/user/git-maintenance@.timer b/systemd/user/git-maintenance@.timer
> new file mode 100644
> index 0000000000..40fbc77a62
> --- /dev/null
> +++ b/systemd/user/git-maintenance@.timer
> @@ -0,0 +1,9 @@
> +[Unit]
> +Description=Optimize Git repositories data
> +
> +[Timer]
> +OnCalendar=%i
> +Persistent=true
> +
> +[Install]
> +WantedBy=timers.target
> -- 
> 2.44.0
> 
>