GNU bug report logs - #75959
[PATCH] (home-)syncthing-service: added support for config serialization

Previous Next

Full log

View this message in rfc822 format

From: Leo Famulari <leo <at> famulari.name>
To: Zacchaeus <eikcaz <at> zacchae.us>
Cc: 75959 <at> debbugs.gnu.org
Subject: [bug#75959] [PATCH v5] services: syncthing: Added support for config file serialization.
Date: Fri, 7 Feb 2025 17:44:55 -0500

> Subject: [PATCH v5] services: syncthing: Added support for config file
>  serialization.

Great! I tested the system-level service according to the config in my
last email and was able to successfully integrate into my existing
Syncthing cluster.

Since we have already worked out the bugs on IRC, my feedback here will
be largely cosmetic.

I think of this patch as being more about generating Syncthing config
files, rather than serializing them.

> * gnu/services/syncthing.scm: (syncthing-config-file) (syncthing-folder)
> (syncthing-device) (syncthing-folder-device): New records;
> (syncthing-service-type): added special-files-service-type extension for the
> config file; (syncthing-files-service): service to create config file
> * gnu/home/services/syncthing.scm: (home-syncthing-service-type): extended
> home-files-services-type and re-exported more things from
> gnu/services/syncthing.scm
> * doc/guix.texi: (syncthing-service-type): document additions

In the commit title and message, please capitalize and punctuate
sentences normally, and avoid unecessary abbreviations. Try to write
imperative sentences. For example, "Add support for configuration file
serialization [or generation]".

When listing the new records / variables, they can be contained in the
same parenthetical list, like (syncthing-config-file, syncthing-folder,
[...]): New records.

> @@ -22669,9 +22670,298 @@ This assumes that the specified group exists.
>  Common configuration and data directory.  The default configuration
>  directory is @file{$HOME} of the specified Syncthing @code{user}.
>  
> +@item @code{config-file} (default: @var{#f})
> +Either a file-like object that resolves to a syncthing configuration xml
> +file, or a syncthing-config-file record (see below).  If set to #f, Guix
> +will not try to generate a config file, and the syncthing will generate
> +a default one which will not be touched on reconfigure.  Specifying this
> +in a system service moves Syncthing's common configuration and data
> +directory to @file{/var/lib/syncthnig-<user>}.

Again, use standard capitalization, etc. Also, use texinfo markup and
cross-references when useful:

https://www.gnu.org/software/texinfo/manual/texinfo/html_node/Cross-References.html

For example, "Either a file-like object that resolves to a Syncthing
configuration XML file, or a @code{syncthing-config-file} record (see
below).  When set to @code{#f} [...]"

If you like, I can copy-edit the documentation portion of this patch for
spelling, style, and grammar, as well as mark it up with texinfo. Let me
know. If you'd prefer I don't, I'll send another review with feedback on
those subjects.

> +In the below, only details specific to Guix, or related to how your
> +device will ``ping'' others, are presented.  Otherwise, you should
> +consult @uref{https://docs.syncthing.net/users/config.html, Syncthing
> +config documentation}.  Camelcase is preserved below only as to be
> +consistent with its appearance in Syncthing code/documentation.  If you
> +would like to migrate to Guix-powered Syncthing configuration, the
> +generated config adds newlines/whitespace to the produced config such
> +that your old config can be diff'ed with the new one.  You can still
> +modify Syncthing from the GUI or through ``introducer'' and
> +``autoAcceptFolders'' mechanisms, but such changes will be reset on
> +reconfigure.

I have some misgivings about the use of camel case in user-facing
interfaces. I do understand the utility of preserving the upstream
names, but I wonder how much utility it really provides. In my
experience, it's not typical to edit Syncthing's config.xml "by hand" (I
did it some when I first started with Syncthing), so I'd expect users'
to have little familiarity with these names, nor for them to desire a
direct mapping between them. Additionally, there is utility in all Guix
interfaces sharing a consistent style, and the camel case is not
surfaced in Syncthing's own GUI.

What do you think?

> +@item @code{localAnnouncePort} (default: @var{"21027"})
> +@item @code{localAnnounceMCAddr} (default: @var{"[ff12::8384]:21027"})

Not important for this patch, but do you have any ideas to facilitate
multi-user systems, with respect to automatically using alternate ports?

> +@deftp {Data Type} syncthing-folder-device
> +There is some configuration which is specific to the relationship
> +between a specific folder and a specific device.  If you are fine
> +leaving these as their default, then you can simply specify a
> +syncthing-device instead of a @code{syncthing-folder-device} in
> +@code{syncthing-folder}s.

I think this paragraph is not clear enough, although it starts to make
sense after reading about the parameters. Still I think there is room
for improvement.

> +if encryptionPassword is non-empty, then it will be used as a password
> +to encrypt file chunks as they are synced to that device.  For more info
> +on syncing to devices you don't totally trust, see
> +@uref{https://docs.syncthing.net/users/untrusted.html, Syncthing Documentation Untrusted}.

I would write the hyperlink as "Syncthing Documentation on Untrusted
Devices".

> +Here is a more complex example configuration for illustrative purposes:
> +@lisp
> +(service syncthing-service-type
> +         (let ((laptop (syncthing-device (id "VHOD2D6-...-7XRMDEN")))
> +               (desktop (syncthing-device (id "64SAZ37-...-FZJ5GUA")
> +                                          (addresses '("mydomain.example"))))

Let's use the reserved domain name 'example.com'. Also, Syncthing
requires these addresses to use a "protocol specific prefix", so how
about "tcp://example.com"?

https://docs.syncthing.net/users/config.html#config-option-device.address

> @@ -24,9 +25,23 @@ (define-module (gnu home services syncthing)

Okay. Sounds like you've tested the home service satisfactorily.

> @@ -25,9 +26,20 @@ (define-module (gnu services syncthing)
> +;; Some parameters, when empty, are fully omitted from the config file.  It is
> +;; unknown if this causes a functional difference, but stick to the normal
> +;; program's behavior to be safe.

Agreed

> +  (gui-apikey syncthing-config-gui-apikey (default "Vuky3VHVseQEoSk9YgxhSkNTnjQmqYK9"))

I've never used the GUI API, but shouldn't this key be generated
per-device, rather than hard-coded here?

> +;; It is useful to be able to view the xml output by Guix, and to be able to
> +;; diff it with a user's previous config, especially when migrating one's
> +;; config to Guix.  This function adds whitespace that matches the whitespace
> +;; of config files managed by Syncthing for easy diffing.
> +(define (indent-sxml sxml indent-increment current-indent)

Nice. Hopefully this doesn't go stale anytime soon.

Overall, it's looking good. Thanks a lot for this contribution! Let me
know what you think about the feedback I've given here.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.