all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
To: guix-devel@gnu.org
Subject: Re: Layout of ‘define-configuration’ records
Date: Fri, 18 Nov 2022 11:44:53 -0500	[thread overview]
Message-ID: <8735ag43ga.fsf@gmail.com> (raw)
In-Reply-To: <87zgcpdx6q.fsf_-_@gnu.org> ("Ludovic Courtès"'s message of "Thu, 17 Nov 2022 23:37:49 +0100")

Hi Ludo,

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

> Hi,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> This is so that the first field of the generated record matches the first one
>> declared, which makes 'define-configuration' record API compatible with
>> define-record-type* ones.
>>
>> * gnu/services/configuration.scm (define-configuration-helper): Move the
>> %location field below the ones declared by the user.
>> * gnu/services/monitoring.scm (zabbix-front-end-config): Adjust match pattern
>> accordingly.
>
> [...]
>
>> +++ b/gnu/services/configuration.scm
>> @@ -242,17 +242,17 @@ (define-record-type* #,(id #'stem #'< #'stem #'>)
>>                 stem
>>                 #,(id #'stem #'make- #'stem)
>>                 #,(id #'stem #'stem #'?)
>> -               (%location #,(id #'stem #'stem #'-location)
>> -                          (default (and=> (current-source-location)
>> -                                          source-properties->location))
>> -                          (innate))
>>                 #,@(map (lambda (name getter def)
>>                           #`(#,name #,getter (default #,def)
>>                                     (sanitize
>>                                      #,(id #'stem #'validate- #'stem #'- name))))
>>                         #'(field ...)
>>                         #'(field-getter ...)
>> -                       #'(field-default ...)))
>> +                       #'(field-default ...))
>> +               (%location #,(id #'stem #'stem #'-location)
>> +                          (default (and=> (current-source-location)
>> +                                          source-properties->location))
>> +                          (innate)))
>
> Moving the field last is problematic as we’ve seen for any user that
> uses ‘match’ on records—something that’s not recommended but still used
> a lot.

Yep.  I had that on mind when I made the change, though I still missed a
few occurrences.

>>  (define (zabbix-front-end-config config)
>>    (match-record config <zabbix-front-end-configuration>
>> -    (%location db-host db-port db-name db-user db-password db-secret-file
>> -               zabbix-host zabbix-port)
>> +    (db-host db-port db-name db-user db-password db-secret-file
>> +             zabbix-host zabbix-port %location)
>
> This change has no effect because ‘match-record’ matches fields by name,
> precisely to avoid problems when changing the layout of record types.

Good catch; I got confused there, although my confusion luckily had no
side effect :-).

> I’m not sure what was meant by “compatible” in the commit log; how
> fields are laid out is something user code should not be aware of.

I wanted match on define-configuration'd fields to be
backward-compatible with fields migrated from define-record-type*, so
that they such change can be made without worrying breakages.  It seems
good for consistency that both our define-record-type* and
define-configuration fields share a similar internal layout, at least
until all the old-fashion (ice-9 match) record matching usages are
rewritten to use something like 'match-record'.

I initially got tricked by that discrepancy and it took me quite some
time hunting down a cryptic backtrace when authoring the new mcron
configuration records.

> The only thing to keep in mind is that records must not be matched with
> ‘match’ because then the code silently breaks when the record type
> layout is changed.  This is why ‘match-record’ was introduced:
>
>   https://issues.guix.gnu.org/28960#4
>
> One last thing: placing ‘%location’ first can let us implement:
>
>   (define (configuration-location config)
>     (struct-ref config 0))

Would this have worked?

--8<---------------cut here---------------start------------->8---
scheme@(gnu services mcron)> (define config (mcron-configuration))
scheme@(gnu services mcron)> (struct-ref 0 config)
ice-9/boot-9.scm:1685:16: In procedure raise-exception:
In procedure struct-ref: Wrong type argument in position 1 (expecting struct): 0

Entering a new prompt.  Type `,bt' for a backtrace or `,q' to continue.
scheme@(gnu services mcron) [1]> ,q
scheme@(gnu services mcron)> ,use (oop goops)
scheme@(gnu services mcron)> (class-of config)
$5 = #<<class> <<mcron-configuration>> 7fe20fd83e00>
--8<---------------cut here---------------end--------------->8---


All in all, I think that's a rather small change that got our internal
implementation of both type of records in sync between
define-configuration and define-record-type*, that should pave the way
for migrating more of the later to the former without risking breaking
something, going forward.

Alternatively, if we have a good reason to not to go with this, I think
we should abstract all the internal-implementation dependent code from
our code base (e.g., the match patterns directly matching on field
slots).

What do you think?

-- 
Thanks,
Maxim


  reply	other threads:[~2022-11-18 16:45 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-29  3:47 [bug#58855] [PATCH 0/5] Update mcron to latest commit Maxim Cournoyer
2022-10-29  4:16 ` [bug#58855] [PATCH 1/5] services: configuration: Re-order generated record fields Maxim Cournoyer
2022-10-29  4:16   ` [bug#58855] [PATCH 2/5] services: mcron: Add log? and log-format fields to mcron-configuration Maxim Cournoyer
2022-10-29  4:16   ` [bug#58855] [PATCH 3/5] gnu: mcron: Use gexps and strip trailing #t Maxim Cournoyer
2022-10-29  4:16   ` [bug#58855] [PATCH 4/5] gnu: Remove guile2.2-mcron Maxim Cournoyer
2022-10-29  4:16   ` [bug#58855] [PATCH 5/5] gnu: mcron: Update to 1.2.1-0.5fd0ccd Maxim Cournoyer
2022-11-17 22:37   ` Layout of ‘define-configuration’ records Ludovic Courtès
2022-11-18 16:44     ` Maxim Cournoyer [this message]
2022-11-19 21:25       ` Katherine Cox-Buday
2022-11-20 13:47         ` Maxim Cournoyer
2022-11-21 10:26           ` Ludovic Courtès
2022-11-21 20:08             ` Maxim Cournoyer
2022-11-21 16:49           ` Katherine Cox-Buday
2022-11-21 21:00             ` Maxim Cournoyer
2022-11-22 14:52               ` zimoun
2022-11-25 15:18                 ` Maxim Cournoyer
2022-11-21 10:22       ` Ludovic Courtès
2022-11-21 21:16         ` Maxim Cournoyer
2022-11-23 21:56           ` Ludovic Courtès
2022-11-25 15:15             ` Maxim Cournoyer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8735ag43ga.fsf@gmail.com \
    --to=maxim.cournoyer@gmail.com \
    --cc=guix-devel@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/guix.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.