GNU bug report logs - #51922
[PATCH 0/2] Improve the reported location of configuration warnings

Previous Next

Package: guix-patches;

Reported by: Josselin Poiret <dev <at> jpoiret.xyz>

Date: Wed, 17 Nov 2021 14:41: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 51922 in the body.
You can then email your comments to 51922 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


Report forwarded to guix-patches <at> gnu.org:
bug#51922; Package guix-patches. (Wed, 17 Nov 2021 14:41:01 GMT) Full text and rfc822 format available.

Acknowledgement sent to Josselin Poiret <dev <at> jpoiret.xyz>:
New bug report received and forwarded. Copy sent to guix-patches <at> gnu.org. (Wed, 17 Nov 2021 14:41:01 GMT) Full text and rfc822 format available.

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

From: Josselin Poiret <dev <at> jpoiret.xyz>
To: guix-patches <at> gnu.org
Cc: Josselin Poiret <dev <at> jpoiret.xyz>
Subject: [PATCH 0/2] Improve the reported location of configuration warnings
Date: Wed, 17 Nov 2021 14:40:28 +0000
Hello everyone,

While working on the swap-space patch, I noticed that currently,
warnings about deprecated fields in guix records use the location of
the record definition macro, rather than of the invalid value.  For
some records such as 'operating-system', this makes it rather user
unfriendly and confusing.

This patchset first adds the syntax 'define-with-syntax-properties',
which helps avoid boilerplate code to define sanitizers with proper
location reporting.  I put it in guix/diagnostics.scm as I thought
this was the place that was most likely to be use-module'd for warning
messages, as this is quite tied to that use.  The second patch makes
use of this new helper to update two warnings: the one about 'target'
to 'targets' in bootloader.scm, and the one about setuid-programs.  In
both cases, a `guix system reconfigure` now reports the exact location
of the incriminating values, rather than of the
'bootloader-configuration' or 'operating-system' lines respectively.

The approach I've taken for the helper was to make something simple
and general enough for most uses.  It should correctly report syntax
errors with custom errors messages rather than the generic "source
expression failed to match any pattern ...".  Note although that it
isn't possible to do any defines in the body of the macro, as this
doesn't use any lambda-like macros (see the second case for an
example).

I can see two drawbacks to this macro:
1) This macro will not help you write expand-time checkers.  This
would introduce too much complexity, and I'm not sure the end-user
would notice a significant change.
2) It doesn't deconstruct values such as lists to get the individual
list values's properties.  This would also introduce too much
complexity (eg. checking if the list is literal, deconstructing it,
and falling back to a generic source location if the list is only
available at eval-time).

Best,
Josselin Poiret

Josselin Poiret (2):
  guix: Add syntax to capture arguments' syntax-properties.
  gnu: system: Improve location of some configuration warnings.

 gnu/bootloader.scm   | 16 +++++++---------
 gnu/system.scm       | 11 ++++++-----
 guix/diagnostics.scm | 38 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 50 insertions(+), 15 deletions(-)

-- 
2.33.1





Information forwarded to guix-patches <at> gnu.org:
bug#51922; Package guix-patches. (Wed, 17 Nov 2021 14:44:02 GMT) Full text and rfc822 format available.

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

From: Josselin Poiret <dev <at> jpoiret.xyz>
To: 51922 <at> debbugs.gnu.org
Cc: Josselin Poiret <dev <at> jpoiret.xyz>
Subject: [PATCH 2/2] gnu: system: Improve location of some configuration
 warnings.
Date: Wed, 17 Nov 2021 14:43:48 +0000
* gnu/bootloader.scm (%warn-target-field-deprecation): Remove it.
* gnu/bootloader.scm (warn-target-field-deprecation): Use
define-with-syntax-properties.
* gnu/system.scm (ensure-setuid-program-list): Ditto.  Also rename the
'location' variable to 'properties'.
---
 gnu/bootloader.scm | 16 +++++++---------
 gnu/system.scm     | 11 ++++++-----
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/gnu/bootloader.scm b/gnu/bootloader.scm
index d1c72c0c85..9cf5457873 100644
--- a/gnu/bootloader.scm
+++ b/gnu/bootloader.scm
@@ -183,8 +183,13 @@ (define-record-type* <bootloader>
 ;; The <bootloader-configuration> record contains bootloader independant
 ;; configuration used to fill bootloader configuration file.
 
-(define-syntax-rule (warn-target-field-deprecation value)
-  (%warn-target-field-deprecation value (current-source-location)))
+(define-with-syntax-properties (warn-target-field-deprecation
+                                (value properties))
+  (when value
+    (warning (source-properties->location properties)
+             (G_ "the 'target' field is deprecated, please use 'targets' \
+instead~%")))
+  value)
 
 (define-record-type* <bootloader-configuration>
   bootloader-configuration make-bootloader-configuration
@@ -213,13 +218,6 @@ (define-record-type* <bootloader-configuration>
   (serial-speed       bootloader-configuration-serial-speed ;integer | #f
                       (default #f)))
 
-(define (%warn-target-field-deprecation value location)
-  (when value
-    (warning (source-properties->location location)
-             (G_ "the 'target' field is deprecated, please use 'targets' \
-instead~%")))
-  value)
-
 (define-deprecated (bootloader-configuration-target config)
   bootloader-configuration-targets
   (%bootloader-configuration-target config))
diff --git a/gnu/system.scm b/gnu/system.scm
index 03fb55db15..9de0f79b44 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -1107,16 +1107,17 @@ (define (operating-system-environment-variables os)
     ;; TODO: Remove when glibc <at> 2.23 is long gone.
     ("GUIX_LOCPATH" . "/run/current-system/locale")))
 
-(define-syntax-rule (ensure-setuid-program-list lst)
-  "Ensure LST is a list of <setuid-program> records and warn otherwise."
-  (%ensure-setuid-program-list lst (current-source-location)))
+;; Ensure LST is a list of <setuid-program> records and warn otherwise.
+(define-with-syntax-properties (ensure-setuid-program-list (lst properties))
+  (%ensure-setuid-program-list lst properties))
 
-(define (%ensure-setuid-program-list lst location)
+;; We want to be able to use defines, so define a procedure.
+(define (%ensure-setuid-program-list lst properties)
   (define warned? #f)
 
   (define (warn-once)
     (unless warned?
-      (warning (source-properties->location location)
+      (warning (source-properties->location properties)
                (G_ "representing setuid programs with file-like objects is \
 deprecated; use 'setuid-program' instead~%"))
       (set! warned? #t)))
-- 
2.33.1





Information forwarded to guix-patches <at> gnu.org:
bug#51922; Package guix-patches. (Wed, 17 Nov 2021 14:45:02 GMT) Full text and rfc822 format available.

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

From: Josselin Poiret <dev <at> jpoiret.xyz>
To: 51922 <at> debbugs.gnu.org
Cc: Josselin Poiret <dev <at> jpoiret.xyz>
Subject: [PATCH 1/2] guix: Add syntax to capture arguments' syntax-properties.
Date: Wed, 17 Nov 2021 14:43:47 +0000
* guix/diagnostics.scm (define-with-syntax-properties): Add it.
---
 guix/diagnostics.scm | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/guix/diagnostics.scm b/guix/diagnostics.scm
index 6a792febd4..337a73c1a2 100644
--- a/guix/diagnostics.scm
+++ b/guix/diagnostics.scm
@@ -54,7 +54,9 @@ (define-module (guix diagnostics)
             condition-fix-hint
 
             guix-warning-port
-            program-name))
+            program-name
+
+            define-with-syntax-properties))
 
 ;;; Commentary:
 ;;;
@@ -331,3 +333,37 @@ (define guix-warning-port
 (define program-name
   ;; Name of the command-line program currently executing, or #f.
   (make-parameter #f))
+
+
+(define-syntax define-with-syntax-properties
+  (lambda (x)
+    "Define BINDING to be a syntax form replacing each VALUE-IDENTIFIER and
+SYNTAX-PROPERTIES-IDENTIFIER in body by the syntax and syntax-properties,
+respectively, of each ensuing syntax object."
+    (syntax-case x ()
+      ((_ (binding (value-identifier syntax-properties-identifier)
+                   ...)
+          body ...)
+       (and (and-map identifier? #'(value-identifier ...))
+            (and-map identifier? #'(syntax-properties-identifier ...)))
+       #'(define-syntax binding
+           (lambda (y)
+             (with-ellipsis :::
+               (syntax-case y ()
+                 ((_ value-identifier ...)
+                  (with-syntax ((syntax-properties-identifier
+                                 #`'#,(datum->syntax y
+                                                     (syntax-source
+                                                      #'value-identifier)))
+                                ...)
+                    #'(begin body ...)))
+                 (_
+                  (syntax-violation #f (format #f
+                                               "Expected (~a~{ ~a~})"
+                                               'binding
+                                               '(value-identifier ...))
+                                    y)))))))
+      (_
+       (syntax-violation #f "Expected a definition of the form \
+(define-with-syntax-properties (binding (value syntax-properties) \
+...) body ...)" x)))))
-- 
2.33.1





Reply sent to Ludovic Courtès <ludo <at> gnu.org>:
You have taken responsibility. (Wed, 17 Nov 2021 16:59:02 GMT) Full text and rfc822 format available.

Notification sent to Josselin Poiret <dev <at> jpoiret.xyz>:
bug acknowledged by developer. (Wed, 17 Nov 2021 16:59:02 GMT) Full text and rfc822 format available.

Message #16 received at 51922-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Josselin Poiret <dev <at> jpoiret.xyz>
Cc: 51922-done <at> debbugs.gnu.org
Subject: Re: bug#51922: [PATCH 0/2] Improve the reported location of
 configuration warnings
Date: Wed, 17 Nov 2021 17:58:14 +0100
Hi,

Josselin Poiret <dev <at> jpoiret.xyz> skribis:

> While working on the swap-space patch, I noticed that currently,
> warnings about deprecated fields in guix records use the location of
> the record definition macro, rather than of the invalid value.  For
> some records such as 'operating-system', this makes it rather user
> unfriendly and confusing.
>
> This patchset first adds the syntax 'define-with-syntax-properties',
> which helps avoid boilerplate code to define sanitizers with proper
> location reporting.  I put it in guix/diagnostics.scm as I thought
> this was the place that was most likely to be use-module'd for warning
> messages, as this is quite tied to that use.  The second patch makes
> use of this new helper to update two warnings: the one about 'target'
> to 'targets' in bootloader.scm, and the one about setuid-programs.  In
> both cases, a `guix system reconfigure` now reports the exact location
> of the incriminating values, rather than of the
> 'bootloader-configuration' or 'operating-system' lines respectively.

Neat!  That’s a much welcome improvement.

> The approach I've taken for the helper was to make something simple
> and general enough for most uses.  It should correctly report syntax
> errors with custom errors messages rather than the generic "source
> expression failed to match any pattern ...".  Note although that it
> isn't possible to do any defines in the body of the macro, as this
> doesn't use any lambda-like macros (see the second case for an
> example).

I think that’s OK, that’s a reasonable approach.

> I can see two drawbacks to this macro:
> 1) This macro will not help you write expand-time checkers.  This
> would introduce too much complexity, and I'm not sure the end-user
> would notice a significant change.
> 2) It doesn't deconstruct values such as lists to get the individual
> list values's properties.  This would also introduce too much
> complexity (eg. checking if the list is literal, deconstructing it,
> and falling back to a generic source location if the list is only
> available at eval-time).

Again, I think that’s fine: this macro solves what it was designed to
address, and if we need something fancier, we can think about it later.

>   guix: Add syntax to capture arguments' syntax-properties.
>   gnu: system: Improve location of some configuration warnings.

I tweaked the first commit log and applied it.

Thank you!

Ludo’.




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Thu, 16 Dec 2021 12:24:07 GMT) Full text and rfc822 format available.

This bug report was last modified 2 years and 93 days ago.

Previous Next


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