* [bug#56075] [PATCH 0/2] Report location of invalid configuration field values
@ 2022-06-18 21:36 Ludovic Courtès
2022-06-18 21:38 ` [bug#56075] [PATCH 1/2] services: configuration: Report the location of field type errors Ludovic Courtès
0 siblings, 1 reply; 6+ messages in thread
From: Ludovic Courtès @ 2022-06-18 21:36 UTC (permalink / raw)
To: 56075; +Cc: Ludovic Courtès
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [bug#56075] [PATCH 1/2] services: configuration: Report the location of field type errors.
2022-06-18 21:36 [bug#56075] [PATCH 0/2] Report location of invalid configuration field values Ludovic Courtès
@ 2022-06-18 21:38 ` Ludovic Courtès
2022-06-18 21:38 ` [bug#56075] [PATCH 2/2] services: configuration: Remove 'validate-configuration' Ludovic Courtès
2022-06-23 16:05 ` [bug#56075] [PATCH 1/2] services: configuration: Report the location of field type errors Maxim Cournoyer
0 siblings, 2 replies; 6+ messages in thread
From: Ludovic Courtès @ 2022-06-18 21:38 UTC (permalink / raw)
To: 56075; +Cc: Ludovic Courtès
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [bug#56075] [PATCH 2/2] services: configuration: Remove 'validate-configuration'.
2022-06-18 21:38 ` [bug#56075] [PATCH 1/2] services: configuration: Report the location of field type errors Ludovic Courtès
@ 2022-06-18 21:38 ` Ludovic Courtès
2022-06-23 18:30 ` Maxim Cournoyer
2022-06-23 16:05 ` [bug#56075] [PATCH 1/2] services: configuration: Report the location of field type errors Maxim Cournoyer
1 sibling, 1 reply; 6+ messages in thread
From: Ludovic Courtès @ 2022-06-18 21:38 UTC (permalink / raw)
To: 56075; +Cc: Ludovic Courtès
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [bug#56075] [PATCH 1/2] services: configuration: Report the location of field type errors.
2022-06-18 21:38 ` [bug#56075] [PATCH 1/2] services: configuration: Report the location of field type errors Ludovic Courtès
2022-06-18 21:38 ` [bug#56075] [PATCH 2/2] services: configuration: Remove 'validate-configuration' Ludovic Courtès
@ 2022-06-23 16:05 ` Maxim Cournoyer
2022-06-24 21:43 ` bug#56075: [PATCH 0/2] Report location of invalid configuration field values Ludovic Courtès
1 sibling, 1 reply; 6+ messages in thread
From: Maxim Cournoyer @ 2022-06-23 16:05 UTC (permalink / raw)
To: Ludovic Courtès; +Cc: 56075
Hello,
Ludovic Courtès <ludo@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [bug#56075] [PATCH 2/2] services: configuration: Remove 'validate-configuration'.
2022-06-18 21:38 ` [bug#56075] [PATCH 2/2] services: configuration: Remove 'validate-configuration' Ludovic Courtès
@ 2022-06-23 18:30 ` Maxim Cournoyer
0 siblings, 0 replies; 6+ messages in thread
From: Maxim Cournoyer @ 2022-06-23 18:30 UTC (permalink / raw)
To: Ludovic Courtès; +Cc: 56075
Hi,
Ludovic Courtès <ludo@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#56075: [PATCH 0/2] Report location of invalid configuration field values
2022-06-23 16:05 ` [bug#56075] [PATCH 1/2] services: configuration: Report the location of field type errors Maxim Cournoyer
@ 2022-06-24 21:43 ` Ludovic Courtès
0 siblings, 0 replies; 6+ messages in thread
From: Ludovic Courtès @ 2022-06-24 21:43 UTC (permalink / raw)
To: Maxim Cournoyer; +Cc: 56075-done
Hi,
Maxim Cournoyer <maxim.cournoyer@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’.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-06-24 21:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-18 21:36 [bug#56075] [PATCH 0/2] Report location of invalid configuration field values Ludovic Courtès
2022-06-18 21:38 ` [bug#56075] [PATCH 1/2] services: configuration: Report the location of field type errors Ludovic Courtès
2022-06-18 21:38 ` [bug#56075] [PATCH 2/2] services: configuration: Remove 'validate-configuration' Ludovic Courtès
2022-06-23 18:30 ` Maxim Cournoyer
2022-06-23 16:05 ` [bug#56075] [PATCH 1/2] services: configuration: Report the location of field type errors Maxim Cournoyer
2022-06-24 21:43 ` bug#56075: [PATCH 0/2] Report location of invalid configuration field values Ludovic Courtès
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).