Subject: Re: [bug#75959] [PATCH v10] services: syncthing: Add support for config file generation.

From: Zacchaeus Scheffer <[email-masked]>
Date: Mon, 17 Feb 2025 15:32:01 -0500

Ludovic Courtès <[email-masked]> writes:

> Hi,
>
> Zacchaeus Scheffer <[email-masked]> skribis:
>
>>>>From a573fd78e6b8d10b32eb10a753423073c7bbaeef Mon Sep 17 00:00:00 2001
>> From: Zacchaeus <[email-masked]>
>> Date: Sun, 21 Jul 2024 00:54:25 -0700
>> Subject: [PATCH v10] services: syncthing: Add support for config file
>>  generation.
>>
>> * gnu/services/syncthing.scm: (syncthing-config-file,
>> syncthing-folder, syncthing-device, syncthing-folder-device): New
>> records;  (syncthing-service-type): Add special-files-service-type
>> extension for the config file; (syncthing-files-service): Add service
>> to create config file.
>> * gnu/home/services/syncthing.scm: (home-syncthing-service-type):
>> Extend home-files-services-type and re-exported more things from
>> gnu/services/syncthing.scm.
>> * doc/guix.texi: (syncthing-service-type): Document changes.
>>
>> Change-Id: I87eeba1ee1fdada8f29c2ee881fbc6bc4113dde9
>
> This is a nice improvement!  Much better than fiddling with the GUI.
> :-)

Thanks.  I'm glad you agree.  Great for managing swarms of devices

> Sorry for not chiming in earlier; I just noticed a couple of stylistic
> issues:
>
>> +This section documents a subset of the Syncthing configuration
>> +options—specifically those related to Guix or those affecting how your
>> +computer will connect to other computers over the network (such as
>> +Syncthing relays or discovery servers).  The configuration is fully
>> +documented in the upstream
>> +@uref{https://docs.syncthing.net/users/config.html, Syncthing config
>> +documentation}; camelCase there is converted to kebab-case here.  If you
>
> “Kebab-case”, really? :-)

I believe that is an accurate term:
https://en.wikipedia.org/wiki/Naming_convention_(programming)#Delimiter-separated_words

>> +@table @asis
>> +@item @code{folders} (default: @var{(list (syncthing-folder (id "default") (label "Default Folder") (path "~/Sync")))}
>
> Should be @code, not @var (throughout this table).
>
> Could you send a patch fixing this?

Sure thing.  I assume I can send it to the same issue even though it is
closed?

> BTW, ‘define-configuration’ would make it easier to generate this doc.

I originally used define-record because that is what the original
syncthing service implementation used.  If there are reasons to change
this I could update this as well, but it seems fine.

>> +@item @code{gui-enabled} (default: @var{"true"})
>
> The naming convention used in all of Guix is to add a final question
> mark for Boolean values; this would be ‘gui-enabled?’.
>
> As for the value itself, it would help a lot to use #t and #f instead of
> the strings "true" and "false" (both of which have truth value in
> Scheme).  It leads to a bit of extra work in the serializer, but I think
> it’s worth it.  Because then we can also have type-checking of fields.

This would be easiest to implement as a sanitizer as in:

(define (bool-string bool)
  (if bool "true" "false"))
(define-record-type* <syncthing-config-file>
...
  (gui-enabled <accessor> (default "true")
                          (sanitizer bool-string))
...

But then the guix docs would technically be wrong reporting a default
value of #t instead of the actual default value of "true".  I think this
would yield a more helpful type-checking error though, so I'll go this
direction unless you think otherwise.

>> +@item @code{gui-tls} (default: @var{"false"})
>> +@item @code{gui-debugging} (default: @var{"false"})
>> +@item @code{gui-send-basic-auth-prompt} (default: @var{"false"})
>> +@item @code{gui-address} (default: @var{"127.0.0.1:8384"})
>> +@item @code{gui-user} (default: @var{#f})
>> +@item @code{gui-password} (default: @var{#f})
>> +A bcrypt hash of the GUI password.  Remember that this will be globally
>> +exposed in @file{/gnu/store}.
>
> I believe you want @itemx for all but the first item.

Does it matter that the describing text only applies to gui-password,
not to the whole block?  For now, I'll assume not and change all the
consecutive items with no specific documentation throughout to itemx.

>> +@item @code{local-announce-port} (default: @var{"21027"})
>> +@item @code{local-announce-mcaddr} (default: @var{"[ff12::8384]:21027"})
>> +@item @code{max-send-kbps} (default: @var{"0"})
>> +@item @code{max-recv-kbps} (default: @var{"0"})
>> +@item @code{reconnection-interval-s} (default: @var{"60"})
>
> Similar to what I wrote above: numbers should be numbers, not strings.

I'll figure this out as well, using sanitizers (or leaving it as
numbers.  It seems sxml allows the body of a tag to be a number, but not
values of parameters of tags, so most the time it just works.)

> Last note: the convention throughout Guix it to avoid abbreviations.

My goal was to, as much as possible, avoid needing to maintain Syncthing
documentation in Guix.  My original patch even had camelcase for record
names so the user could easily search the Syncthing documentation
knowing what keyword to search.  Since you can't directly search anymore
anyway, maybe it does make sense to expand out some names.  I'll do that
for these:

auth - authorization
s - seconds
m - minutes
h - hours
fs - file-system
pct - percentage
mcaddr - mac-address
recv - recieve
ur - usage-reporting

> I’m not sure about the way forward; maybe we can make those typing
> changes (and perhaps some naming changes) and afford some
> incompatibility because the service is young?  WDYT?
>
> Ludo’.

I think it's unlikely that the service update will have major adoption
in the couple days between patches, so I think a bit of incompatibility
is fine.

eikcaz-