Package: guix-patches;
Reported by: Wilko Meyer <w <at> wmeyer.eu>
Date: Wed, 23 Oct 2024 15:11:02 UTC
Severity: normal
Tags: patch
Message #8 received at 73967 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Wilko Meyer <w <at> wmeyer.eu> Cc: Ludovic Courtès <ludo <at> gnu.org>, 73967 <at> debbugs.gnu.org Subject: Re: [bug#73967] [PATCH] gnu: services: Add xandikos-service-type. Date: Fri, 02 May 2025 21:01:11 +0900
Hi!
Wilko Meyer <w <at> wmeyer.eu> writes:
> * gnu/services/dav.scm: New file.
> * gnu/local.mk (GNU_SYSTEM_MODULES): Add file.
> * doc/guix.texi (DAV Services): Document the service.
>
[...]
> I noticed that we already had a xandikos[0] package in Guix, but not a
> corresponding service, so I decided to create one.
Neat, although I've never heard of Xandikos until now :-).
> I took the liberty to add gnu/services/dav.scm as well as a DAV
> Services section in our documentation. We do have another
> CardDAV/CalDAV server available in guix, radicale, which currently
> resides in (gnu services mail) and should probably be moved into (gnu
> services dav) as well in the near future?
Since we already have a caldav server in (gnu services mail), and that
these things are often used together, it seems it'd have been simpler to
keep this one in (gnu services mail). In general I prefer to have more
grouping of services, that's less imports in an operating system config.
I guess it depends on the number of caldav server implementations we're
planning to add. I don't feel too strongly about it! If you prefer the
new, more accurate home, please do keep it (and move the other caldav
thing there in a later commit).
[...]
> Permission is granted to copy, distribute and/or modify this document
> under the terms of the GNU Free Documentation License, Version 1.3 or
> @@ -413,6 +414,7 @@ Top
> * DNS Services:: DNS daemons.
> * VNC Services:: VNC daemons.
> * VPN Services:: VPN daemons.
> +* DAV Services:: DAV daemons.
> * Network File System:: NFS related services.
> * Samba Services:: Samba services.
> * Continuous Integration:: Cuirass and Laminar services.
> @@ -19045,6 +19047,7 @@ Services
> * DNS Services:: DNS daemons.
> * VNC Services:: VNC daemons.
> * VPN Services:: VPN daemons.
> +* DAV Services:: DAV daemons.
> * Network File System:: NFS related services.
> * Samba Services:: Samba services.
> * Continuous Integration:: Cuirass and Laminar services.
> @@ -34511,6 +34514,59 @@ VPN Services
> @end table
> @end deftp
>
> +@node DAV Services
> +@subsection DAV Services
> +
> +@defvar xandikos-service-type
> +This service starts @code{xandikos}, a a lightweight CardDAV/CalDAV
s/a a/a/
> +server that backs onto a Git repository.
'backed by a Git repository', perhaps?
> +The service's value is a @code{xandikos-configuration} record.
> +@end defvar
> +
> +@deftp {Data Type} xandikos-configuration
> +This is the data type representing the configuration for the
> +xandikos-shepherd-service.
@code{xandikos-shepherd-service}
> +
> +It has the following parameters:
> +
> +@table @asis
Since each table element is using @code, it'd be better to simply use
@table @code
to declare the table and drop the usages of @code on each item.
> +@item @code{package} (default: @code{xandikos})
> +The @code{xandikos} package to use.
> +
> +@item @code{directory} (default: @code{"/var/xandikos/dav"})
> +Directory to serve from.
The directory [...]
> +@item @code{listen-address} (default: @code{127.0.0.1})
> +The address @code{xandikos} listens on.
s/@code/@command/
> +
> +@item @code{port} (default: @code{8080})
> +The port to run @code{xandikos} on.
Ditto.
> +@item @code{current-user-principal} (default: @code{"/user/"})
> +Path to current user principal.
In GNU the convention is to use 'file name' for files, not path (path is
used for search paths like $PATH).
> +@item @code{route-prefix} (default: @code{"/"})
> +Path to @code{xandikos} (useful when Xandikos is behind a reverse proxy).
'File name', and s/@code/@command/.
> +@item @code{defaults?} (default: @code{#t})
> +Create initial calendar and address book. Implies --autocreate.
Use two spaces between sentences. @option{--autocreate}
> +
> +@item @code{dump?} (default: @code{#f})
> +Print DAV XML request/responses.
> +
> +@item @code{avahi?} (default: @code{#f})
> +Announce services with avahi.
> +
> +@item @code{autocreate?} (default: @code{#f})
> +Automatically create necessary directories.
> +
> +@item @code{no-strict?} (default: @code{#f})
> +Enable workarounds for buggy CalDAV/CardDAV client implementations.
> +@end table
> +@end deftp
[...]
> diff --git a/gnu/services/dav.scm b/gnu/services/dav.scm
[...]
> +(define-module (gnu services dav)
> + #:use-module (gnu services)
> + #:use-module (gnu services shepherd)
> + #:use-module (gnu services configuration)
> + #:use-module (gnu packages dav)
> + #:use-module (guix deprecation)
> + #:use-module (guix gexp)
> + #:use-module (guix records)
> + #:use-module (ice-9 match)
Please keep imports lexicographically sorted.
> + #:export (xandikos-configuration
> + xandikos-configuration?
> + xandikos-service-type))
> +
> +;;;
> +;;; Xandikos
Add a '.' after Xandikos. That's conventional.
> +;;;
> +
> +(define-configuration/no-serialization xandikos-configuration
> + (package
> + (file-like xandikos)
> + "Xandikos package to use.")
> + (directory
> + (string "/var/xandikos/dav")
> + "Directory to serve from.")
> + (listen-address
> + (string "127.0.0.1")
> + "The address Xandikos listens on.")
> + (port
> + (integer 8080)
You con steal a the 'port?' definition used in the pounce-configuration
and elsewhere to validate a correct range.
> + "The port to run Xandikos on.")
> + (current-user-principal
> + (string "/user/")
> + "Path to current user principal.")
> + (route-prefix
> + (string "/")
> + "Path to Xandikos. (useful when Xandikos is behind a reverse proxy)")
The period should be moved after the closing parens.
> + (defaults?
> + (boolean #t)
> + "Create initial calendar and address book.")
> + (dump?
> + (boolean #f)
> + "Print DAV XML request/responses.")
> + (avahi?
> + (boolean #f)
> + "Announce services with avahi.")
> + (autocreate?
> + (boolean #f)
> + "Automatically create necessary directories.")
> + (no-strict?
> + (boolean #f)
> + "Enable workarounds for buggy CalDAV/CardDAV client implementations."))
Make sure to mirror any comments I made against the generated Texinfo
earlier in the actual source above.
> +(define (xandikos-shepherd-service config)
> + (match-record config <xandikos-configuration>
> + (package directory listen-address port current-user-principal route-prefix defaults? dump? avahi? autocreate? no-strict?)
Our maximum column width guideline is 80 :-).
> + (list
> + (shepherd-service
> + (provision '(xandikos))
> + (documentation "Caldav/CardDAV server")
> + (requirement '(networking))
This should be updated to include user-processes. If the services
starts happily before networking is set up, it's best to not explicitly
depend on 'networking', though I see the example systemd unit requires
networking explicitly, so maybe it's needed [0].
[0] https://github.com/jelmer/xandikos/blob/master/examples/xandikos.service#L3
> + (start #~(make-forkexec-constructor
It looks like xandikos supports the socket activation (on a unix socket)
scheme of systemd, which Shepherd supports too, via its
make-systemd-constructor constructor. That'd be nicer to use that, for
faster boot and synchronization. Could you adapt your service to use
it?
It could also be hardened via the least-authority wrapper, to
containerized the process, which usually suites network services well.
> + (list #$(file-append xandikos "/bin/xandikos")
> + "--listen-address" #$listen-address
> + "--port" #$(number->string port)
> + "-d" #$directory
> + "--route-prefix" #$route-prefix
> + "--current-user-principal" #$current-user-principal
> + #$@(if dump? '("--dump-dav-xml") '())
> + #$@(if avahi? '("--avahi") '())
> + #$@(if autocreate? '("--autocreate") '())
> + #$@(if defaults? '("--defaults") '())
> + #$@(if no-strict? '("--no-strict") '()))))
> + (stop #~(make-kill-destructor))
> + (respawn? #t)))))
Unrelated, but I'm surprised that respawn?'s defaults is #f in Shepherd.
Shouldn't the service manager tries its utmost by defgault to keep
things running? Does that mean that if any process crashes on my
machine, and the service hasn't been defined with respawn? #t, it will
remain in a stopped, failed state?
Thank you! If you could prepare a v2, I'll try to review it faster than
I did here, which shouldn't be too difficult, ah!
--
Thanks,
Maxim
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.