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

Previous Next

Full log

Message #32 received at 75959 <at> debbugs.gnu.org (full text, mbox):

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

Leo Famulari <leo <at> famulari.name> writes:

>> 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.

Makes sense.  I'll update serialization -> generation

>> * 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]".

Will do.

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

Interesting.  I'll do as you suggest, but that's not what I've seen in
other commit messages. e.g. commit
e73cf57a204f2bf430c90930394afa08e9ec3399

>> @@ -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.

I'll try to clean up these points myself first (I feel bad giving you
more work), but feel free to make changes as you see fit.  I'm aware
that my writing needs work...

>> +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?

I explained my reasoning in this thread and in the IRC, and didn't
recieve much pushback.  However, I hadn't even realized that the
Syncthing GUI doesn't use camelcase...  I've been generating config
files via Emacs Org Mode src blocks with heavy use of noweb-refs, so my
experience is probably skewed.  If this were my library, I would
definitely use camelcase because I think that is clearest, but it's not
that important to me, and I don't think the draw-backs are that large.
I'll just put more effort into adding links to syncthing documentation
for things that don't map so obviously (e.g. ldap-transport is not found
in the documentation as ldapTransport, it is the transport field of the
ldap element).

>> +@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?

No, and I'm not sure you should as automatically changing a port means
another machine doesn't know which port to look at (or so I assume,
maybe nameservers fix this issue, but I'd rather not bake in a
dependancy on third-party services).  Maybe this would make sense if
using guix-deploy because then you could tell each machine how the ports
match up.

>> +@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.

Basically, if you have D devices and F folders, then there are D*F
relationships between those folders.  Specifically, we need to know, for
a given folder, if a device should only recieve encrypted data, and from
where we know that device (so we can automatically remove this device if
the introducing device is removed).  This could be accomplished by
having devices be listed in a folder as a tuple (device password
introducer), but I think that would be abusing lists, so a record seems
appropriate here.  I'll try and make this text more clear as to this
record's function.

>> +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".

Will do

>> +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

Yes that's right

>> @@ -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?

That's a good point.  It's probably bad to have ANY default here.  I
just tested, and syncthing does not complain when apikey is omitted, so
I've made that change.  I'll add notes to this, the GUI password, and
encryption passwords that these are stored in /gnu/store and are
globally readable (GUI password is encrypted, but encryption passwords
are plain-text).

>> +;; 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.

The whitespace rules have been consistent for as long as I've used
syncthing, so my hopes are high.

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

Thank you for reviewing!  I'll submit another patch shortly implementing
what we have discussed.

Previous Next

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