Package: guix-patches;
Reported by: Ludovic Courtès <ludo <at> gnu.org>
Date: Sat, 18 Jun 2022 21:38:01 UTC
Severity: normal
Tags: patch
Done: Ludovic Courtès <ludo <at> gnu.org>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 56075 in the body.
You can then email your comments to 56075 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
guix-patches <at> gnu.org:bug#56075; Package guix-patches.
(Sat, 18 Jun 2022 21:38:01 GMT) Full text and rfc822 format available.Ludovic Courtès <ludo <at> gnu.org>:guix-patches <at> gnu.org.
(Sat, 18 Jun 2022 21:38:02 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: guix-patches <at> gnu.org Cc: Ludovic Courtès <ludo <at> gnu.org> Subject: [PATCH 0/2] Report location of invalid configuration field values Date: Sat, 18 Jun 2022 23:36:40 +0200
Hello Guix! This change has ‘define-configuration’ use the ‘sanitize’ property to type-check fields instead of a custom mechanism. More importantly, it improves error reporting of invalid field value such that instead of: guix home: error: Invalid value for field latitude: "44.81" you get the location of the faulty field value: home-config.scm:391:23: error: invalid value "44.81" for field 'latitude' Additionally the message is now internationalized. Thoughts? Ludo’. Ludovic Courtès (2): services: configuration: Report the location of field type errors. services: configuration: Remove 'validate-configuration'. doc/guix.texi | 6 --- gnu/services/configuration.scm | 64 +++++++++++++++++++++----------- gnu/services/mail.scm | 6 +-- gnu/services/vpn.scm | 2 - po/guix/POTFILES.in | 1 + tests/services/configuration.scm | 12 ++++++ 6 files changed, 57 insertions(+), 34 deletions(-) base-commit: 7f208f68dea828fe02718ca8ce81d5975136cff8 -- 2.36.1
guix-patches <at> gnu.org:bug#56075; Package guix-patches.
(Sat, 18 Jun 2022 21:39:01 GMT) Full text and rfc822 format available.Message #8 received at 56075 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: 56075 <at> debbugs.gnu.org Cc: Ludovic Courtès <ludo <at> gnu.org> Subject: [PATCH 1/2] services: configuration: Report the location of field type errors. Date: Sat, 18 Jun 2022 23:38:31 +0200
Previously field type errors would be reported in a non-standard way,
and without any source location information. This fixes it.
* gnu/services/configuration.scm (configuration-field-error): Add a
'loc' parameter and honor it. Use 'formatted-message' instead of plain
'format'.
(define-configuration-helper)[field-sanitizer]: New procedure.
Use it. Use STEM as the identifier of the syntactic constructor of the
record type. Add a 'sanitize' property to each field. Remove now
useless STEM macro that would call 'validate-configuration'.
* gnu/services/mail.scm (serialize-listener-configuration): Adjust to
new 'configuration-field-error' prototype.
* tests/services/configuration.scm ("wrong type for a field"): New test.
* po/guix/POTFILES.in: Add gnu/services/configuration.scm.
---
gnu/services/configuration.scm | 55 +++++++++++++++++++++++++-------
gnu/services/mail.scm | 2 +-
po/guix/POTFILES.in | 1 +
tests/services/configuration.scm | 12 +++++++
4 files changed, 57 insertions(+), 13 deletions(-)
diff --git a/gnu/services/configuration.scm b/gnu/services/configuration.scm
index f6b20fb82b..c39ea5a02a 100644
--- a/gnu/services/configuration.scm
+++ b/gnu/services/configuration.scm
@@ -27,7 +27,8 @@ (define-module (gnu services configuration)
#:use-module (guix records)
#:use-module (guix gexp)
#:use-module ((guix utils) #:select (source-properties->location))
- #:use-module ((guix diagnostics) #:select (formatted-message location-file))
+ #:use-module ((guix diagnostics)
+ #:select (formatted-message location-file &error-location))
#:use-module ((guix modules) #:select (file-name->module-name))
#:use-module (guix i18n)
#:autoload (texinfo) (texi-fragment->stexi)
@@ -87,9 +88,17 @@ (define-condition-type &configuration-error &error
(define (configuration-error message)
(raise (condition (&message (message message))
(&configuration-error))))
-(define (configuration-field-error field val)
- (configuration-error
- (format #f "Invalid value for field ~a: ~s" field val)))
+(define (configuration-field-error loc field value)
+ (raise (apply
+ make-compound-condition
+ (formatted-message (G_ "invalid value ~s for field '~a'")
+ value field)
+ (condition (&configuration-error))
+ (if loc
+ (list (condition
+ (&error-location (location loc))))
+ '()))))
+
(define (configuration-missing-field kind field)
(configuration-error
(format #f "~a configuration missing required field ~a" kind field)))
@@ -210,9 +219,33 @@ (define (define-configuration-helper serialize? serializer-prefix syn)
(id #'stem #'serialize- type))))))
#'(field-type ...)
#'((custom-serializer ...) ...))))
+ (define (field-sanitizer name pred)
+ ;; Define a macro for use as a record field sanitizer, where NAME
+ ;; is the name of the field and PRED is the predicate that tells
+ ;; whether a value is valid for this field.
+ #`(define-syntax #,(id #'stem #'validate- #'stem #'- name)
+ (lambda (s)
+ ;; Make sure the given VALUE, for field NAME, passes PRED.
+ (syntax-case s ()
+ ((_ value)
+ (with-syntax ((name #'#,name)
+ (pred #'#,pred)
+ (loc (datum->syntax #'value
+ (syntax-source #'value))))
+ #'(if (pred value)
+ value
+ (configuration-field-error
+ (and=> 'loc source-properties->location)
+ 'name value))))))))
+
#`(begin
+ ;; Define field validation macros.
+ #,@(map field-sanitizer
+ #'(field ...)
+ #'(field-predicate ...))
+
(define-record-type* #,(id #'stem #'< #'stem #'>)
- #,(id #'stem #'% #'stem)
+ stem
#,(id #'stem #'make- #'stem)
#,(id #'stem #'stem #'?)
(%location #,(id #'stem #'stem #'-location)
@@ -220,10 +253,13 @@ (define-record-type* #,(id #'stem #'< #'stem #'>)
source-properties->location))
(innate))
#,@(map (lambda (name getter def)
- #`(#,name #,getter (default #,def)))
+ #`(#,name #,getter (default #,def)
+ (sanitize
+ #,(id #'stem #'validate- #'stem #'- name))))
#'(field ...)
#'(field-getter ...)
#'(field-default ...)))
+
(define #,(id #'stem #'stem #'-fields)
(list (configuration-field
(name 'field)
@@ -240,12 +276,7 @@ (define #,(id #'stem #'stem #'-fields)
'#,(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 no-serialization ;syntactic keyword for 'define-configuration'
'(no serialization))
diff --git a/gnu/services/mail.scm b/gnu/services/mail.scm
index d99743ac31..c2fd4d8670 100644
--- a/gnu/services/mail.scm
+++ b/gnu/services/mail.scm
@@ -285,7 +285,7 @@ (define (serialize-listener-configuration field-name val)
(serialize-fifo-listener-configuration field-name val))
((inet-listener-configuration? val)
(serialize-inet-listener-configuration field-name val))
- (else (configuration-field-error field-name val))))
+ (else (configuration-field-error #f field-name val))))
(define (listener-configuration-list? val)
(and (list? val) (and-map listener-configuration? val)))
(define (serialize-listener-configuration-list field-name val)
diff --git a/po/guix/POTFILES.in b/po/guix/POTFILES.in
index 201e5dcc87..f50dd00422 100644
--- a/po/guix/POTFILES.in
+++ b/po/guix/POTFILES.in
@@ -4,6 +4,7 @@ gnu.scm
gnu/packages.scm
gnu/services.scm
gnu/system.scm
+gnu/services/configuration.scm
gnu/services/shepherd.scm
gnu/home/services.scm
gnu/home/services/ssh.scm
diff --git a/tests/services/configuration.scm b/tests/services/configuration.scm
index 334a1e409b..cf3e504233 100644
--- a/tests/services/configuration.scm
+++ b/tests/services/configuration.scm
@@ -19,6 +19,7 @@
(define-module (tests services configuration)
#:use-module (gnu services configuration)
+ #:use-module (guix diagnostics)
#:use-module (guix gexp)
#:use-module (srfi srfi-34)
#:use-module (srfi srfi-64))
@@ -43,6 +44,17 @@ (define-configuration port-configuration
80
(port-configuration-port (port-configuration)))
+(test-equal "wrong type for a field"
+ '("configuration.scm" 56 11) ;error location
+ (guard (c ((configuration-error? c)
+ (let ((loc (error-location c)))
+ (list (basename (location-file loc))
+ (location-line loc)
+ (location-column loc)))))
+ (port-configuration
+ ;; This is line 55; the test relies on line/column numbers!
+ (port "This is not a number!"))))
+
(define-configuration port-configuration-cs
(port (number 80) "The port number." empty-serializer))
--
2.36.1
guix-patches <at> gnu.org:bug#56075; Package guix-patches.
(Sat, 18 Jun 2022 21:39:01 GMT) Full text and rfc822 format available.Message #11 received at 56075 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: 56075 <at> debbugs.gnu.org Cc: Ludovic Courtès <ludo <at> gnu.org> Subject: [PATCH 2/2] services: configuration: Remove 'validate-configuration'. Date: Sat, 18 Jun 2022 23:38:32 +0200
Now that configuration records use the 'sanitize' property for each
field, 'validate-configuration' has become useless because it's
impossible to construct an invalid configuration record.
* gnu/services/configuration.scm (validate-configuration): Remove.
* gnu/services/mail.scm (dovecot-service): Remove call.
* gnu/services/vpn.scm (openvpn-client-service)
(openvpn-server-service): Likewise.
* doc/guix.texi (Complex Configurations): Remove documentation.
---
doc/guix.texi | 6 ------
gnu/services/configuration.scm | 9 ---------
gnu/services/mail.scm | 4 ----
gnu/services/vpn.scm | 2 --
4 files changed, 21 deletions(-)
diff --git a/doc/guix.texi b/doc/guix.texi
index 86348fc02c..45f2620476 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -38886,12 +38886,6 @@ Return a G-expression that contains the values corresponding to the
disk by using something like @code{mixed-text-file}.
@end deffn
-@deffn {Scheme Procedure} validate-configuration @var{configuration}
-@var{fields}
-Type-check @var{fields}, a list of field names of @var{configuration}, a
-configuration record created by @code{define-configuration}.
-@end deffn
-
@deffn {Scheme Procedure} empty-serializer @var{field-name} @var{value}
A serializer that just returns an empty string. The
@code{serialize-package} procedure is an alias for this.
diff --git a/gnu/services/configuration.scm b/gnu/services/configuration.scm
index c39ea5a02a..e3c101d042 100644
--- a/gnu/services/configuration.scm
+++ b/gnu/services/configuration.scm
@@ -57,7 +57,6 @@ (define-module (gnu services configuration)
serialize-configuration
define-maybe
define-maybe/no-serialization
- validate-configuration
generate-documentation
configuration->documentation
empty-serializer
@@ -125,14 +124,6 @@ (define (serialize-configuration config fields)
((configuration-field-getter field) config)))
fields)))
-(define (validate-configuration config fields)
- (for-each (lambda (field)
- (let ((val ((configuration-field-getter field) config)))
- (unless ((configuration-field-predicate field) val)
- (configuration-field-error
- (configuration-field-name field) val))))
- fields))
-
(define-syntax-rule (id ctx parts ...)
"Assemble PARTS into a raw (unhygienic) identifier."
(datum->syntax ctx (symbol-append (syntax->datum parts) ...)))
diff --git a/gnu/services/mail.scm b/gnu/services/mail.scm
index c2fd4d8670..10e6523861 100644
--- a/gnu/services/mail.scm
+++ b/gnu/services/mail.scm
@@ -1610,10 +1610,6 @@ (define* (dovecot-service #:key (config (dovecot-configuration)))
by @code{dovecot-configuration}. @var{config} may also be created by
@code{opaque-dovecot-configuration}, which allows specification of the
@code{dovecot.conf} as a string."
- (validate-configuration config
- (if (opaque-dovecot-configuration? config)
- opaque-dovecot-configuration-fields
- dovecot-configuration-fields))
(service dovecot-service-type config))
;; A little helper to make it easier to document all those fields.
diff --git a/gnu/services/vpn.scm b/gnu/services/vpn.scm
index 8be632d55f..ec9ef6b6f0 100644
--- a/gnu/services/vpn.scm
+++ b/gnu/services/vpn.scm
@@ -540,11 +540,9 @@ (define openvpn-client-service-type
to an existing @acronym{VPN, virtual private network}.")))
(define* (openvpn-client-service #:key (config (openvpn-client-configuration)))
- (validate-configuration config openvpn-client-configuration-fields)
(service openvpn-client-service-type config))
(define* (openvpn-server-service #:key (config (openvpn-server-configuration)))
- (validate-configuration config openvpn-server-configuration-fields)
(service openvpn-server-service-type config))
(define (generate-openvpn-server-documentation)
--
2.36.1
guix-patches <at> gnu.org:bug#56075; Package guix-patches.
(Thu, 23 Jun 2022 16:06:02 GMT) Full text and rfc822 format available.Message #14 received at 56075 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ludovic Courtès <ludo <at> gnu.org> Cc: 56075 <at> debbugs.gnu.org Subject: Re: [bug#56075] [PATCH 1/2] services: configuration: Report the location of field type errors. Date: Thu, 23 Jun 2022 12:05:16 -0400
Hello,
Ludovic Courtès <ludo <at> gnu.org> writes:
> Previously field type errors would be reported in a non-standard way,
> and without any source location information. This fixes it.
>
> * gnu/services/configuration.scm (configuration-field-error): Add a
> 'loc' parameter and honor it. Use 'formatted-message' instead of plain
> 'format'.
> (define-configuration-helper)[field-sanitizer]: New procedure.
> Use it. Use STEM as the identifier of the syntactic constructor of the
> record type. Add a 'sanitize' property to each field. Remove now
> useless STEM macro that would call 'validate-configuration'.
> * gnu/services/mail.scm (serialize-listener-configuration): Adjust to
> new 'configuration-field-error' prototype.
> * tests/services/configuration.scm ("wrong type for a field"): New test.
> * po/guix/POTFILES.in: Add gnu/services/configuration.scm.
Very nice! I had been meaning to look at what define-configure could be
improved w.r.t. the recently added sanitizers; I felt perhaps
`define-configure' would perhaps loose its relevance, but I'm happy to
see you saw value in upgrading it!
The first part LGTM, although I so rarely dabble with syntax-case that
it looks a bit alien to my eyes now.
[...]
> --- a/tests/services/configuration.scm
> +++ b/tests/services/configuration.scm
> @@ -19,6 +19,7 @@
You forgot to add your copyright notice line.
> (define-module (tests services configuration)
> #:use-module (gnu services configuration)
> + #:use-module (guix diagnostics)
> #:use-module (guix gexp)
> #:use-module (srfi srfi-34)
> #:use-module (srfi srfi-64))
> @@ -43,6 +44,17 @@ (define-configuration port-configuration
> 80
> (port-configuration-port (port-configuration)))
>
> +(test-equal "wrong type for a field"
> + '("configuration.scm" 56 11) ;error location
> + (guard (c ((configuration-error? c)
> + (let ((loc (error-location c)))
> + (list (basename (location-file loc))
> + (location-line loc)
> + (location-column loc)))))
> + (port-configuration
> + ;; This is line 55; the test relies on line/column numbers!
> + (port "This is not a number!"))))
> +
It seems fragile to rely on the line/column number, but if we truly need
to test that, I don't see a better options.
Thanks,
Maxim
guix-patches <at> gnu.org:bug#56075; Package guix-patches.
(Thu, 23 Jun 2022 18:32:02 GMT) Full text and rfc822 format available.Message #17 received at 56075 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ludovic Courtès <ludo <at> gnu.org> Cc: 56075 <at> debbugs.gnu.org Subject: Re: [bug#56075] [PATCH 2/2] services: configuration: Remove 'validate-configuration'. Date: Thu, 23 Jun 2022 14:30:57 -0400
Hi, Ludovic Courtès <ludo <at> gnu.org> writes: > Now that configuration records use the 'sanitize' property for each > field, 'validate-configuration' has become useless because it's > impossible to construct an invalid configuration record. > > * gnu/services/configuration.scm (validate-configuration): Remove. > * gnu/services/mail.scm (dovecot-service): Remove call. > * gnu/services/vpn.scm (openvpn-client-service) > (openvpn-server-service): Likewise. > * doc/guix.texi (Complex Configurations): Remove documentation. I tried "make check-system TESTS='prosody jami-provisioning'" and they pass. I guess it's good to go. Thank you! Maxim
Ludovic Courtès <ludo <at> gnu.org>:Ludovic Courtès <ludo <at> gnu.org>:Message #22 received at 56075-done <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 56075-done <at> debbugs.gnu.org Subject: Re: bug#56075: [PATCH 0/2] Report location of invalid configuration field values Date: Fri, 24 Jun 2022 23:43:50 +0200
Hi,
Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
> Very nice! I had been meaning to look at what define-configure could be
> improved w.r.t. the recently added sanitizers; I felt perhaps
> `define-configure' would perhaps loose its relevance, but I'm happy to
> see you saw value in upgrading it!
Yeah, the mechanism that ‘define-configuration’ had had become
redundant.
> The first part LGTM, although I so rarely dabble with syntax-case that
> it looks a bit alien to my eyes now.
Well, ‘define-configuration’ is a bit hairy. :-)
>> +++ b/tests/services/configuration.scm
>> @@ -19,6 +19,7 @@
>
> You forgot to add your copyright notice line.
Fixed.
>> +(test-equal "wrong type for a field"
>> + '("configuration.scm" 56 11) ;error location
>> + (guard (c ((configuration-error? c)
>> + (let ((loc (error-location c)))
>> + (list (basename (location-file loc))
>> + (location-line loc)
>> + (location-column loc)))))
>> + (port-configuration
>> + ;; This is line 55; the test relies on line/column numbers!
>> + (port "This is not a number!"))))
>> +
>
> It seems fragile to rely on the line/column number, but if we truly need
> to test that, I don't see a better options.
Yeah; there is another option, which is to read code from a string port
and to simulate its location, but it’s more lines of code for little
IMO.
Pushed, thanks!
6505f727e1 services: configuration: Remove 'validate-configuration'.
fb7e6ccba7 services: configuration: Report the location of field type errors.
Ludo’.
Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org.
(Sat, 23 Jul 2022 11:24:10 GMT) Full text and rfc822 format available.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.