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
next prev parent 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.