Subject: Re: bug#51922: [PATCH 0/2] Improve the reported location of configuration warnings
From: Ludovic Courtès <[email-masked]>
To: Josselin Poiret <[email-masked]>
Cc: [email-masked]
Date: Wed, 17 Nov 2021 17:58:14 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

Hi,

Josselin Poiret <[email-masked]> 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’.