bug#59423: Invalid 'location' field generated in dovecot configuration

[Ludovic Courtès]

Hi,

Maxim Cournoyer  skribis:

>> We’ll have to discuss the implications of a possible move to
>> ‘define-configuration’.  For example, ‘define-configuration’ cannot
>> report missing field values (for fields that lack a default value) at
>> macro-expansion time, contrary to plain ‘define-record-type*’.  Anyway,
>> future work!
>
> OK.  That's optimization work rather than an impediment to migrate
> though, right?  If so, I think the value for users of having errors on
> invalid field types outweighs run time efficiency :-).

I guess my point is “we’ll have to discuss”.  It has non-obvious
implications such as this one that have a visible impact on users.

Ludo’.

bug#59423: Invalid 'location' field generated in dovecot configuration

[Maxim Cournoyer]

Hi Ludovic,

Ludovic Courtès  writes:

> Maxim Cournoyer  skribis:
>
>> Thanks for this extra bit of information and for spotting this usage.  I
>> think "location" is likely to conflict for the general purpose
>> 'define-configuration' generated records, so I've renamed the "location"
>> *accessor* to "source-location".
>
> Thank you.
>
> It wasn’t my preferred solution¹ but I think it’s a good one.
>
> ¹ https://issues.guix.gnu.org/59423#15-lineno81
>
>> In the near future I want to migrate more service configurations to the
>> 'define-configuration' machinery, to benefit from its useful
>> self-validating property.  For now I wouldn't feel at ease doing so
>> unless raw record matching (not yet using 'match-record') works the same
>> way, since we still have many occurrences making use of that (often via
>> match-lambda).  For that reason, I prefer to not revert the record
>> layout until we've gotten rid of all the match-lambda matching record
>> fields directly (which will take some time).
>
> Right, especially given that ‘match-record’ was added in 2017.  :-)

Hehe!  I had started adapting all the match-lambda, and got overwhelmed
by the changes needed.  So I think progressively (i.e., small) be easier
to review or revert in case of errors.

> We’ll have to discuss the implications of a possible move to
> ‘define-configuration’.  For example, ‘define-configuration’ cannot
> report missing field values (for fields that lack a default value) at
> macro-expansion time, contrary to plain ‘define-record-type*’.  Anyway,
> future work!

OK.  That's optimization work rather than an impediment to migrate
though, right?  If so, I think the value for users of having errors on
invalid field types outweighs run time efficiency :-).

-- 
Thanks,
Maxim

bug#59423: Invalid 'location' field generated in dovecot configuration

[Ludovic Courtès]

Maxim Cournoyer  skribis:

> Thanks for this extra bit of information and for spotting this usage.  I
> think "location" is likely to conflict for the general purpose
> 'define-configuration' generated records, so I've renamed the "location"
> *accessor* to "source-location".

Thank you.

It wasn’t my preferred solution¹ but I think it’s a good one.

¹ https://issues.guix.gnu.org/59423#15-lineno81

> In the near future I want to migrate more service configurations to the
> 'define-configuration' machinery, to benefit from its useful
> self-validating property.  For now I wouldn't feel at ease doing so
> unless raw record matching (not yet using 'match-record') works the same
> way, since we still have many occurrences making use of that (often via
> match-lambda).  For that reason, I prefer to not revert the record
> layout until we've gotten rid of all the match-lambda matching record
> fields directly (which will take some time).

Right, especially given that ‘match-record’ was added in 2017.  :-)

We’ll have to discuss the implications of a possible move to
‘define-configuration’.  For example, ‘define-configuration’ cannot
report missing field values (for fields that lack a default value) at
macro-expansion time, contrary to plain ‘define-record-type*’.  Anyway,
future work!

Ludo’.

bug#59423: Invalid 'location' field generated in dovecot configuration

[Maxim Cournoyer]

Hi Ludovic,

Ludovic Courtès  writes:

> Ludovic Courtès  skribis:
>
>> Maxim Cournoyer  skribis:
>
> [...]
>
>>> No :-).  I meant why do we even set a default accessor for the *source
>>> location* information (in the (gnu service configuration) macros); it's
>>> that one that doesn't seem to get used (or I'm blind to it!), at least
>>> via this accessor.  If it's not strictly necessary, we can stop
>>> producing it, and that would solve the problem.
>>
>> Like I wrote, I think it’s necessary, even if not used now.
>
> To complement this answer: key high-level record types usually have a
> ‘location’ field: , , , ,
> , etc.  The rationale is that it allows us to report
> accurate location info for errors and warnings, to jump to their
> definition, and so on.
>
> For configuration records this is not a common pattern, but the
> rationale holds.  ‘zabbix-front-end-config’ uses the ‘%location’ field,
> but it seems to be the only one so far.

Thanks for this extra bit of information and for spotting this usage.  I
think "location" is likely to conflict for the general purpose
'define-configuration' generated records, so I've renamed the "location"
*accessor* to "source-location".

In the near future I want to migrate more service configurations to the
'define-configuration' machinery, to benefit from its useful
self-validating property.  For now I wouldn't feel at ease doing so
unless raw record matching (not yet using 'match-record') works the same
way, since we still have many occurrences making use of that (often via
match-lambda).  For that reason, I prefer to not revert the record
layout until we've gotten rid of all the match-lambda matching record
fields directly (which will take some time).

I've applied the rename fix to master.

-- 
Thanks,
Maxim

bug#59423: Invalid 'location' field generated in dovecot configuration

[Maxim Cournoyer]

Hi Pierre,

Pierre Langlois  writes:

> Hi all!
>
> As suggested by mirai off-list, it would be nice to have a test that
> would have caught the issue. How do people feel about something along
> the following patch? The idea is to use namespaces in the dovecot config
> to declare the INBOX and another additional mailbox.
>
> The bug is quite obscure and not really about dovecot itself, so I don't
> think adding such a test is strictly necessary, maybe more tests for the
> configuration macro would be good instead. But I figured expanding the
> dovecot system test can't hurt. Let me know if you agree and I'll format
> it properly.

Thanks for the diff!  I've turned it into a patch and was ready to apply
it, but 2 tests are always failing, even after addressing this issue;
could you look into why?  It looks good otherwise.

>From 149790c4bd264d81938596cdbfa3376f31f9182f Mon Sep 17 00:00:00 2001
From: Pierre Langlois 
Date: Fri, 2 Dec 2022 14:17:25 -0500
Subject: [PATCH] tests: Expound Dovecot test suite.

Relates to .

* gnu/tests/mail.scm (%dovecot-os) [services] : Add two
namespaces.
(run-dovecot-test) : New syntax.
: New test.
: Use with-imap-connection.
: New test.
: Adjust test.
: New test.

Signed-off-by: Maxim Cournoyer 
---
 gnu/tests/mail.scm | 79 +++---
 1 file changed, 60 insertions(+), 19 deletions(-)

diff --git a/gnu/tests/mail.scm b/gnu/tests/mail.scm
index f13751b72f..8a2dbd798f 100644
--- a/gnu/tests/mail.scm
+++ b/gnu/tests/mail.scm
@@ -301,8 +301,19 @@ (define %dovecot-os
  (auth-anonymous-username "alice")
  (mail-location
   (string-append "maildir:~/Maildir"
- ":INBOX=~/Maildir/INBOX"
- ":LAYOUT=fs"))
+ ":LAYOUT=fs"))
+ (namespaces
+  (list
+   (namespace-configuration
+(name "INBOX")
+(inbox? #t)
+(separator "/")
+(location "maildir:~/Maildir/INBOX"))
+   (namespace-configuration
+(name "guix-devel")
+(separator "/")
+(prefix "guix-devel/")
+(location "maildir:~/Maildir/guix-devel"
 
 (define (run-dovecot-test)
   "Return a test of an OS running Dovecot service."
@@ -351,9 +362,10 @@ (define message "From: t...@example.com\n\
   (marionette-eval `(file-exists? (string-append "/proc/" ,pid))
marionette)))
 
-  (test-assert "accept an email"
+  (define-syntax-rule (with-imap-connection imap exp ...)
 (let ((imap (socket AF_INET SOCK_STREAM 0))
-  (addr (make-socket-address AF_INET INADDR_LOOPBACK 8143)))
+  (addr (make-socket-address AF_INET INADDR_LOOPBACK 8143))
+  (body (lambda (imap) exp ...)))
   (connect imap addr)
   ;; Be greeted.
   (read-line imap) ;OK
@@ -362,6 +374,43 @@ (define message "From: t...@example.com\n\
   (read-line imap) ;+
   (write-line "c2lyaGM=" imap)
   (read-line imap) ;OK
+
+  (let ((ok (body imap)))
+;; Logout
+(write-line "a LOGOUT" imap)
+(close imap)
+ok)))
+
+  (define (marionette-read-new-mail mailbox marionette)
+(marionette-eval
+ `(begin
+(use-modules (ice-9 ftw)
+ (ice-9 match))
+(match (scandir ,mailbox)
+  (("." ".." message-file)
+   (call-with-input-file
+   (string-append ,mailbox message-file)
+ get-string-all
+ marionette))
+
+  (test-assert "accept an email"
+(with-imap-connection imap
+  ;; Append a message to the INBOX mailbox
+  (write-line (format #f "a APPEND INBOX {~a}"
+  (number->string (message-length message)))
+  imap)
+  (read-line imap) ;+
+  (write-line message imap)
+  (read-line imap) ;OK
+  #t))
+
+  (test-equal "mail arrived"
+message
+(marionette-read-new-mail "/home/alice/Maildir/INBOX/new/"
+  marionette))
+
+  (test-assert "accept an email in fresh mailbox"
+(with-imap-connection imap
   ;; Create a TESTBOX mailbox
   (write-line "a CREATE TESTBOX" imap)
   (read-line imap) ;OK
@@ -372,24 +421,16 @@ (define message "From: t...@example.com\n\
   (read-line imap) ;+
   

bug#59423: Invalid 'location' field generated in dovecot configuration

[Ludovic Courtès]

Ludovic Courtès  skribis:

> Maxim Cournoyer  skribis:

[...]

>> No :-).  I meant why do we even set a default accessor for the *source
>> location* information (in the (gnu service configuration) macros); it's
>> that one that doesn't seem to get used (or I'm blind to it!), at least
>> via this accessor.  If it's not strictly necessary, we can stop
>> producing it, and that would solve the problem.
>
> Like I wrote, I think it’s necessary, even if not used now.

To complement this answer: key high-level record types usually have a
‘location’ field: , , , ,
, etc.  The rationale is that it allows us to report
accurate location info for errors and warnings, to jump to their
definition, and so on.

For configuration records this is not a common pattern, but the
rationale holds.  ‘zabbix-front-end-config’ uses the ‘%location’ field,
but it seems to be the only one so far.

Ludo’.

bug#59423: Invalid 'location' field generated in dovecot configuration

[Ludovic Courtès]

Hi Maxim,

Maxim Cournoyer  skribis:

> Ludovic Courtès  writes:

[...]

 That generates two accessors called ‘namespace-configuration-location’.
 The second one shadows the first one.
>>>
>>> Yes.  You didn't address my question directly though, so let me ask it
>>> again: where is this %location field access (named "location") used?
>
> [...]
>
>> … the field is accessed via its accessor,
>> ‘name-configuration-location’.  We can kinda see it here:
>
> [...]
>
>> Each  record has a ‘getter’ field that refers to
>> the accessor.  In the case of ‘location’, that’s the “wrong”
>> accessor—the accessor of ‘%location’.
>>
>> I hope that addresses your question!
>
> No :-).  I meant why do we even set a default accessor for the *source
> location* information (in the (gnu service configuration) macros); it's
> that one that doesn't seem to get used (or I'm blind to it!), at least
> via this accessor.  If it's not strictly necessary, we can stop
> producing it, and that would solve the problem.

Like I wrote, I think it’s necessary, even if not used now.

We’ve been knowingly shipping a broken Dovecot for two weeks now.  As I
wrote, and as Pierre suggested, can we just revert the ‘%location’ field
shuffling for now?  I can even do it on your behalf.

After that we can continue that discussion (though I don’t have much to
add to be honest).

Thanks in advance!

Ludo’.

bug#59423: Invalid 'location' field generated in dovecot configuration

[Pierre Langlois]

Hi all!

As suggested by mirai off-list, it would be nice to have a test that
would have caught the issue. How do people feel about something along
the following patch? The idea is to use namespaces in the dovecot config
to declare the INBOX and another additional mailbox.

The bug is quite obscure and not really about dovecot itself, so I don't
think adding such a test is strictly necessary, maybe more tests for the
configuration macro would be good instead. But I figured expanding the
dovecot system test can't hurt. Let me know if you agree and I'll format
it properly.



signature.asc
Description: PGP signature
diff --git a/gnu/tests/mail.scm b/gnu/tests/mail.scm
index f13751b72f..8a2dbd798f 100644
--- a/gnu/tests/mail.scm
+++ b/gnu/tests/mail.scm
@@ -301,8 +301,19 @@ (define %dovecot-os
  (auth-anonymous-username "alice")
  (mail-location
   (string-append "maildir:~/Maildir"
- ":INBOX=~/Maildir/INBOX"
- ":LAYOUT=fs"))
+ ":LAYOUT=fs"))
+ (namespaces
+  (list
+   (namespace-configuration
+(name "INBOX")
+(inbox? #t)
+(separator "/")
+(location "maildir:~/Maildir/INBOX"))
+   (namespace-configuration
+(name "guix-devel")
+(separator "/")
+(prefix "guix-devel/")
+(location "maildir:~/Maildir/guix-devel"
 
 (define (run-dovecot-test)
   "Return a test of an OS running Dovecot service."
@@ -351,9 +362,10 @@ (define message "From: t...@example.com\n\
   (marionette-eval `(file-exists? (string-append "/proc/" ,pid))
marionette)))
 
-  (test-assert "accept an email"
+  (define-syntax-rule (with-imap-connection imap exp ...)
 (let ((imap (socket AF_INET SOCK_STREAM 0))
-  (addr (make-socket-address AF_INET INADDR_LOOPBACK 8143)))
+  (addr (make-socket-address AF_INET INADDR_LOOPBACK 8143))
+  (body (lambda (imap) exp ...)))
   (connect imap addr)
   ;; Be greeted.
   (read-line imap) ;OK
@@ -362,6 +374,43 @@ (define message "From: t...@example.com\n\
   (read-line imap) ;+
   (write-line "c2lyaGM=" imap)
   (read-line imap) ;OK
+
+  (let ((ok (body imap)))
+;; Logout
+(write-line "a LOGOUT" imap)
+(close imap)
+ok)))
+
+  (define (marionette-read-new-mail mailbox marionette)
+(marionette-eval
+ `(begin
+(use-modules (ice-9 ftw)
+ (ice-9 match))
+(match (scandir ,mailbox)
+  (("." ".." message-file)
+   (call-with-input-file
+   (string-append ,mailbox message-file)
+ get-string-all
+ marionette))
+
+  (test-assert "accept an email"
+(with-imap-connection imap
+  ;; Append a message to the INBOX mailbox
+  (write-line (format #f "a APPEND INBOX {~a}"
+  (number->string (message-length message)))
+  imap)
+  (read-line imap) ;+
+  (write-line message imap)
+  (read-line imap) ;OK
+  #t))
+
+  (test-equal "mail arrived"
+message
+(marionette-read-new-mail "/home/alice/Maildir/INBOX/new/"
+  marionette))
+
+  (test-assert "accept an email in fresh mailbox"
+(with-imap-connection imap
   ;; Create a TESTBOX mailbox
   (write-line "a CREATE TESTBOX" imap)
   (read-line imap) ;OK
@@ -372,24 +421,16 @@ (define message "From: t...@example.com\n\
   (read-line imap) ;+
   (write-line message imap)
   (read-line imap) ;OK
-  ;; Logout
-  (write-line "a LOGOUT" imap)
-  (close imap)
   #t))
 
-  (test-equal "mail arrived"
+  (test-equal "mail arrived in fresh mailbox"
 message
-(marionette-eval
- '(begin
-(use-modules (ice-9 ftw)
- (ice-9 match))
-(let ((TESTBOX/new "/home/alice/Maildir/TESTBOX/new/"))
-  (match (scandir TESTBOX/new)
-(("." ".." message-file)
- (call-with-input-file
- (string-append TESTBOX/new message-file)
-   get-string-all)
- marionette))
+   

bug#59423: Invalid 'location' field generated in dovecot configuration

[Pierre Langlois]

Hi Maxim,

Maxim Cournoyer  writes:

> Hi Ludovic,
>
> Ludovic Courtès  writes:
>
>> Hi,
>>
>> Maxim Cournoyer  skribis:
>>
> The issue seems to be with the serialization of the
>  object nested in the 
> record.  I tried this at the REPL:
>
> scheme@(guile-user)> ,m (gnu services mail)
> scheme@(gnu services mail)> (namespace-configuration (name "inbox"))
> $8 = #< name: "inbox" type: "private"
> separator: "" prefix: "" location: "" inbox?: #f hidden?: #f
> list?: #t subscriptions?: #t mailboxes: () %location: #f>
> scheme@(gnu services mail)> (serialize-configuration $8 
> namespace-configuration-fields)
> name=inbox
> type=private
> separator=
> prefix=
> location=#f

 The location here should probably be empty rather than `#f' no? It looks
 as though the value is coming from the internal %location, rather than
 the user-provided location.
>>
>> Uh.
>>
 I'll if I can find anything the macro, it looks quite complex to me :-).
>>>
>>> It's not only to you, if that helps.  It's rather... intimidating ^^'.
>>
>> [...]
>>
>>> Ludovic, would you have an idea of where the %location field or its
>>> CONFIGURATION-location accessor come into play?
>>
>> We have this:
>>
>>  (define-record-type* #,(id #'stem #'< #'stem #'>)
>>stem
>>#,(id #'stem #'make- #'stem)
>>#,(id #'stem #'stem #'?)
>>#,@(map (lambda (name getter def)
>>  #`(#,name #,getter (default #,def)
>>(sanitize
>> #,(id #'stem #'validate- #'stem #'- name
>>#'(field ...)
>>#'(field-getter ...)
>>#'(field-default ...))
>>(%location #,(id #'stem #'stem #'-location)
>>   (default (and=> (current-source-location)
>>   source-properties->location))
>>   (innate)))
>>
>> That generates two accessors called ‘namespace-configuration-location’.
>> The second one shadows the first one.
>
> Yes.  You didn't address my question directly though, so let me ask it
> again: where is this %location field access (named "location") used?  It
> seems nowhere.  Thus, we can simply rename it without impacting
> anything, right (except theoretical usages in the wild, since in the
> API).
>
>> With commit 44554e7133aa60e1b453436be1e80394189cabd9, the second one
>> is the “wrong” one: ‘namespace-configuration-location’ now returns the
>> ‘%location’ field, not the user-specified ‘location’ field.  (I
>> reported that issue in .)
>>
>> What do you think of reverting 44554e7133aa60e1b453436be1e80394189cabd9?
>
> No.  If we revert something, it won't be that whole commit, but just the
> moving of the field in the define-configuration produced record.

If we don't have an obvious solution to the issue and it may need more
time, how do you feel about reverting the changes? Unless there is a
work around that could be applied until a nicer more permanent solution
is found (although those temporary fixes do tend to stick around
sometimes :-) ).

Thanks,
Pierre


signature.asc
Description: PGP signature

bug#59423: Invalid 'location' field generated in dovecot configuration

[Maxim Cournoyer]

Hi Ludovic,

Ludovic Courtès  writes:

> Hi Maxim,
>
> Maxim Cournoyer  skribis:
>
>>> We have this:
>>>
>>>  (define-record-type* #,(id #'stem #'< #'stem #'>)
>>>stem
>>>#,(id #'stem #'make- #'stem)
>>>#,(id #'stem #'stem #'?)
>>>#,@(map (lambda (name getter def)
>>>  #`(#,name #,getter (default #,def)
>>>(sanitize
>>> #,(id #'stem #'validate- #'stem #'- name
>>>#'(field ...)
>>>#'(field-getter ...)
>>>#'(field-default ...))
>>>(%location #,(id #'stem #'stem #'-location)
>>>   (default (and=> (current-source-location)
>>>   source-properties->location))
>>>   (innate)))
>>>
>>> That generates two accessors called ‘namespace-configuration-location’.
>>> The second one shadows the first one.
>>
>> Yes.  You didn't address my question directly though, so let me ask it
>> again: where is this %location field access (named "location") used?

[...]

> … the field is accessed via its accessor,
> ‘name-configuration-location’.  We can kinda see it here:

[...]

> Each  record has a ‘getter’ field that refers to
> the accessor.  In the case of ‘location’, that’s the “wrong”
> accessor—the accessor of ‘%location’.
>
> I hope that addresses your question!

No :-).  I meant why do we even set a default accessor for the *source
location* information (in the (gnu service configuration) macros); it's
that one that doesn't seem to get used (or I'm blind to it!), at least
via this accessor.  If it's not strictly necessary, we can stop
producing it, and that would solve the problem.

Does that make sense?  I'm not talking about namespace-location; that
one has the right to be!  I'm talking about the auto-generated
x-location or y-location, where x and y are configuration records that
were specified via 'define-configuration'.

[...]

> What would need renaming in this case is the accessor, not the field.
> In  you proposed renaming the
> accessor to *-source-location instead of *-location.  That sounds like a
> good idea to me.  We should get unbound-variable warnings in modules
> that use the previous name, so we’ll see if that breaks something.
>
> The only downside is that the convention elsewhere in the code is
> -location, not -source-location.

What about giving it an even more cryptic accessor name like -%location
or dropping it entirely as suggested above?

> So the other option is to rename ‘location’ in
> .  That also has the advantage of being less
> intrusive.

I don't like that alternative, because 'location' seems likely to be
used for future services and reintroduce the same name clash problem.

-- 
Thanks,
Maxim

bug#59423: Invalid 'location' field generated in dovecot configuration

[Ludovic Courtès]

Hi Maxim,

Maxim Cournoyer  skribis:

>> We have this:
>>
>>  (define-record-type* #,(id #'stem #'< #'stem #'>)
>>stem
>>#,(id #'stem #'make- #'stem)
>>#,(id #'stem #'stem #'?)
>>#,@(map (lambda (name getter def)
>>  #`(#,name #,getter (default #,def)
>>(sanitize
>> #,(id #'stem #'validate- #'stem #'- name
>>#'(field ...)
>>#'(field-getter ...)
>>#'(field-default ...))
>>(%location #,(id #'stem #'stem #'-location)
>>   (default (and=> (current-source-location)
>>   source-properties->location))
>>   (innate)))
>>
>> That generates two accessors called ‘namespace-configuration-location’.
>> The second one shadows the first one.
>
> Yes.  You didn't address my question directly though, so let me ask it
> again: where is this %location field access (named "location") used?

When doing something like this:

--8<---cut here---start->8---
scheme@(guix-user)> ,m(gnu services mail)
scheme@(gnu services mail)> (namespace-configuration (name "inbox"))
$1 = #< name: "inbox" type: "private" separator: "" 
prefix: "" location: "" inbox?: #f hidden?: #f list?: #t subscriptions?: #t 
mailboxes: () %location: #f>
scheme@(gnu services mail)> (serialize-configuration $1 
namespace-configuration-fields)
name=inbox
type=private
separator=
prefix=
location=#f
inbox=no
hidden=no
list=yes
subscriptions=yes
$2 = #
--8<---cut here---end--->8---

… the field is accessed via its accessor,
‘name-configuration-location’.  We can kinda see it here:

--8<---cut here---start->8---
scheme@(gnu services mail)> ,pp namespace-configuration-fields
$1 = (#< name: name type: string getter: # predicate: # serializer: # 
default-value-thunk: # documentation: "Name for this namespace.">
 #< name: type type: string getter: # predicate: # serializer: # 
default-value-thunk: # documentation: "Namespace type: @samp{private}, @samp{shared} or 
@samp{public}.">
[…]
 #< name: location type: string getter: # predicate: # serializer: # 
default-value-thunk: # documentation: "Physical location of the mailbox. This is in same format 
as\nmail_location, which is also the default for it.">
[…]
--8<---cut here---end--->8---

Each  record has a ‘getter’ field that refers to
the accessor.  In the case of ‘location’, that’s the “wrong”
accessor—the accessor of ‘%location’.

I hope that addresses your question!

>> What do you think of reverting 44554e7133aa60e1b453436be1e80394189cabd9?
>
> No.  If we revert something, it won't be that whole commit, but just the
> moving of the field in the define-configuration produced record.

Yes, that’s what I meant; sorry for the confusion.

>> After that we can work on renaming the ‘location’ field of
>>  while preserving backward compatibility.
>
> Why do we have to massage the user facing record
> (namespace-configuration) instead of the underlying mechanics?  The
> macro should serve us, not the other way around :-).  See my idea to
> simply rename/remove that automatically produced "location" accessor
> which appears unused to me.  Would that work?

What would need renaming in this case is the accessor, not the field.
In  you proposed renaming the
accessor to *-source-location instead of *-location.  That sounds like a
good idea to me.  We should get unbound-variable warnings in modules
that use the previous name, so we’ll see if that breaks something.

The only downside is that the convention elsewhere in the code is
-location, not -source-location.

So the other option is to rename ‘location’ in
.  That also has the advantage of being less
intrusive.

WDYT?

Ludo’.

bug#59423: Invalid 'location' field generated in dovecot configuration

[Maxim Cournoyer]

Hi Ludovic,

Ludovic Courtès  writes:

> Hi,
>
> Maxim Cournoyer  skribis:
>
 The issue seems to be with the serialization of the
  object nested in the 
 record.  I tried this at the REPL:

 scheme@(guile-user)> ,m (gnu services mail)
 scheme@(gnu services mail)> (namespace-configuration (name "inbox"))
 $8 = #< name: "inbox" type: "private"
 separator: "" prefix: "" location: "" inbox?: #f hidden?: #f
 list?: #t subscriptions?: #t mailboxes: () %location: #f>
 scheme@(gnu services mail)> (serialize-configuration $8 
 namespace-configuration-fields)
 name=inbox
 type=private
 separator=
 prefix=
 location=#f
>>>
>>> The location here should probably be empty rather than `#f' no? It looks
>>> as though the value is coming from the internal %location, rather than
>>> the user-provided location.
>
> Uh.
>
>>> I'll if I can find anything the macro, it looks quite complex to me :-).
>>
>> It's not only to you, if that helps.  It's rather... intimidating ^^'.
>
> [...]
>
>> Ludovic, would you have an idea of where the %location field or its
>> CONFIGURATION-location accessor come into play?
>
> We have this:
>
>  (define-record-type* #,(id #'stem #'< #'stem #'>)
>stem
>#,(id #'stem #'make- #'stem)
>#,(id #'stem #'stem #'?)
>#,@(map (lambda (name getter def)
>  #`(#,name #,getter (default #,def)
>(sanitize
> #,(id #'stem #'validate- #'stem #'- name
>#'(field ...)
>#'(field-getter ...)
>#'(field-default ...))
>(%location #,(id #'stem #'stem #'-location)
>   (default (and=> (current-source-location)
>   source-properties->location))
>   (innate)))
>
> That generates two accessors called ‘namespace-configuration-location’.
> The second one shadows the first one.

Yes.  You didn't address my question directly though, so let me ask it
again: where is this %location field access (named "location") used?  It
seems nowhere.  Thus, we can simply rename it without impacting
anything, right (except theoretical usages in the wild, since in the
API).

> With commit 44554e7133aa60e1b453436be1e80394189cabd9, the second one
> is the “wrong” one: ‘namespace-configuration-location’ now returns the
> ‘%location’ field, not the user-specified ‘location’ field.  (I
> reported that issue in .)
>
> What do you think of reverting 44554e7133aa60e1b453436be1e80394189cabd9?

No.  If we revert something, it won't be that whole commit, but just the
moving of the field in the define-configuration produced record.

> After that we can work on renaming the ‘location’ field of
>  while preserving backward compatibility.

Why do we have to massage the user facing record
(namespace-configuration) instead of the underlying mechanics?  The
macro should serve us, not the other way around :-).  See my idea to
simply rename/remove that automatically produced "location" accessor
which appears unused to me.  Would that work?

-- 
Thanks,
Maxim

bug#59423: Invalid 'location' field generated in dovecot configuration

[Ludovic Courtès]

Hi,

Maxim Cournoyer  skribis:

>>> The issue seems to be with the serialization of the
>>>  object nested in the 
>>> record.  I tried this at the REPL:
>>>
>>> scheme@(guile-user)> ,m (gnu services mail)
>>> scheme@(gnu services mail)> (namespace-configuration (name "inbox"))
>>> $8 = #< name: "inbox" type: "private" separator: 
>>> "" prefix: "" location: "" inbox?: #f hidden?: #f list?: #t subscriptions?: 
>>> #t mailboxes: () %location: #f>
>>> scheme@(gnu services mail)> (serialize-configuration $8 
>>> namespace-configuration-fields)
>>> name=inbox
>>> type=private
>>> separator=
>>> prefix=
>>> location=#f
>>
>> The location here should probably be empty rather than `#f' no? It looks
>> as though the value is coming from the internal %location, rather than
>> the user-provided location.

Uh.

>> I'll if I can find anything the macro, it looks quite complex to me :-).
>
> It's not only to you, if that helps.  It's rather... intimidating ^^'.

[...]

> Ludovic, would you have an idea of where the %location field or its
> CONFIGURATION-location accessor come into play?

We have this:

 (define-record-type* #,(id #'stem #'< #'stem #'>)
   stem
   #,(id #'stem #'make- #'stem)
   #,(id #'stem #'stem #'?)
   #,@(map (lambda (name getter def)
 #`(#,name #,getter (default #,def)
   (sanitize
#,(id #'stem #'validate- #'stem #'- name
   #'(field ...)
   #'(field-getter ...)
   #'(field-default ...))
   (%location #,(id #'stem #'stem #'-location)
  (default (and=> (current-source-location)
  source-properties->location))
  (innate)))

That generates two accessors called ‘namespace-configuration-location’.
The second one shadows the first one.  With commit
44554e7133aa60e1b453436be1e80394189cabd9, the second one is the “wrong”
one: ‘namespace-configuration-location’ now returns the ‘%location’
field, not the user-specified ‘location’ field.  (I reported that issue
in .)

What do you think of reverting 44554e7133aa60e1b453436be1e80394189cabd9?

After that we can work on renaming the ‘location’ field of
 while preserving backward compatibility.

Thanks,
Ludo’.

bug#59423: Invalid 'location' field generated in dovecot configuration

[Maxim Cournoyer]

Hi Pierre,

Pierre Langlois  writes:

> Hi Maxim,
>
> Maxim Cournoyer  writes:

[...]

>> The change was reinstated as part of the mcron update, in
>> 44554e7133aa60e1b453436be1e80394189cabd9.  The bit that seems to cause
>> the issue here (still not clearly understood) is probably this one:
>>
>> diff --git a/gnu/services/configuration.scm b/gnu/services/configuration.scm
>> index 636c49ccba..dacfc52ba9 100644
>> --- a/gnu/services/configuration.scm
>> +++ 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)))
>>
>>   (define #,(id #'stem #'stem #'-fields)
>> (list (configuration-field
>>
>>
>> Reverting it would likely fix the issue (haven't tried), but it'd be
>> nice to have a clear understanding of what's going on.  It may have
>> unmasked a bug waiting to bite.
>>
>> The issue seems to be with the serialization of the
>>  object nested in the 
>> record.  I tried this at the REPL:
>>
>> scheme@(guile-user)> ,m (gnu services mail)
>> scheme@(gnu services mail)> (namespace-configuration (name "inbox"))
>> $8 = #< name: "inbox" type: "private" separator: "" 
>> prefix: "" location: "" inbox?: #f hidden?: #f list?: #t subscriptions?: #t 
>> mailboxes: () %location: #f>
>> scheme@(gnu services mail)> (serialize-configuration $8 
>> namespace-configuration-fields)
>> name=inbox
>> type=private
>> separator=
>> prefix=
>> location=#f
>
> The location here should probably be empty rather than `#f' no? It looks
> as though the value is coming from the internal %location, rather than
> the user-provided location.

Good eye!  Perhaps my earlier simple session was able to reproduce after
all!  #f sure doesn't read as a successfully serialized value :-).  It
probably came from %location, which is set to #f when working at the
REPL.

> If the two fields can shadow each other,
> then indeed, that looks like an existing bug that was exposed by the
> reordering, rather than a bug with the reorder itself.
>
> I'll if I can find anything the macro, it looks quite complex to me :-).

It's not only to you, if that helps.  It's rather... intimidating ^^'.

Looking at it again, the problem is not so mysterious after all... The
%location field has its accessor set to be:

--8<---cut here---start->8---
   (%location #,(id #'stem #'stem #'-location)
  (default (and=> (current-source-location)
  source-properties->location))
  (innate)))
--8<---cut here---end--->8---

#,(id #'stem #'stem #'-location)

which gets expanded to namespace-configuration-location, which shadows
that of the now preceding "location" field.

The bug in the previous condition would have been reversed; the source
location would have been shadowed by the "location" field value.

Ludovic, would you have an idea of where the %location field or its
CONFIGURATION-location accessor come into play?  I tried tracing it in
the source, but I only see it being set and the location being pulled
from the sanitizer via "(datum->syntax #'value (syntax-source #'value)"
in (gnu services configuration) around line 227, which is the location
that would get printed in the error handler CALL-WITH-ERROR-HANDLING
from (guix ui):

--8<---cut here---start->8---
 ((formatted-message? c)
  (apply report-error
 (and (error-location? c) (error-location c))
 (gettext (formatted-message-string c) %gettext-domain)
 (formatted-message-arguments c))
  (when (fix-hint? c)
(display-hint (condition-fix-hint c)))
  (exit 1))
--8<---cut here---end--->8---

I could be wrong, but since the "location" of a field 

bug#59423: Invalid 'location' field generated in dovecot configuration

[Fredrik Salomonsson]

Hello,

I think I encountered the same bug when I updated my home rofi service
to use a configuration. It also has a `location` entry but it takes an
integer.

Here is a repro to reproduce it using `guix repl`:

scheme@(guix-user)> (use-modules (gnu services configuration))
scheme@(guix-user)> (define (serialize-integer f v) (number->string v))
scheme@(guix-user)> (define-configuration repro-configuration (location 
(integer 2) "set location"))
;;; :3:0: warning: shadows previous definition of 
`%repro-configuration-location-procedure' at :3:0
;;; : warning: shadows previous definition of 
`repro-configuration-location' at 
scheme@(guix-user)> (serialize-configuration (repro-configuration) 
repro-configuration-fields)
ice-9/boot-9.scm:1685:16: In procedure raise-exception:
In procedure number->string: Wrong type argument in position 1: #f

Entering a new prompt.  Type `,bt' for a backtrace or `,q' to continue.
scheme@(guix-user) [1]> ,q
scheme@(guix-user)> (repro-configuration)
$1 = #< location: 2 %location: #f>

Note the warning about location being shadowed. And it looks like
%location is trying to use `serialize-integer` but failing as it is #f.

Here is what `guix describe` reports on what guix version I'm running:

Generation 13   nov 26 2022 15:05:06(current)
  guix 68925b5
repository URL: https://git.savannah.gnu.org/git/guix.git
branch: master
commit: 68925b5ee7e0d96b0c84ae98a633eea5097bf511

I hope this helps with debugging this.

-- 
s/Fred[re]+i[ck]+/Fredrik/g

bug#59423: Invalid 'location' field generated in dovecot configuration

[Pierre Langlois]

Hi Maxim,

Maxim Cournoyer  writes:

[...]

 Yeah I'm afraid I still see the same issue after a `git pull` just now:

 ~/code/guix [env]$ ./pre-inst-env guix system build -e '(@@ (gnu tests 
 mail) %dovecot-os)'
 /gnu/store/ayfvf5s561q955kv8wrkklrvq3ga3qpy-system
 ~/code/guix [env]$ guix gc -R
 /gnu/store/ayfvf5s561q955kv8wrkklrvq3ga3qpy-system | grep
 dovecot\.conf | xargs grep "^location"
 location=#< file: "gnu/tests/mail.scm" line: 297 column: 20>

 Have you tried to rebuild from scratch, after a `make clean-go'? When
 first bisecting this, I was working from the git repo and couldn't
 reproduce the bug. Then it worked by using `guix time-machine' to bisect
 rather than work from git.

 So I'm guessing the change being in a macro, there could be residue .go
 files that need recompiling?
>>>
>>> Oh, I just realized the change was reverted with
>>> 44554e7133aa60e1b453436be1e80394189cabd9, then I'm probably the one who
>>> needs to do a `make clean-go' :-).
>
> The change was reinstated as part of the mcron update, in
> 44554e7133aa60e1b453436be1e80394189cabd9.  The bit that seems to cause
> the issue here (still not clearly understood) is probably this one:
>
> diff --git a/gnu/services/configuration.scm b/gnu/services/configuration.scm
> index 636c49ccba..dacfc52ba9 100644
> --- a/gnu/services/configuration.scm
> +++ 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)))
>  
>   (define #,(id #'stem #'stem #'-fields)
> (list (configuration-field
>
>
> Reverting it would likely fix the issue (haven't tried), but it'd be
> nice to have a clear understanding of what's going on.  It may have
> unmasked a bug waiting to bite.
>
> The issue seems to be with the serialization of the
>  object nested in the 
> record.  I tried this at the REPL:
>
> scheme@(guile-user)> ,m (gnu services mail)
> scheme@(gnu services mail)> (namespace-configuration (name "inbox"))
> $8 = #< name: "inbox" type: "private" separator: "" 
> prefix: "" location: "" inbox?: #f hidden?: #f list?: #t subscriptions?: #t 
> mailboxes: () %location: #f>
> scheme@(gnu services mail)> (serialize-configuration $8 
> namespace-configuration-fields)
> name=inbox
> type=private
> separator=
> prefix=
> location=#f

The location here should probably be empty rather than `#f' no? It looks
as though the value is coming from the internal %location, rather than
the user-provided location. If the two fields can shadow each other,
then indeed, that looks like an existing bug that was exposed by the
reordering, rather than a bug with the reorder itself.

I'll if I can find anything the macro, it looks quite complex to me :-).

> inbox=no
> hidden=no
> list=yes
> subscriptions=yes
> $9 = #
>
> But as you can see, it doesn't reproduce in this environment.  I'll keep 
> experimenting.

Thanks for looking into this!


signature.asc
Description: PGP signature

bug#59423: Invalid 'location' field generated in dovecot configuration

[Maxim Cournoyer]

Hi Pierre,

Pierre Langlois  writes:

[...]

 I did:

 $ ./pre-inst-env guix system build -e '(@@ (gnu tests mail) %dovecot-os)'
 /gnu/store/gpl6g2ia84kc41zma7ik9y4p3kik5aiy-system

 Then:

 $ guix gc -R /gnu/store/gpl6g2ia84kc41zma7ik9y4p3kik5aiy-system | grep 
 dovecot.conf
 /gnu/store/1ijjsm3sj8v0qj88fhlwqxgdszd6q6h7-dovecot.conf

OK, after 'make clean-go' and rebuilding everything, I now see the issue
with the above:

--8<---cut here---start->8---
location=#< file: "gnu/tests/mail.scm" line: 297 column: 20>
--8<---cut here---end--->8---

So, that's that.

>>> Yeah I'm afraid I still see the same issue after a `git pull` just now:
>>>
>>> ~/code/guix [env]$ ./pre-inst-env guix system build -e '(@@ (gnu tests 
>>> mail) %dovecot-os)'
>>> /gnu/store/ayfvf5s561q955kv8wrkklrvq3ga3qpy-system
>>> ~/code/guix [env]$ guix gc -R
>>> /gnu/store/ayfvf5s561q955kv8wrkklrvq3ga3qpy-system | grep
>>> dovecot\.conf | xargs grep "^location"
>>> location=#< file: "gnu/tests/mail.scm" line: 297 column: 20>
>>>
>>> Have you tried to rebuild from scratch, after a `make clean-go'? When
>>> first bisecting this, I was working from the git repo and couldn't
>>> reproduce the bug. Then it worked by using `guix time-machine' to bisect
>>> rather than work from git.
>>>
>>> So I'm guessing the change being in a macro, there could be residue .go
>>> files that need recompiling?
>>
>> Oh, I just realized the change was reverted with
>> 44554e7133aa60e1b453436be1e80394189cabd9, then I'm probably the one who
>> needs to do a `make clean-go' :-).

The change was reinstated as part of the mcron update, in
44554e7133aa60e1b453436be1e80394189cabd9.  The bit that seems to cause
the issue here (still not clearly understood) is probably this one:

--8<---cut here---start->8---
diff --git a/gnu/services/configuration.scm b/gnu/services/configuration.scm
index 636c49ccba..dacfc52ba9 100644
--- a/gnu/services/configuration.scm
+++ 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)))
 
  (define #,(id #'stem #'stem #'-fields)
(list (configuration-field
--8<---cut here---end--->8---

Reverting it would likely fix the issue (haven't tried), but it'd be
nice to have a clear understanding of what's going on.  It may have
unmasked a bug waiting to bite.

The issue seems to be with the serialization of the
 object nested in the 
record.  I tried this at the REPL:

--8<---cut here---start->8---
scheme@(guile-user)> ,m (gnu services mail)
scheme@(gnu services mail)> (namespace-configuration (name "inbox"))
$8 = #< name: "inbox" type: "private" separator: "" 
prefix: "" location: "" inbox?: #f hidden?: #f list?: #t subscriptions?: #t 
mailboxes: () %location: #f>
scheme@(gnu services mail)> (serialize-configuration $8 
namespace-configuration-fields)
name=inbox
type=private
separator=
prefix=
location=#f
inbox=no
hidden=no
list=yes
subscriptions=yes
$9 = #
--8<---cut here---end--->8---

But as you can see, it doesn't reproduce in this environment.  I'll keep 
experimenting.

-- 
Thanks,
Maxim

bug#59423: Invalid 'location' field generated in dovecot configuration

[Pierre Langlois]

Pierre Langlois  writes:

> [[PGP Signed Part:Undecided]]
>
> Pierre Langlois  writes:
>
>> [[PGP Signed Part:Undecided]]
>>
>> Maxim Cournoyer  writes:
>>
>>> Hi Pierre,
>>>
>>> Pierre Langlois  writes:
>>>
 Hi Guix!

 After updating the system, the dovecot service got confused and started
 moving around all mailboxes.  I looked up the configuration and noticed
 strange invalid syntax for the location field:

 location=#< file: "path/to/config.scm" line: 297 column: 20>


 Because the # character is interpreted as a comment, dovecot doesn't
 crash and instead moves mailboxes around in weird ways I don't quite
 understand :-/.

 This can actually be reproduced locally with the dovecot system test if
 one dumps the following expression to check the configuration:

 (format #t "~a\n" (wait-for-file "/etc/dovecot/dovecot.conf"
  marionette
  #:read 'get-string-all))


 Giving us the snippets like this in the config:

 $ make check-system TESTS="dovecot"  VERBOSE=1
 ...
 namespace inbox {
 type=private
 separator=
 prefix=
 location=#< file: "gnu/tests/mail.scm" line: 297 column: 20>
 inbox=yes
 hidden=no
 list=yes
 subscriptions=yes
 mailbox "Drafts" {
 auto=no
 special_use=\Drafts
 }
 mailbox "Junk" {
 auto=no
 special_use=\Junk
 }
 mailbox "Trash" {
 auto=no
 special_use=\Trash
 }
 mailbox "Sent" {
 auto=no
 special_use=\Sent
 }
 mailbox "Sent Messages" {
 auto=no
 special_use=\Sent
 }
 mailbox "Drafts" {
 auto=no
 special_use=\Drafts
 }
 }
>>>
>>> I did:
>>>
>>> $ ./pre-inst-env guix system build -e '(@@ (gnu tests mail) %dovecot-os)'
>>> /gnu/store/gpl6g2ia84kc41zma7ik9y4p3kik5aiy-system
>>>
>>> Then:
>>>
>>> $ guix gc -R /gnu/store/gpl6g2ia84kc41zma7ik9y4p3kik5aiy-system | grep 
>>> dovecot.conf
>>> /gnu/store/1ijjsm3sj8v0qj88fhlwqxgdszd6q6h7-dovecot.conf
>>
>> 
>>
>> Oh that's a nice way of doing this, better than my hack to print the
>> config, I'll have to remember the `guix gc -R' flag.
>>
>>>
>>> And what I see in this file is now:
>>>
>>> namespace inbox {
>>> type=private
>>> separator=
>>> prefix=
>>> location=
>>> inbox=yes
>>> hidden=no
>>> list=yes
>>> subscriptions=yes
>>> mailbox "Drafts" {
>>> auto=no
>>> special_use=\Drafts
>>> }
>>> mailbox "Junk" {
>>> auto=no
>>> special_use=\Junk
>>> }
>>> mailbox "Trash" {
>>> auto=no
>>> special_use=\Trash
>>> }
>>> mailbox "Sent" {
>>> auto=no
>>> special_use=\Sent
>>> }
>>> mailbox "Sent Messages" {
>>> auto=no
>>> special_use=\Sent
>>> }
>>> mailbox "Drafts" {
>>> auto=no
>>> special_use=\Drafts
>>> }
>>> }
>>>
>>> Notice that location is empty.  So that's at least different to your
>>> findings, on latest commit.  Can you still reproduce?
>>
>> Yeah I'm afraid I still see the same issue after a `git pull` just now:
>>
>> ~/code/guix [env]$ ./pre-inst-env guix system build -e '(@@ (gnu tests mail) 
>> %dovecot-os)'
>> /gnu/store/ayfvf5s561q955kv8wrkklrvq3ga3qpy-system
>> ~/code/guix [env]$ guix gc -R 
>> /gnu/store/ayfvf5s561q955kv8wrkklrvq3ga3qpy-system | grep dovecot\.conf | 
>> xargs grep "^location"
>> location=#< file: "gnu/tests/mail.scm" line: 297 column: 20>
>>
>> Have you tried to rebuild from scratch, after a `make clean-go'? When
>> first bisecting this, I was working from the git repo and couldn't
>> reproduce the bug. Then it worked by using `guix time-machine' to bisect
>> rather than work from git.
>>
>> So I'm guessing the change being in a macro, there could be residue .go
>> files that need recompiling?
>
> Oh, I just realized the change was reverted with
> 44554e7133aa60e1b453436be1e80394189cabd9, then I'm probably the one who
> needs to do a `make clean-go' :-).

I'm afraid I can still reproduce the issue after a fresh rebuild, I can
also reproduce it outside of the git checkout:

--8<---cut here---start->8---
$ guix describe
Generation 1026 Nov 25 2022 20:11:23(current)
  guix 7e0ad0d
repository URL: https://git.savannah.gnu.org/git/guix.git
branch: master
commit: 7e0ad0dd0f2829d6f3776648ba7c88acf9888d7a
(... snip ...)
$ guix gc -R `guix system build -e '(@@ (gnu tests mail) %dovecot-os)'` | grep 
dovecot\.conf | xargs head -n 20
listen=*,::
dict {
}
passdb {
driver=pam
}
userdb {
driver=passwd
override_fields=
}
plugin {
}
namespace inbox {
type=private
separator=
prefix=
location=#< file: "gnu/tests/mail.scm" line: 297 column: 20>
inbox=yes
hidden=no
list=yes
--8<---cut here---end--->8---

Now I'm confused :-/.


signature.asc
Description: PGP signature

bug#59423: Invalid 'location' field generated in dovecot configuration

[Pierre Langlois]

Pierre Langlois  writes:

> [[PGP Signed Part:Undecided]]
>
> Maxim Cournoyer  writes:
>
>> Hi Pierre,
>>
>> Pierre Langlois  writes:
>>
>>> Hi Guix!
>>>
>>> After updating the system, the dovecot service got confused and started
>>> moving around all mailboxes.  I looked up the configuration and noticed
>>> strange invalid syntax for the location field:
>>>
>>> location=#< file: "path/to/config.scm" line: 297 column: 20>
>>>
>>>
>>> Because the # character is interpreted as a comment, dovecot doesn't
>>> crash and instead moves mailboxes around in weird ways I don't quite
>>> understand :-/.
>>>
>>> This can actually be reproduced locally with the dovecot system test if
>>> one dumps the following expression to check the configuration:
>>>
>>> (format #t "~a\n" (wait-for-file "/etc/dovecot/dovecot.conf"
>>>  marionette
>>>  #:read 'get-string-all))
>>>
>>>
>>> Giving us the snippets like this in the config:
>>>
>>> $ make check-system TESTS="dovecot"  VERBOSE=1
>>> ...
>>> namespace inbox {
>>> type=private
>>> separator=
>>> prefix=
>>> location=#< file: "gnu/tests/mail.scm" line: 297 column: 20>
>>> inbox=yes
>>> hidden=no
>>> list=yes
>>> subscriptions=yes
>>> mailbox "Drafts" {
>>> auto=no
>>> special_use=\Drafts
>>> }
>>> mailbox "Junk" {
>>> auto=no
>>> special_use=\Junk
>>> }
>>> mailbox "Trash" {
>>> auto=no
>>> special_use=\Trash
>>> }
>>> mailbox "Sent" {
>>> auto=no
>>> special_use=\Sent
>>> }
>>> mailbox "Sent Messages" {
>>> auto=no
>>> special_use=\Sent
>>> }
>>> mailbox "Drafts" {
>>> auto=no
>>> special_use=\Drafts
>>> }
>>> }
>>
>> I did:
>>
>> $ ./pre-inst-env guix system build -e '(@@ (gnu tests mail) %dovecot-os)'
>> /gnu/store/gpl6g2ia84kc41zma7ik9y4p3kik5aiy-system
>>
>> Then:
>>
>> $ guix gc -R /gnu/store/gpl6g2ia84kc41zma7ik9y4p3kik5aiy-system | grep 
>> dovecot.conf
>> /gnu/store/1ijjsm3sj8v0qj88fhlwqxgdszd6q6h7-dovecot.conf
>
> 
>
> Oh that's a nice way of doing this, better than my hack to print the
> config, I'll have to remember the `guix gc -R' flag.
>
>>
>> And what I see in this file is now:
>>
>> namespace inbox {
>> type=private
>> separator=
>> prefix=
>> location=
>> inbox=yes
>> hidden=no
>> list=yes
>> subscriptions=yes
>> mailbox "Drafts" {
>> auto=no
>> special_use=\Drafts
>> }
>> mailbox "Junk" {
>> auto=no
>> special_use=\Junk
>> }
>> mailbox "Trash" {
>> auto=no
>> special_use=\Trash
>> }
>> mailbox "Sent" {
>> auto=no
>> special_use=\Sent
>> }
>> mailbox "Sent Messages" {
>> auto=no
>> special_use=\Sent
>> }
>> mailbox "Drafts" {
>> auto=no
>> special_use=\Drafts
>> }
>> }
>>
>> Notice that location is empty.  So that's at least different to your
>> findings, on latest commit.  Can you still reproduce?
>
> Yeah I'm afraid I still see the same issue after a `git pull` just now:
>
> ~/code/guix [env]$ ./pre-inst-env guix system build -e '(@@ (gnu tests mail) 
> %dovecot-os)'
> /gnu/store/ayfvf5s561q955kv8wrkklrvq3ga3qpy-system
> ~/code/guix [env]$ guix gc -R 
> /gnu/store/ayfvf5s561q955kv8wrkklrvq3ga3qpy-system | grep dovecot\.conf | 
> xargs grep "^location"
> location=#< file: "gnu/tests/mail.scm" line: 297 column: 20>
>
> Have you tried to rebuild from scratch, after a `make clean-go'? When
> first bisecting this, I was working from the git repo and couldn't
> reproduce the bug. Then it worked by using `guix time-machine' to bisect
> rather than work from git.
>
> So I'm guessing the change being in a macro, there could be residue .go
> files that need recompiling?

Oh, I just realized the change was reverted with
44554e7133aa60e1b453436be1e80394189cabd9, then I'm probably the one who
needs to do a `make clean-go' :-).


signature.asc
Description: PGP signature

bug#59423: Invalid 'location' field generated in dovecot configuration

[Pierre Langlois]

Maxim Cournoyer  writes:

> Hi Pierre,
>
> Pierre Langlois  writes:
>
>> Hi Guix!
>>
>> After updating the system, the dovecot service got confused and started
>> moving around all mailboxes.  I looked up the configuration and noticed
>> strange invalid syntax for the location field:
>>
>> location=#< file: "path/to/config.scm" line: 297 column: 20>
>>
>>
>> Because the # character is interpreted as a comment, dovecot doesn't
>> crash and instead moves mailboxes around in weird ways I don't quite
>> understand :-/.
>>
>> This can actually be reproduced locally with the dovecot system test if
>> one dumps the following expression to check the configuration:
>>
>> (format #t "~a\n" (wait-for-file "/etc/dovecot/dovecot.conf"
>>  marionette
>>  #:read 'get-string-all))
>>
>>
>> Giving us the snippets like this in the config:
>>
>> $ make check-system TESTS="dovecot"  VERBOSE=1
>> ...
>> namespace inbox {
>> type=private
>> separator=
>> prefix=
>> location=#< file: "gnu/tests/mail.scm" line: 297 column: 20>
>> inbox=yes
>> hidden=no
>> list=yes
>> subscriptions=yes
>> mailbox "Drafts" {
>> auto=no
>> special_use=\Drafts
>> }
>> mailbox "Junk" {
>> auto=no
>> special_use=\Junk
>> }
>> mailbox "Trash" {
>> auto=no
>> special_use=\Trash
>> }
>> mailbox "Sent" {
>> auto=no
>> special_use=\Sent
>> }
>> mailbox "Sent Messages" {
>> auto=no
>> special_use=\Sent
>> }
>> mailbox "Drafts" {
>> auto=no
>> special_use=\Drafts
>> }
>> }
>
> I did:
>
> $ ./pre-inst-env guix system build -e '(@@ (gnu tests mail) %dovecot-os)'
> /gnu/store/gpl6g2ia84kc41zma7ik9y4p3kik5aiy-system
>
> Then:
>
> $ guix gc -R /gnu/store/gpl6g2ia84kc41zma7ik9y4p3kik5aiy-system | grep 
> dovecot.conf
> /gnu/store/1ijjsm3sj8v0qj88fhlwqxgdszd6q6h7-dovecot.conf



Oh that's a nice way of doing this, better than my hack to print the
config, I'll have to remember the `guix gc -R' flag.

>
> And what I see in this file is now:
>
> namespace inbox {
> type=private
> separator=
> prefix=
> location=
> inbox=yes
> hidden=no
> list=yes
> subscriptions=yes
> mailbox "Drafts" {
> auto=no
> special_use=\Drafts
> }
> mailbox "Junk" {
> auto=no
> special_use=\Junk
> }
> mailbox "Trash" {
> auto=no
> special_use=\Trash
> }
> mailbox "Sent" {
> auto=no
> special_use=\Sent
> }
> mailbox "Sent Messages" {
> auto=no
> special_use=\Sent
> }
> mailbox "Drafts" {
> auto=no
> special_use=\Drafts
> }
> }
>
> Notice that location is empty.  So that's at least different to your
> findings, on latest commit.  Can you still reproduce?

Yeah I'm afraid I still see the same issue after a `git pull` just now:

--8<---cut here---start->8---
~/code/guix [env]$ ./pre-inst-env guix system build -e '(@@ (gnu tests mail) 
%dovecot-os)'
/gnu/store/ayfvf5s561q955kv8wrkklrvq3ga3qpy-system
~/code/guix [env]$ guix gc -R 
/gnu/store/ayfvf5s561q955kv8wrkklrvq3ga3qpy-system | grep dovecot\.conf | xargs 
grep "^location"
location=#< file: "gnu/tests/mail.scm" line: 297 column: 20>
--8<---cut here---end--->8---

Have you tried to rebuild from scratch, after a `make clean-go'? When
first bisecting this, I was working from the git repo and couldn't
reproduce the bug. Then it worked by using `guix time-machine' to bisect
rather than work from git.

So I'm guessing the change being in a macro, there could be residue .go
files that need recompiling?

Thanks,
Pierre


signature.asc
Description: PGP signature

bug#59423: Invalid 'location' field generated in dovecot configuration

[Pierre Langlois]

Hi,

Maxim Cournoyer  writes:

> Hi,
>
> Ludovic Courtès  writes:
>
>> Hi,
>>
>> Pierre Langlois  skribis:
>>
>>> After updating the system, the dovecot service got confused and started
>>> moving around all mailboxes.  I looked up the configuration and noticed
>>> strange invalid syntax for the location field:
>>>
>>> location=#< file: "path/to/config.scm" line: 297 column: 20>
>>>
>>>
>>> Because the # character is interpreted as a comment, dovecot doesn't
>>> crash and instead moves mailboxes around in weird ways I don't quite
>>> understand :-/.
>>
>> Ouch, sorry about that.
>
> That's a bad situation indeed, apologies for the breakage!

No worries! Bugs happen :-). It was confusing but I didn't lose any
data, dovecot moved folders to archives so all I had to do was rolling
guix back and move folders again.

>>> I did a `git bisect` with `guix time-machine` (this tool is invaluable)
>>> and found the issue started with this commit:
>>>
>>> commit 543d971ed2a1d9eb934af1f51930741d7cc4e7ef
>>> Author: Maxim Cournoyer 
>>> Date:   Fri Oct 28 17:06:16 2022 -0400
>>> services: configuration: Re-order generated record fields.
>>
>> I believe this is now fixed.
>>
>> Maxim, can you confirm?
>
> Pierre, has it resolved on your side?  I don't use dovecot myself, and
> since it doesn't crash, I don't think the dovecot will be an indicator
> of resolution.

I'm afraid I'm still experiencing the issue, I'll follow-up on the other
thread.

Thanks,
Pierre


signature.asc
Description: PGP signature

bug#59423: Invalid 'location' field generated in dovecot configuration

[Maxim Cournoyer]

Hi Pierre,

Pierre Langlois  writes:

> Hi Guix!
>
> After updating the system, the dovecot service got confused and started
> moving around all mailboxes.  I looked up the configuration and noticed
> strange invalid syntax for the location field:
>
> location=#< file: "path/to/config.scm" line: 297 column: 20>
>
>
> Because the # character is interpreted as a comment, dovecot doesn't
> crash and instead moves mailboxes around in weird ways I don't quite
> understand :-/.
>
> This can actually be reproduced locally with the dovecot system test if
> one dumps the following expression to check the configuration:
>
> (format #t "~a\n" (wait-for-file "/etc/dovecot/dovecot.conf"
>  marionette
>  #:read 'get-string-all))
>
>
> Giving us the snippets like this in the config:
>
> $ make check-system TESTS="dovecot"  VERBOSE=1
> ...
> namespace inbox {
> type=private
> separator=
> prefix=
> location=#< file: "gnu/tests/mail.scm" line: 297 column: 20>
> inbox=yes
> hidden=no
> list=yes
> subscriptions=yes
> mailbox "Drafts" {
> auto=no
> special_use=\Drafts
> }
> mailbox "Junk" {
> auto=no
> special_use=\Junk
> }
> mailbox "Trash" {
> auto=no
> special_use=\Trash
> }
> mailbox "Sent" {
> auto=no
> special_use=\Sent
> }
> mailbox "Sent Messages" {
> auto=no
> special_use=\Sent
> }
> mailbox "Drafts" {
> auto=no
> special_use=\Drafts
> }
> }

I did:

$ ./pre-inst-env guix system build -e '(@@ (gnu tests mail) %dovecot-os)'
/gnu/store/gpl6g2ia84kc41zma7ik9y4p3kik5aiy-system

Then:

$ guix gc -R /gnu/store/gpl6g2ia84kc41zma7ik9y4p3kik5aiy-system | grep 
dovecot.conf
/gnu/store/1ijjsm3sj8v0qj88fhlwqxgdszd6q6h7-dovecot.conf

And what I see in this file is now:

--8<---cut here---start->8---
namespace inbox {
type=private
separator=
prefix=
location=
inbox=yes
hidden=no
list=yes
subscriptions=yes
mailbox "Drafts" {
auto=no
special_use=\Drafts
}
mailbox "Junk" {
auto=no
special_use=\Junk
}
mailbox "Trash" {
auto=no
special_use=\Trash
}
mailbox "Sent" {
auto=no
special_use=\Sent
}
mailbox "Sent Messages" {
auto=no
special_use=\Sent
}
mailbox "Drafts" {
auto=no
special_use=\Drafts
}
}
--8<---cut here---end--->8---

Notice that location is empty.  So that's at least different to your
findings, on latest commit.  Can you still reproduce?

Thanks,

Maxim

bug#59423: Invalid 'location' field generated in dovecot configuration

[mirai]

I'm also experiencing the same issue (guix describe: 
7e0ad0dd0f2829d6f3776648ba7c88acf9888d7a).

My guess is that 44554e7133aa60e1b453436be1e80394189cabd9 (which supersedes 
543d971ed2a1d9eb934af1f51930741d7cc4e7ef)
introduces a '%location' field which conflicts with 'dovecot-configuration' 
itself also having a field called 'location'.

In fact, interesting things happen if you define a configuration with a 
'location' field.
With 'guix repl':
```
$ guix repl
GNU Guile 3.0.8
Copyright (C) 1995-2021 Free Software Foundation, Inc.

Guile comes with ABSOLUTELY NO WARRANTY; for details type `,show w'.
This program is free software, and you are welcome to redistribute it
under certain conditions; type `,show c' for details.

Enter `,help' for help.
scheme@(guix-user)> (use-modules (gnu services configuration))
(define-configuration FOO-configuration
  (name
   (string "aaa")
   "")
  (location
   (string "bbb")
   ""))
;;; :2:0: warning: shadows previous definition of 
`%FOO-configuration-location-procedure' at :2:0
;;; : warning: shadows previous definition of 
`FOO-configuration-location' at 
;;; :2:0: warning: possibly unbound variable `serialize-string'
ice-9/boot-9.scm:1685:16: In procedure raise-exception:
error: serialize-string: unbound variable

Entering a new prompt.  Type `,bt' for a backtrace or `,q' to continue.
scheme@(guix-user) [1]>
```

Code snippet for convenience:
```
(use-modules (gnu services configuration))
(define-configuration FOO-configuration
  (name
   (string "aaa")
   "")
  (location
   (string "bbb")
   ""))
```

bug#59423: Invalid 'location' field generated in dovecot configuration

[Maxim Cournoyer]

Hi,

Ludovic Courtès  writes:

> Hi,
>
> Pierre Langlois  skribis:
>
>> After updating the system, the dovecot service got confused and started
>> moving around all mailboxes.  I looked up the configuration and noticed
>> strange invalid syntax for the location field:
>>
>> location=#< file: "path/to/config.scm" line: 297 column: 20>
>>
>>
>> Because the # character is interpreted as a comment, dovecot doesn't
>> crash and instead moves mailboxes around in weird ways I don't quite
>> understand :-/.
>
> Ouch, sorry about that.

That's a bad situation indeed, apologies for the breakage!

>> I did a `git bisect` with `guix time-machine` (this tool is invaluable)
>> and found the issue started with this commit:
>>
>> commit 543d971ed2a1d9eb934af1f51930741d7cc4e7ef
>> Author: Maxim Cournoyer 
>> Date:   Fri Oct 28 17:06:16 2022 -0400
>> services: configuration: Re-order generated record fields.
>
> I believe this is now fixed.
>
> Maxim, can you confirm?

Pierre, has it resolved on your side?  I don't use dovecot myself, and
since it doesn't crash, I don't think the dovecot will be an indicator
of resolution.

At least, the %location field value look normal when excercised at the
REPL (#f).

-- 
Thanks,
Maxim

bug#59423: Invalid 'location' field generated in dovecot configuration

[Ludovic Courtès]

Hi,

Pierre Langlois  skribis:

> After updating the system, the dovecot service got confused and started
> moving around all mailboxes.  I looked up the configuration and noticed
> strange invalid syntax for the location field:
>
> location=#< file: "path/to/config.scm" line: 297 column: 20>
>
>
> Because the # character is interpreted as a comment, dovecot doesn't
> crash and instead moves mailboxes around in weird ways I don't quite
> understand :-/.

Ouch, sorry about that.

> I did a `git bisect` with `guix time-machine` (this tool is invaluable)
> and found the issue started with this commit:
>
> commit 543d971ed2a1d9eb934af1f51930741d7cc4e7ef
> Author: Maxim Cournoyer 
> Date:   Fri Oct 28 17:06:16 2022 -0400
> services: configuration: Re-order generated record fields.

I believe this is now fixed.

Maxim, can you confirm?

Thanks,
Ludo’.

bug#59423: Invalid 'location' field generated in dovecot configuration

[Pierre Langlois]

Hi Guix!

After updating the system, the dovecot service got confused and started
moving around all mailboxes.  I looked up the configuration and noticed
strange invalid syntax for the location field:

--8<---cut here---start->8---
location=#< file: "path/to/config.scm" line: 297 column: 20>
--8<---cut here---end--->8---

Because the # character is interpreted as a comment, dovecot doesn't
crash and instead moves mailboxes around in weird ways I don't quite
understand :-/.

This can actually be reproduced locally with the dovecot system test if
one dumps the following expression to check the configuration:

--8<---cut here---start->8---
(format #t "~a\n" (wait-for-file "/etc/dovecot/dovecot.conf"
 marionette
 #:read 'get-string-all))
--8<---cut here---end--->8---

Giving us the snippets like this in the config:

--8<---cut here---start->8---
$ make check-system TESTS="dovecot"  VERBOSE=1
...
namespace inbox {
type=private
separator=
prefix=
location=#< file: "gnu/tests/mail.scm" line: 297 column: 20>
inbox=yes
hidden=no
list=yes
subscriptions=yes
mailbox "Drafts" {
auto=no
special_use=\Drafts
}
mailbox "Junk" {
auto=no
special_use=\Junk
}
mailbox "Trash" {
auto=no
special_use=\Trash
}
mailbox "Sent" {
auto=no
special_use=\Sent
}
mailbox "Sent Messages" {
auto=no
special_use=\Sent
}
mailbox "Drafts" {
auto=no
special_use=\Drafts
}
}
...
--8<---cut here---end--->8---

I did a `git bisect` with `guix time-machine` (this tool is invaluable)
and found the issue started with this commit:

--8<---cut here---start->8---
commit 543d971ed2a1d9eb934af1f51930741d7cc4e7ef
Author: Maxim Cournoyer 
Date:   Fri Oct 28 17:06:16 2022 -0400
services: configuration: Re-order generated record fields.

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.
--8<---cut here---end--->8---

Sooo, I'm guessing this is something to do with the configuration field
being named "location", and /maybe/ we're patching it with the origin
location of the configuration, something like that? I don't understand
how this works well enough to be able to thing of any fixes.

Thanks,
Pierre




signature.asc
Description: PGP signature