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

Previous Next

Full log

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

From: Zacchaeus Scheffer <eikcaz <at> zacchae.us>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Bruno Victal <mirai <at> makinata.eu>, 75959 <at> debbugs.gnu.org,
 Leo Famulari <leo <at> famulari.name>
Subject: Re: [bug#75959] [PATCH v10] services: syncthing: Add support for
 config file generation.
Date: Fri, 21 Feb 2025 15:26:40 -0500

Hi all,

Ludovic Courtès <ludo <at> gnu.org> writes:

> Hi,
>
> Zacchaeus Scheffer <eikcaz <at> zacchae.us> skribis:
>> Sure thing.  I assume I can send it to the same issue even though it is
>> closed?
>
> Yes, but maybe open a new issue, for clarity.

I have opened a new issue at #76379

>>> 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.
>
> I would advise against using sanitizer in this way because it would lead
> to inconsistencies: users would provide a boolean, but they’d get a
> string when calling the accessor.
>
> Instead the record should contain records of the “right” type;
> converting to the relevant string should be left to whatever serializes
> the record to the config file.
>
> (This is something ‘define-configuration’ really helps with; I think
> it’s worth looking into it!)

Alright, a suggestion from the Ludo made twice should not be so easily
dismissed.  I should mention my original assertion here is slightly
wrong; to work, I should set (default #t), but, as you mention, the
accessor will still retrieve a string instead of a bool.  I originally
tried to do my serialization through 'define-configuration', but most of
that work evaporated when I discovered sxml (which is the right way to
do it).  Still, there may be some room for improvment translating to
'define-configuration'.

I will investigate how to best translate 'define-record-type*' to
'define-configuration', but in the meantime I think we should prioritize
getting my new patch (#76379) through.  I renamed fields and changed
data types per your suggestions, so my new patch is backwards
incompatible with the first.

>>>> +@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?
>
> It does yes, so my suggestion is actually bogus, sorry!

Alright, I reverted (most of) my itemx changes in the v2 patch submitted
at #76379.

> Normally each field would be documented, not just ‘gui-password’.

So here's the problem: Syncthing has a LOT of documentation for a LOT of
fields.  Do we really want to maintain all that documentation in Guix?
I omitted documenting the majority of fields, pointing to Syncthing
documentation instead, out of caution for future Guix maintainers, not
laziness.  I opted for just documenting (1) any feature that is specific
to Guix configuration and (2) anything that controls to which other
devices your device connects.  For anything else, the user should
consult Syncthing documentation.  Is my premise flawed, and I should
document everything?  Maybe a one-liner for each is unlikely to be
invalidated by future Syncthing updates. WDYT?


eikcaz-

Previous Next

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