Add a way to disable serialization support to (guix services configuration)

Add a way to disable serialization support to (guix services configuration)

[Maxim Cournoyer]

Hello Guix!

I've rediscovered the little gem that is (guix services configurations),
and attempted to make it more generally useful by adding an option to
opt out of serialization (which is not well adapted for producing a list
of command line arguments from the configuration for example):

--8<---------------cut here---------------start------------->8---
1 file changed, 10 insertions(+), 1 deletion(-)
gnu/services/configuration.scm | 11 ++++++++++-

modified   gnu/services/configuration.scm
@@ -38,6 +38,9 @@
             configuration-field-getter
             configuration-field-default-value-thunk
             configuration-field-documentation
+
+            %with-serialization?
+
             serialize-configuration
             define-maybe
             define-configuration
@@ -51,6 +54,11 @@
 ;;;
 ;;; Code:
 
+;;; XXX: This doesn't actually work as a parameter with macros such as
+;;; define-configuration; it is to be used as a plain global variable.
+;;; Experiments with define-syntax-parameter did not work either.
+(define %with-serialization? (make-parameter #true))
+
 (define-condition-type &configuration-error &error
   configuration-error?)
 
@@ -123,7 +131,8 @@
                            #'(field-type ...)))
                      ((field-serializer ...)
                       (map (lambda (type)
-                             (id #'stem #'serialize- type))
+                             (and (%with-serialization?)
+                                  (id #'stem #'serialize- type)))
                            #'(field-type ...))))
            #`(begin
                (define-record-type* #,(id #'stem #'< #'stem #'>)
--8<---------------cut here---------------end--------------->8---

Unfortunately, it doesn't work, at least when using it from 'guix
system'.  I've also tried a version relying on syntax-parameter instead
of a parameter, with the same result.

Would someone know how it could be made to work?

Thanks,

Maxim

Re: Add a way to disable serialization support to (guix services configuration)

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> I've rediscovered the little gem that is (guix services configurations),
> and attempted to make it more generally useful by adding an option to
> opt out of serialization (which is not well adapted for producing a list
> of command line arguments from the configuration for example):

In the meantime I saw you discuss this on #guile so maybe you’ve solved
the issue now?

If not, attached is a possible solution and example.  It’s not perfect:
in the example, you’ll get a warning about ‘serialize-integer’ being
unbound, which is completely harmless but suboptimal.  Not sure how to
address that.

HTH!

Ludo’.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 1493 bytes --]

diff --git a/gnu/services/configuration.scm b/gnu/services/configuration.scm
index 90f12a8d39..20e1647335 100644
--- a/gnu/services/configuration.scm
+++ b/gnu/services/configuration.scm
@@ -109,6 +109,9 @@
              (define (serialize-maybe-stem field-name val)
                (if (stem? val) (serialize-stem field-name val) ""))))))))
 
+(define-syntax-parameter configuration-field-serialization?
+  (identifier-syntax #t))
+
 (define-syntax define-configuration
   (lambda (stx)
     (syntax-case stx ()
@@ -123,7 +126,8 @@
                            #'(field-type ...)))
                      ((field-serializer ...)
                       (map (lambda (type)
-                             (id #'stem #'serialize- type))
+                             #`(and configuration-field-serialization?
+                                    #,(id #'stem #'serialize- type)))
                            #'(field-type ...))))
            #`(begin
                (define-record-type* #,(id #'stem #'< #'stem #'>)
@@ -152,6 +156,16 @@
                                            #,(id #'stem #'stem #'-fields))
                    conf))))))))
 
+(define-syntax-rule (without-field-serialization definition)
+  (syntax-parameterize ((configuration-field-serialization?
+                         (identifier-syntax #f)))
+    definition
+    #t))
+
+(without-field-serialization
+  (define-configuration foo
+    (bar (integer 123) "doc")))
+
 (define (serialize-package field-name val)
   "")
 

Re: Add a way to disable serialization support to (guix services configuration)

[Maxim Cournoyer]

Hi Ludovic,

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

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> I've rediscovered the little gem that is (guix services configurations),
>> and attempted to make it more generally useful by adding an option to
>> opt out of serialization (which is not well adapted for producing a list
>> of command line arguments from the configuration for example):
>
> In the meantime I saw you discuss this on #guile so maybe you’ve solved
> the issue now?

No, thank you for your reply! :-)

> If not, attached is a possible solution and example.  It’s not perfect:
> in the example, you’ll get a warning about ‘serialize-integer’ being
> unbound, which is completely harmless but suboptimal.  Not sure how to
> address that.
>
> diff --git a/gnu/services/configuration.scm b/gnu/services/configuration.scm
> index 90f12a8d39..20e1647335 100644
> --- a/gnu/services/configuration.scm
> +++ b/gnu/services/configuration.scm
> @@ -109,6 +109,9 @@
>               (define (serialize-maybe-stem field-name val)
>                 (if (stem? val) (serialize-stem field-name val) ""))))))))
>
> +(define-syntax-parameter configuration-field-serialization?
> +  (identifier-syntax #t))
> +
>  (define-syntax define-configuration
>    (lambda (stx)
>      (syntax-case stx ()
> @@ -123,7 +126,8 @@
>                             #'(field-type ...)))
>                       ((field-serializer ...)
>                        (map (lambda (type)
> -                             (id #'stem #'serialize- type))
> +                             #`(and configuration-field-serialization?
> +                                    #,(id #'stem #'serialize- type)))

I tried:

(if (syntax->datum configuration-field-serialization?)
    #,(id #'stem #'serialize- type)
    #false)

hoping to get rid of the warning, but that appears to be strictly
equivalent, resulting in the same warning.  I also don't know how to fix
that.

>                             #'(field-type ...))))
>             #`(begin
>                 (define-record-type* #,(id #'stem #'< #'stem #'>)
> @@ -152,6 +156,16 @@
>                                             #,(id #'stem #'stem #'-fields))
>                     conf))))))))
>
> +(define-syntax-rule (without-field-serialization definition)
> +  (syntax-parameterize ((configuration-field-serialization?
> +                         (identifier-syntax #f)))
> +    definition
> +    #t))
> +
> +(without-field-serialization
> +  (define-configuration foo
> +    (bar (integer 123) "doc")))
> +
>  (define (serialize-package field-name val)
>    "")


That does work, thanks!  I'm a bit uncomfortable adding more warnings to
the Guix build output; OTOH it provides value (another user recently
pointed to the same annoyance here [0]).

Maxim

[0]  https://lists.gnu.org/archive/html/guix-devel/2021-04/msg00319.html

Re: Add a way to disable serialization support to (guix services configuration)

[Ludovic Courtès]

Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

>> +(define-syntax-rule (without-field-serialization definition)
>> +  (syntax-parameterize ((configuration-field-serialization?
>> +                         (identifier-syntax #f)))
>> +    definition
>> +    #t))
>> +
>> +(without-field-serialization
>> +  (define-configuration foo
>> +    (bar (integer 123) "doc")))

In hindsight, I find this syntax quite inelegant and suboptimal.

Wouldn’t it be nicer to write:

  (define-configuration foo
    (bar (integer 123) "doc" no-serializer)
    (baz (string "") "doc"))

where ‘bar’ wouldn’t have a serializer and ‘baz’ would?

It’s also probably easier to implement correctly.

Ludo’.

Re: Add a way to disable serialization support to (guix services configuration)

[Xinglu Chen]

Hi,

On Fri, Apr 23 2021, Ludovic Courtès wrote:

> Hi,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>>> +(define-syntax-rule (without-field-serialization definition)
>>> +  (syntax-parameterize ((configuration-field-serialization?
>>> +                         (identifier-syntax #f)))
>>> +    definition
>>> +    #t))
>>> +
>>> +(without-field-serialization
>>> +  (define-configuration foo
>>> +    (bar (integer 123) "doc")))
>
> In hindsight, I find this syntax quite inelegant and suboptimal.
>
> Wouldn’t it be nicer to write:
>
>   (define-configuration foo
>     (bar (integer 123) "doc" no-serializer)
>     (baz (string "") "doc"))
>
> where ‘bar’ wouldn’t have a serializer and ‘baz’ would?
>
> It’s also probably easier to implement correctly.

I think that would be a good idea, maybe it could also make having a
default value be optional, like this:

#+begin_src scheme
(define-configuration foo
  (bar (integer) "doc" no-serializer) ;no default
  (baz (string "default") "doc"))
#+end_src

Maxim and I had a discussion about this[1].

[1]: https://yhetil.org/guix-devel/87k0ov7w72.fsf@yoctocell.xyz

Re: Add a way to disable serialization support to (guix services configuration)

[Xinglu Chen]

[-- Attachment #1: Type: text/plain, Size: 960 bytes --]

Hi,

On Fri, Apr 23 2021, Xinglu Chen wrote:

>> Wouldn’t it be nicer to write:
>>
>>   (define-configuration foo
>>     (bar (integer 123) "doc" no-serializer)
>>     (baz (string "") "doc"))
>>
>> where ‘bar’ wouldn’t have a serializer and ‘baz’ would?
>>
>> It’s also probably easier to implement correctly.
>
> I think that would be a good idea, maybe it could also make having a
> default value be optional, like this:
>
> #+begin_src scheme
> (define-configuration foo
>   (bar (integer) "doc" no-serializer) ;no default
>   (baz (string "default") "doc"))
> #+end_src

Since nobody seemed to show any objections, I went ahead and did
something like this myself (see the attached patches).  My macros skills
aren’t that good so I hope I didn’t make any obvious mistakes. :)

I haven’t tested it thoroughly, but at least it didn’t break anything
when I ran ‘./pre-inst-env guix home build ...’. :)


[-- Attachment #2: no-default-value --]
[-- Type: text/x-patch, Size: 6509 bytes --]

From 231281ebf555295e83513873293a1ad3eab884a8 Mon Sep 17 00:00:00 2001
Message-Id: <231281ebf555295e83513873293a1ad3eab884a8.1619869705.git.public@yoctocell.xyz>
In-Reply-To: <cover.1619869705.git.public@yoctocell.xyz>
References: <cover.1619869705.git.public@yoctocell.xyz>
From: Xinglu Chen <public@yoctocell.xyz>
Date: Sat, 1 May 2021 13:24:43 +0200
Subject: [PATCH 1/2] services: configuration: Support fields without default
 values.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Not all fields in a configuration have a sensible default value,
e.g. ‘user.name’ in gitconfig, the user should have to set that themselves

* gnu/services/configuration.scm (configuration-missing-field): New procedure.
(define-configuration): Make default value optional.
---
 gnu/services/configuration.scm | 78 ++++++++++++++++++++++------------
 1 file changed, 51 insertions(+), 27 deletions(-)

diff --git a/gnu/services/configuration.scm b/gnu/services/configuration.scm
index 90f12a8d39..85e1ac78cb 100644
--- a/gnu/services/configuration.scm
+++ b/gnu/services/configuration.scm
@@ -2,6 +2,7 @@
 ;;; Copyright © 2015 Andy Wingo <wingo@igalia.com>
 ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
 ;;; Copyright © 2017, 2018 Clément Lassieur <clement@lassieur.org>
+;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -63,6 +64,9 @@
 (define (configuration-missing-field kind field)
   (configuration-error
    (format #f "~a configuration missing required field ~a" kind field)))
+(define (configuration-no-default-value kind field)
+  (configuration-error
+   (format #f "`~a' in `~a' does not have a default value" kind field)))
 
 (define-record-type* <configuration-field>
   configuration-field make-configuration-field configuration-field?
@@ -112,7 +116,7 @@
 (define-syntax define-configuration
   (lambda (stx)
     (syntax-case stx ()
-      ((_ stem (field (field-type def) doc) ...)
+      ((_ stem (field (field-type properties ...) doc) ...)
        (with-syntax (((field-getter ...)
                       (map (lambda (field)
                              (id #'stem #'stem #'- field))
@@ -121,36 +125,56 @@
                       (map (lambda (type)
                              (id #'stem type #'?))
                            #'(field-type ...)))
+                     ((field-default ...)
+                      (map (match-lambda
+                             ((field-type default _ ...) default)
+                             ;; We get warnings about `disabled' being an
+                             ;; unbound variable unless we quote it.
+                             (_ (syntax 'disabled)))
+                           #'((field-type properties ...) ...)))
                      ((field-serializer ...)
                       (map (lambda (type)
                              (id #'stem #'serialize- type))
                            #'(field-type ...))))
-           #`(begin
-               (define-record-type* #,(id #'stem #'< #'stem #'>)
-                 #,(id #'stem #'% #'stem)
-                 #,(id #'stem #'make- #'stem)
-                 #,(id #'stem #'stem #'?)
-                 (%location #,(id #'stem #'-location)
-                            (default (and=> (current-source-location)
-                                            source-properties->location))
-                            (innate))
-                 (field field-getter (default def))
-                 ...)
-               (define #,(id #'stem #'stem #'-fields)
-                 (list (configuration-field
-                        (name 'field)
-                        (type 'field-type)
-                        (getter field-getter)
-                        (predicate field-predicate)
-                        (serializer field-serializer)
-                        (default-value-thunk (lambda () def))
-                        (documentation doc))
-                       ...))
-               (define-syntax-rule (stem arg (... ...))
-                 (let ((conf (#,(id #'stem #'% #'stem) arg (... ...))))
-                   (validate-configuration conf
-                                           #,(id #'stem #'stem #'-fields))
-                   conf))))))))
+         #`(begin
+             (define-record-type* #,(id #'stem #'< #'stem #'>)
+               #,(id #'stem #'% #'stem)
+               #,(id #'stem #'make- #'stem)
+               #,(id #'stem #'stem #'?)
+               (%location #,(id #'stem #'-location)
+                          (default (and=> (current-source-location)
+                                          source-properties->location))
+                          (innate))
+               #,@(map (lambda (name getter def)
+                         (if (equal? (syntax->datum def) (quote 'disabled))
+                             #`(#,name #,getter)
+                             #`(#,name #,getter (default #,def))))
+                       #'(field ...)
+                       #'(field-getter ...)
+                       #'(field-default ...)))
+             (define #,(id #'stem #'stem #'-fields)
+               (list (configuration-field
+                      (name 'field)
+                      (type 'field-type)
+                      (getter field-getter)
+                      (predicate field-predicate)
+                      (serializer field-serializer)
+                      ;; TODO: What if there is no default value?
+                      (default-value-thunk
+                        (lambda ()
+                          (display '#,(id #'stem #'% #'stem))
+                          (if (equal? (syntax->datum field-default)
+                                      (quote 'disabled))
+                              (configuration-no-default-value
+                               '#,(id #'stem #'% #'stem) 'field)
+                              field-default)))
+                      (documentation doc))
+                     ...))
+             (define-syntax-rule (stem arg (... ...))
+               (let ((conf (#,(id #'stem #'% #'stem) arg (... ...))))
+                 (validate-configuration conf
+                                         #,(id #'stem #'stem #'-fields))
+                 conf))))))))
 
 (define (serialize-package field-name val)
   "")
-- 
2.31.1


[-- Attachment #3: custom-serializer --]
[-- Type: text/x-patch, Size: 3020 bytes --]

From 90d63a46a29a8080b7f95eabcec115c5c2c6481e Mon Sep 17 00:00:00 2001
Message-Id: <90d63a46a29a8080b7f95eabcec115c5c2c6481e.1619869705.git.public@yoctocell.xyz>
In-Reply-To: <cover.1619869705.git.public@yoctocell.xyz>
References: <cover.1619869705.git.public@yoctocell.xyz>
From: Xinglu Chen <public@yoctocell.xyz>
Date: Sat, 1 May 2021 13:31:27 +0200
Subject: [PATCH 2/2] services: configuration: Add ability to use custom
 serializer.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Some configuration values should not be serialized to a configuration file,
e.g. command line options for a daemon.

* gnu/services/configuration.scm (define-configuration): Add option to use a
custom serializer.
(empty-serializer): New procedure.
(serialize-package): Alias to ‘empty-serializer’.
---
 gnu/services/configuration.scm | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/gnu/services/configuration.scm b/gnu/services/configuration.scm
index 85e1ac78cb..7024b13136 100644
--- a/gnu/services/configuration.scm
+++ b/gnu/services/configuration.scm
@@ -44,6 +44,7 @@
             define-configuration
             validate-configuration
             generate-documentation
+            empty-serializer
             serialize-package))
 
 ;;; Commentary:
@@ -116,7 +117,7 @@
 (define-syntax define-configuration
   (lambda (stx)
     (syntax-case stx ()
-      ((_ stem (field (field-type properties ...) doc) ...)
+      ((_ stem (field (field-type properties ...) doc custom-serializer ...) ...)
        (with-syntax (((field-getter ...)
                       (map (lambda (field)
                              (id #'stem #'stem #'- field))
@@ -133,9 +134,12 @@
                              (_ (syntax 'disabled)))
                            #'((field-type properties ...) ...)))
                      ((field-serializer ...)
-                      (map (lambda (type)
-                             (id #'stem #'serialize- type))
-                           #'(field-type ...))))
+                      (map (lambda (type serializer)
+                             (match serializer
+                               (((serializer)) serializer)
+                               (_ (id #'stem #'serialize- type))))
+                           #'(field-type ...)
+                           #'((custom-serializer ...) ...))))
          #`(begin
              (define-record-type* #,(id #'stem #'< #'stem #'>)
                #,(id #'stem #'% #'stem)
@@ -176,8 +180,8 @@
                                          #,(id #'stem #'stem #'-fields))
                  conf))))))))
 
-(define (serialize-package field-name val)
-  "")
+(define (empty-serializer field-name val) "")
+(define serialize-package empty-serializer)
 
 ;; A little helper to make it easier to document all those fields.
 (define (generate-documentation documentation documentation-name)
-- 
2.31.1

Re: Add a way to disable serialization support to (guix services configuration)

[Maxim Cournoyer]

Hello Xinglu!

Thank you for working on it!  I spent the evening trying things but none
worked, so your kudos for finding how to make it work! :-).  Some
comments follow (and a patch implementing them):

Xinglu Chen <public@yoctocell.xyz> writes:

[...]

> @@ -63,6 +64,9 @@
>  (define (configuration-missing-field kind field)
>    (configuration-error
>     (format #f "~a configuration missing required field ~a" kind field)))
> +(define (configuration-no-default-value kind field)
> +  (configuration-error
> +   (format #f "`~a' in `~a' does not have a default value" kind field)))

The kind and field should be inverted.

>  (define-record-type* <configuration-field>
>    configuration-field make-configuration-field configuration-field?
> @@ -112,7 +116,7 @@
>  (define-syntax define-configuration
>    (lambda (stx)
>      (syntax-case stx ()
> -      ((_ stem (field (field-type def) doc) ...)
> +      ((_ stem (field (field-type properties ...) doc) ...)

I'd rather keep the 'def' binding for the default value; properties is
too vague and implies many of them, which is not a supported syntax.

>         (with-syntax (((field-getter ...)
>                        (map (lambda (field)
>                               (id #'stem #'stem #'- field))
> @@ -121,36 +125,56 @@
>                        (map (lambda (type)
>                               (id #'stem type #'?))
>                             #'(field-type ...)))
> +                     ((field-default ...)
> +                      (map (match-lambda
> +                             ((field-type default _ ...) default)
> +                             ;; We get warnings about `disabled' being an
> +                             ;; unbound variable unless we quote it.
> +                             (_ (syntax 'disabled)))

Here I think it'd be better to have the pattern more strict (e.g,
(field-type default-value) or (field-type); so as to not accept invalid
syntax.

I also think it'd be clearer to use another symbol than 'disabled, as
this already has a meaning for the validator and could confuse readers.

> +                           #'((field-type properties ...) ...)))
>                       ((field-serializer ...)
>                        (map (lambda (type)
>                               (id #'stem #'serialize- type))
>                             #'(field-type ...))))
> -           #`(begin
> -               (define-record-type* #,(id #'stem #'< #'stem #'>)
> -                 #,(id #'stem #'% #'stem)
> -                 #,(id #'stem #'make- #'stem)
> -                 #,(id #'stem #'stem #'?)
> -                 (%location #,(id #'stem #'-location)
> -                            (default (and=> (current-source-location)
> -                                            source-properties->location))
> -                            (innate))
> -                 (field field-getter (default def))
> -                 ...)
> -               (define #,(id #'stem #'stem #'-fields)
> -                 (list (configuration-field
> -                        (name 'field)
> -                        (type 'field-type)
> -                        (getter field-getter)
> -                        (predicate field-predicate)
> -                        (serializer field-serializer)
> -                        (default-value-thunk (lambda () def))
> -                        (documentation doc))
> -                       ...))
> -               (define-syntax-rule (stem arg (... ...))
> -                 (let ((conf (#,(id #'stem #'% #'stem) arg (... ...))))
> -                   (validate-configuration conf
> -                                           #,(id #'stem #'stem #'-fields))
> -                   conf))))))))
> +         #`(begin
> +             (define-record-type* #,(id #'stem #'< #'stem #'>)
> +               #,(id #'stem #'% #'stem)
> +               #,(id #'stem #'make- #'stem)
> +               #,(id #'stem #'stem #'?)
> +               (%location #,(id #'stem #'-location)
> +                          (default (and=> (current-source-location)
> +                                          source-properties->location))
> +                          (innate))
> +               #,@(map (lambda (name getter def)
> +                         (if (equal? (syntax->datum def) (quote 'disabled))

nitpick: eq? suffices to check for symbols.

> +                             #`(#,name #,getter)
> +                             #`(#,name #,getter (default #,def))))
> +                       #'(field ...)
> +                       #'(field-getter ...)
> +                       #'(field-default ...)))
> +             (define #,(id #'stem #'stem #'-fields)
> +               (list (configuration-field
> +                      (name 'field)
> +                      (type 'field-type)
> +                      (getter field-getter)
> +                      (predicate field-predicate)
> +                      (serializer field-serializer)
> +                      ;; TODO: What if there is no default value?

Seems this TODO was taken care of already :-).

> +                      (default-value-thunk
> +                        (lambda ()
> +                          (display '#,(id #'stem #'% #'stem))
> +                          (if (equal? (syntax->datum field-default)
> +                                      (quote 'disabled))

Like above (eq? would do).  More importantly (and confusingly), here the
'disabled expected value must *not* be quoted.  I haven't investigated
why but it seems one level of quote got striped at that point.

> +                              (configuration-no-default-value
> +                               '#,(id #'stem #'% #'stem) 'field)
> +                              field-default)))
> +                      (documentation doc))
> +                     ...))
> +             (define-syntax-rule (stem arg (... ...))
> +               (let ((conf (#,(id #'stem #'% #'stem) arg (... ...))))
> +                 (validate-configuration conf
> +                                         #,(id #'stem #'stem #'-fields))
> +                 conf))))))))

The following patch implements the above comments;

modified   gnu/services/configuration.scm
@@ -66,7 +66,8 @@
    (format #f "~a configuration missing required field ~a" kind field)))
 (define (configuration-no-default-value kind field)
   (configuration-error
-   (format #f "`~a' in `~a' does not have a default value" kind field)))
+   (format #f "The field `~a' of the `~a' configuration record \
+does not have a default value" field kind)))
 
 (define-record-type* <configuration-field>
   configuration-field make-configuration-field configuration-field?
@@ -116,7 +117,7 @@
 (define-syntax define-configuration
   (lambda (stx)
     (syntax-case stx ()
-      ((_ stem (field (field-type properties ...) doc) ...)
+      ((_ stem (field (field-type def ...) doc) ...)
        (with-syntax (((field-getter ...)
                       (map (lambda (field)
                              (id #'stem #'stem #'- field))
@@ -127,11 +128,13 @@
                            #'(field-type ...)))
                      ((field-default ...)
                       (map (match-lambda
-                             ((field-type default _ ...) default)
-                             ;; We get warnings about `disabled' being an
-                             ;; unbound variable unless we quote it.
-                             (_ (syntax 'disabled)))
-                           #'((field-type properties ...) ...)))
+                             ((field-type default-value)
+                              default-value)
+                             ((field-type)
+                              ;; Quote `undefined' to prevent a possibly
+                              ;; unbound warning.
+                              (syntax 'undefined)))
+                           #'((field-type def ...) ...)))
                      ((field-serializer ...)
                       (map (lambda (type)
                              (id #'stem #'serialize- type))
@@ -146,7 +149,7 @@
                                           source-properties->location))
                           (innate))
                #,@(map (lambda (name getter def)
-                         (if (equal? (syntax->datum def) (quote 'disabled))
+                         (if (eq? (syntax->datum def) (quote 'undefined))
                              #`(#,name #,getter)
                              #`(#,name #,getter (default #,def))))
                        #'(field ...)
@@ -159,12 +162,11 @@
                       (getter field-getter)
                       (predicate field-predicate)
                       (serializer field-serializer)
-                      ;; TODO: What if there is no default value?
                       (default-value-thunk
                         (lambda ()
                           (display '#,(id #'stem #'% #'stem))
-                          (if (equal? (syntax->datum field-default)
-                                      (quote 'disabled))
+                          (if (eq? (syntax->datum field-default)
+                                   'undefined)
                               (configuration-no-default-value
                                '#,(id #'stem #'% #'stem) 'field)
                               field-default)))

I'll attempt to review patch 2/2 shortly!

Thanks a lot for this neat improvement!

Maxim

Re: Add a way to disable serialization support to (guix services configuration)

[Xinglu Chen]

Hi Maxim,

On Fri, May 07 2021, Maxim Cournoyer wrote:

> Hello Xinglu!
>
> Thank you for working on it!

You are very welcome!  These are things that have annoyed me enough so I
decided (try) to fix it myself :)

>> +(define (configuration-no-default-value kind field)
>> +  (configuration-error
>> +   (format #f "`~a' in `~a' does not have a default value" kind field)))
>
> The kind and field should be inverted.

Good catch

>>    configuration-field make-configuration-field configuration-field?
>> @@ -112,7 +116,7 @@
>>  (define-syntax define-configuration
>>    (lambda (stx)
>>      (syntax-case stx ()
>> -      ((_ stem (field (field-type def) doc) ...)
>> +      ((_ stem (field (field-type properties ...) doc) ...)
>
> I'd rather keep the 'def' binding for the default value; properties is
> too vague and implies many of them, which is not a supported syntax.

Yeah, not exactly sure why I changed it to ‘properties’, anyway...

>>         (with-syntax (((field-getter ...)
>>                        (map (lambda (field)
>>                               (id #'stem #'stem #'- field))
>> @@ -121,36 +125,56 @@
>>                        (map (lambda (type)
>>                               (id #'stem type #'?))
>>                             #'(field-type ...)))
>> +                     ((field-default ...)
>> +                      (map (match-lambda
>> +                             ((field-type default _ ...) default)
>> +                             ;; We get warnings about `disabled' being an
>> +                             ;; unbound variable unless we quote it.
>> +                             (_ (syntax 'disabled)))
>
> Here I think it'd be better to have the pattern more strict (e.g,
> (field-type default-value) or (field-type); so as to not accept invalid
> syntax.

Good point!

> I also think it'd be clearer to use another symbol than 'disabled, as
> this already has a meaning for the validator and could confuse readers.

Yeah, it’s not very descriptive.

>> +                           #'((field-type properties ...) ...)))
>>                       ((field-serializer ...)
>>                        (map (lambda (type)
>>                               (id #'stem #'serialize- type))
>>                             #'(field-type ...))))
>> -           #`(begin
>> -               (define-record-type* #,(id #'stem #'< #'stem #'>)
>> -                 #,(id #'stem #'% #'stem)
>> -                 #,(id #'stem #'make- #'stem)
>> -                 #,(id #'stem #'stem #'?)
>> -                 (%location #,(id #'stem #'-location)
>> -                            (default (and=> (current-source-location)
>> -                                            source-properties->location))
>> -                            (innate))
>> -                 (field field-getter (default def))
>> -                 ...)
>> -               (define #,(id #'stem #'stem #'-fields)
>> -                 (list (configuration-field
>> -                        (name 'field)
>> -                        (type 'field-type)
>> -                        (getter field-getter)
>> -                        (predicate field-predicate)
>> -                        (serializer field-serializer)
>> -                        (default-value-thunk (lambda () def))
>> -                        (documentation doc))
>> -                       ...))
>> -               (define-syntax-rule (stem arg (... ...))
>> -                 (let ((conf (#,(id #'stem #'% #'stem) arg (... ...))))
>> -                   (validate-configuration conf
>> -                                           #,(id #'stem #'stem #'-fields))
>> -                   conf))))))))
>> +         #`(begin
>> +             (define-record-type* #,(id #'stem #'< #'stem #'>)
>> +               #,(id #'stem #'% #'stem)
>> +               #,(id #'stem #'make- #'stem)
>> +               #,(id #'stem #'stem #'?)
>> +               (%location #,(id #'stem #'-location)
>> +                          (default (and=> (current-source-location)
>> +                                          source-properties->location))
>> +                          (innate))
>> +               #,@(map (lambda (name getter def)
>> +                         (if (equal? (syntax->datum def) (quote 'disabled))
>
> nitpick: eq? suffices to check for symbols.
>
>> +                             #`(#,name #,getter)
>> +                             #`(#,name #,getter (default #,def))))
>> +                       #'(field ...)
>> +                       #'(field-getter ...)
>> +                       #'(field-default ...)))
>> +             (define #,(id #'stem #'stem #'-fields)
>> +               (list (configuration-field
>> +                      (name 'field)
>> +                      (type 'field-type)
>> +                      (getter field-getter)
>> +                      (predicate field-predicate)
>> +                      (serializer field-serializer)
>> +                      ;; TODO: What if there is no default value?
>
> Seems this TODO was taken care of already :-).
>
>> +                      (default-value-thunk
>> +                        (lambda ()
>> +                          (display '#,(id #'stem #'% #'stem))
>> +                          (if (equal? (syntax->datum field-default)
>> +                                      (quote 'disabled))
>
> Like above (eq? would do).  More importantly (and confusingly), here the
> 'disabled expected value must *not* be quoted.  I haven't investigated
> why but it seems one level of quote got striped at that point.

Hmm, I am still a little confused about how quotation works with
syntax-case & friends.

>> +                              (configuration-no-default-value
>> +                               '#,(id #'stem #'% #'stem) 'field)
>> +                              field-default)))
>> +                      (documentation doc))
>> +                     ...))
>> +             (define-syntax-rule (stem arg (... ...))
>> +               (let ((conf (#,(id #'stem #'% #'stem) arg (... ...))))
>> +                 (validate-configuration conf
>> +                                         #,(id #'stem #'stem #'-fields))
>> +                 conf))))))))
>
> The following patch implements the above comments;
>
> [...]
>
> I'll attempt to review patch 2/2 shortly!
>
> Thanks a lot for this neat improvement!

Thank you for the review and the improvements!  

Re: Add a way to disable serialization support to (guix services configuration)

[Maxim Cournoyer]

Hi,

Xinglu Chen <public@yoctocell.xyz> writes:

[...]

> From 90d63a46a29a8080b7f95eabcec115c5c2c6481e Mon Sep 17 00:00:00 2001
> Message-Id: <90d63a46a29a8080b7f95eabcec115c5c2c6481e.1619869705.git.public@yoctocell.xyz>
> In-Reply-To: <cover.1619869705.git.public@yoctocell.xyz>
> References: <cover.1619869705.git.public@yoctocell.xyz>
> From: Xinglu Chen <public@yoctocell.xyz>
> Date: Sat, 1 May 2021 13:31:27 +0200
> Subject: [PATCH 2/2] services: configuration: Add ability to use custom
>  serializer.
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Some configuration values should not be serialized to a configuration file,
> e.g. command line options for a daemon.
>
> * gnu/services/configuration.scm (define-configuration): Add option to use a
> custom serializer.
> (empty-serializer): New procedure.
> (serialize-package): Alias to ‘empty-serializer’.
> ---
>  gnu/services/configuration.scm | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/gnu/services/configuration.scm b/gnu/services/configuration.scm
> index 85e1ac78cb..7024b13136 100644
> --- a/gnu/services/configuration.scm
> +++ b/gnu/services/configuration.scm
> @@ -44,6 +44,7 @@
>              define-configuration
>              validate-configuration
>              generate-documentation
> +            empty-serializer
>              serialize-package))
>  
>  ;;; Commentary:
> @@ -116,7 +117,7 @@
>  (define-syntax define-configuration
>    (lambda (stx)
>      (syntax-case stx ()
> -      ((_ stem (field (field-type properties ...) doc) ...)
> +      ((_ stem (field (field-type properties ...) doc custom-serializer ...) ...)
>         (with-syntax (((field-getter ...)
>                        (map (lambda (field)
>                               (id #'stem #'stem #'- field))
> @@ -133,9 +134,12 @@
>                               (_ (syntax 'disabled)))
>                             #'((field-type properties ...) ...)))
>                       ((field-serializer ...)
> -                      (map (lambda (type)
> -                             (id #'stem #'serialize- type))
> -                           #'(field-type ...))))
> +                      (map (lambda (type serializer)
> +                             (match serializer
> +                               (((serializer)) serializer)
> +                               (_ (id #'stem #'serialize- type))))
> +                           #'(field-type ...)
> +                           #'((custom-serializer ...) ...))))
>           #`(begin
>               (define-record-type* #,(id #'stem #'< #'stem #'>)
>                 #,(id #'stem #'% #'stem)
> @@ -176,8 +180,8 @@
>                                           #,(id #'stem #'stem #'-fields))
>                   conf))))))))
>  
> -(define (serialize-package field-name val)
> -  "")
> +(define (empty-serializer field-name val) "")
> +(define serialize-package empty-serializer)
>  
>  ;; A little helper to make it easier to document all those fields.
>  (define (generate-documentation documentation documentation-name)

Rebased on a change that allows to globally (at the configuration level)
disable serialization and pushed as b3e99d3399.

Thank you! :-)

Maxim

Re: Add a way to disable serialization support to (guix services configuration)

[Maxim Cournoyer]

Hello again,

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

[...]

> diff --git a/gnu/services/configuration.scm b/gnu/services/configuration.scm
> index 90f12a8d39..20e1647335 100644
> --- a/gnu/services/configuration.scm
> +++ b/gnu/services/configuration.scm
> @@ -109,6 +109,9 @@
>               (define (serialize-maybe-stem field-name val)
>                 (if (stem? val) (serialize-stem field-name val) ""))))))))
>
> +(define-syntax-parameter configuration-field-serialization?
> +  (identifier-syntax #t))
> +
>  (define-syntax define-configuration
>    (lambda (stx)
>      (syntax-case stx ()
> @@ -123,7 +126,8 @@
>                             #'(field-type ...)))
>                       ((field-serializer ...)
>                        (map (lambda (type)
> -                             (id #'stem #'serialize- type))
> +                             #`(and configuration-field-serialization?
> +                                    #,(id #'stem #'serialize- type)))
>                             #'(field-type ...))))
>             #`(begin
>                 (define-record-type* #,(id #'stem #'< #'stem #'>)
> @@ -152,6 +156,16 @@
>                                             #,(id #'stem #'stem #'-fields))
>                     conf))))))))
>
> +(define-syntax-rule (without-field-serialization definition)
> +  (syntax-parameterize ((configuration-field-serialization?
> +                         (identifier-syntax #f)))
> +    definition
> +    #t))
> +
> +(without-field-serialization
> +  (define-configuration foo
> +    (bar (integer 123) "doc")))
> +
>  (define (serialize-package field-name val)
>    "")

Actually, this doesn't seem to work; when exporting the
'without-field-serialization' from this module and using it in another
module (wrapping a (define-configuration ...) form), the compiler
complains that the 'foo' variable is not defined; perhaps because foo is
exported as part of the module definition?

If you have an idea why, hints welcome.

Maxim