GNU bug report logs - #60014
[PATCH v2] doc: Clarify special-files-service-type expected value.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 60014 in the body.
You can then email your comments to 60014 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: mirai <at> makinata.eu
To: guix-patches <at> gnu.org
Cc: Bruno Victal <mirai <at> makinata.eu>
Subject: [PATCH] activation: make install-special-file match against pairs as
 well.
Date: Mon, 12 Dec 2022 17:45:47 +0000

From: Bruno Victal <mirai <at> makinata.eu>

special-files is a list of 2-tuples (pairs) but matching against
a non-list pair would fail as match-lambda was only matching
against a list pattern.
---
 gnu/build/activation.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gnu/build/activation.scm b/gnu/build/activation.scm
index 10c9045740..d4a7559651 100644
--- a/gnu/build/activation.scm
+++ b/gnu/build/activation.scm
@@ -341,7 +341,7 @@ (define (activate-special-files special-files)
 "
   (define install-special-file
     (match-lambda
-      ((target file)
+      ((or (target file) (? pair? (= car target) (= cdr file)))
        (let ((pivot (string-append target ".new")))
          (mkdir-p (dirname target))
          (symlink file pivot)

base-commit: 5fb5af5658b7575a945579a7cf51c193600b76bb
-- 
2.38.1

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

From: Josselin Poiret <dev <at> jpoiret.xyz>
To: mirai <at> makinata.eu, 60014 <at> debbugs.gnu.org
Cc: Bruno Victal <mirai <at> makinata.eu>
Subject: Re: [bug#60014] [PATCH] activation: make install-special-file match
 against pairs as well.
Date: Mon, 12 Dec 2022 21:34:02 +0100

Hi Bruno, 

Is this patch related to some specific problem you're running into?  I
personally would prefer keeping the special file interface as-is, and
not mix two different kinds of entries: lists with 2 elements, and
pairs.  That would avoid having to manage even more edge-cases down the
line if some more processing is needed.

Otherwise, you're missing the ChangeLog entry format for the commit
message, which you can find described at [1].  You can take some
inspiration from other commits in the repository.

Best,
-- 
Josselin Poiret

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

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: Josselin Poiret <dev <at> jpoiret.xyz>
Cc: 60014 <at> debbugs.gnu.org, Bruno Victal <mirai <at> makinata.eu>,
 guix-patches <at> gnu.org
Subject: Re: [bug#60014] [PATCH] activation: make install-special-file match
 against pairs as well.
Date: Mon, 12 Dec 2022 21:52:46 +0100

[Message part 1 (text/plain, inline)]

Josselin Poiret via Guix-patches via 写道：
> I personally would prefer keeping the special file interface 
> as-is, and
> not mix two different kinds of entries: lists with 2 elements, 
> and
> pairs.  That would avoid having to manage even more edge-cases 
> down the
> line if some more processing is needed.

I agree with this reasoning, and would go as far as to say that if 
this fixes anything, that thing should probably be fixed instead…?

‘Takes a list of As, but as a special case, a single A’ is 
confusing and makes it that much harder for newcomers to move 
beyond cargo-culting magical snippets.

Kind regards,

T G-R

[signature.asc (application/pgp-signature, inline)]

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

From: mirai <mirai <at> makinata.eu>
To: Josselin Poiret <dev <at> jpoiret.xyz>, 60014 <at> debbugs.gnu.org
Subject: Re: [bug#60014] [PATCH] activation: make install-special-file match
 against pairs as well.
Date: Mon, 12 Dec 2022 22:09:49 +0000

On 2022-12-12 20:34, Josselin Poiret wrote:
> Hi Bruno, 
> 
> Is this patch related to some specific problem you're running into?  I
> personally would prefer keeping the special file interface as-is, and
> not mix two different kinds of entries: lists with 2 elements, and
> pairs.  That would avoid having to manage even more edge-cases down the
> line if some more processing is needed.

I'm writing a service definition that uses a special-files-service-type service-extension.
The documentation for it says:
--8<---------------cut here---------------start------------->8---
The value associated with special-files-service-type services must be a list of tuples where the first element is the “special file” and the second element is its target. 
--8<---------------cut here---------------end--------------->8---

I assume a pair is a reasonable interpretation of 'tuples' in this context, so I proceeded to serialize the fields with:
--8<---------------cut here---------------start------------->8---
(cons "filename here" (mixed-text-file "filename" contents ...))
--8<---------------cut here---------------end--------------->8---

Which I think is the natural way of doing it. (and communicates the intent, a pair with a path and a file-like object.)

Of course, (list "path" file-like-obj) works as well but imo the pair is clearer in purpose.
(what meaning would the third element and so on have, if ever present?)
This I found out the hard way by getting strange errors until I looked into what happens behind
`special-files-service-type' and realizing that only lists were accepted.

The mixing of cases is unfortunate (it should have been pairs from the start) but preserves
compatibility with existing syntax. 

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

From: mirai <mirai <at> makinata.eu>
To: Tobias Geerinckx-Rice <me <at> tobias.gr>, Josselin Poiret <dev <at> jpoiret.xyz>
Cc: 60014 <at> debbugs.gnu.org, guix-patches <at> gnu.org
Subject: Re: [bug#60014] [PATCH] activation: make install-special-file match
 against pairs as well.
Date: Mon, 12 Dec 2022 22:25:05 +0000

On 2022-12-12 20:52, Tobias Geerinckx-Rice wrote:
> Josselin Poiret via Guix-patches via 写道：
>> I personally would prefer keeping the special file interface as-is, and
>> not mix two different kinds of entries: lists with 2 elements, and
>> pairs.  That would avoid having to manage even more edge-cases down the
>> line if some more processing is needed.
> 
> I agree with this reasoning, and would go as far as to say that if this fixes anything, that thing should probably be fixed instead…?
> 
> ‘Takes a list of As, but as a special case, a single A’ is confusing and makes it that much harder for newcomers to move beyond cargo-culting magical snippets.

That's not what's happening here, right now what guix does is: take a list of tuples, where tuples are 2-element lists of path + file-like.
This patch does: take a list of tuples, where tuples are pairs of path + file-like (and as a bonus,
preserve existing configurations by allowing the pairs to be lists as well).

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

From: Josselin Poiret <dev <at> jpoiret.xyz>
To: mirai <mirai <at> makinata.eu>, 60014 <at> debbugs.gnu.org
Subject: Re: [bug#60014] [PATCH] activation: make install-special-file match
 against pairs as well.
Date: Tue, 13 Dec 2022 11:15:34 +0100

Hi Bruno,

mirai <mirai <at> makinata.eu> writes:

> The documentation for it says:
> --8<---------------cut here---------------start------------->8---
> The value associated with special-files-service-type services must be a list of tuples where the first element is the “special file” and the second element is its target. 
> --8<---------------cut here---------------end--------------->8---
>
> Which I think is the natural way of doing it. (and communicates the intent, a pair with a path and a file-like object.)

Right, that's unfortunate, although that could be changed to “list of
lists” to make it clearer.

> Of course, (list "path" file-like-obj) works as well but imo the pair is clearer in purpose.
> (what meaning would the third element and so on have, if ever present?)
> This I found out the hard way by getting strange errors until I looked into what happens behind
> `special-files-service-type' and realizing that only lists were accepted.
>
> The mixing of cases is unfortunate (it should have been pairs from the start) but preserves
> compatibility with existing syntax. 

I agree with you here, but then I think to avoid having to maintain both
cases at the same time, all existing uses of special-files-service-type
should also be modified, and only one kind should remain, with the other
triggering some deprecation warning.  You could match to `(path
. file-like)`, and if (list? file-like), throw an exception.

As a sidenote, the main problem is that Guile is not a statically typed
language, but that's a whole other debate to have.

In any case, I don't think this patch will be accepted as-is.  I would
only be partially in favor of the second solution (because it breaks
existing code), while the first solution is low-effort and should work
well enough.  Up to you (and maintainers) to decide.

Best,
-- 
Josselin Poiret

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

From: mirai <mirai <at> makinata.eu>
To: Josselin Poiret <dev <at> jpoiret.xyz>, 60014 <at> debbugs.gnu.org
Subject: Re: [bug#60014] [PATCH] activation: make install-special-file match
 against pairs as well.
Date: Tue, 13 Dec 2022 13:04:07 +0000

On 2022-12-13 10:15, Josselin Poiret wrote:
> Hi Bruno,
> 
> mirai <mirai <at> makinata.eu> writes:
> 
>> The documentation for it says:
>> --8<---------------cut here---------------start------------->8---
>> The value associated with special-files-service-type services must be a list of tuples where the first element is the “special file” and the second element is its target. 
>> --8<---------------cut here---------------end--------------->8---
>>
>> Which I think is the natural way of doing it. (and communicates the intent, a pair with a path and a file-like object.)
> 
> Right, that's unfortunate, although that could be changed to “list of
> lists” to make it clearer.
> 
>> Of course, (list "path" file-like-obj) works as well but imo the pair is clearer in purpose.
>> (what meaning would the third element and so on have, if ever present?)
>> This I found out the hard way by getting strange errors until I looked into what happens behind
>> `special-files-service-type' and realizing that only lists were accepted.
>>
>> The mixing of cases is unfortunate (it should have been pairs from the start) but preserves
>> compatibility with existing syntax. 
> 

> I agree with you here, but then I think to avoid having to maintain both
> cases at the same time, all existing uses of special-files-service-type
> should also be modified, and only one kind should remain, with the other
> triggering some deprecation warning.  You could match to `(path
> . file-like)`, and if (list? file-like), throw an exception.

The `(= car target) (= cdr file)' match pattern is lifted from
https://git.savannah.gnu.org/cgit/guile.git/tree/module/ice-9/match.upstream.scm?id=b54263dc98b2700fa777745405ad7651601bcdc6#n139
as Guile's Pattern Matching page doesn't specify how to match against pairs when I was looking into it.

> As a sidenote, the main problem is that Guile is not a statically typed
> language, but that's a whole other debate to have.
> 
> In any case, I don't think this patch will be accepted as-is.  I would
> only be partially in favor of the second solution (because it breaks
> existing code), while the first solution is low-effort and should work
> well enough.  Up to you (and maintainers) to decide.

A breaking change here (or a non-breaking "deprecated" warning similar to how
bootloader target field was renamed to targets can be done too, but before
any further changes its best to discuss if such a change will be received.


On 2022-12-12 20:34, Josselin Poiret wrote:
> Otherwise, you're missing the ChangeLog entry format for the commit
> message, which you can find described at [1].  You can take some
> inspiration from other commits in the repository.

I'm missing the link at [1], could you resend it?

Cheers,
Bruno

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

From: Josselin Poiret <dev <at> jpoiret.xyz>
To: mirai <mirai <at> makinata.eu>, 60014 <at> debbugs.gnu.org
Subject: Re: [bug#60014] [PATCH] activation: make install-special-file match
 against pairs as well.
Date: Tue, 13 Dec 2022 20:56:25 +0100

mirai <mirai <at> makinata.eu> writes:

> I'm missing the link at [1], could you resend it?

My bad, here it is

[1] https://www.gnu.org/prep/standards/html_node/Change-Logs.html#Change-Logs
-- 
Josselin Poiret

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

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: mirai <mirai <at> makinata.eu>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, 60014 <at> debbugs.gnu.org,
 guix-patches <at> gnu.org
Subject: Re: [bug#60014] [PATCH] activation: make install-special-file match
 against pairs as well.
Date: Tue, 13 Dec 2022 21:04:42 +0100

[Message part 1 (text/plain, inline)]

Heyo,

mirai 写道：
> This patch does: take a list of tuples, where tuples are pairs 
> of path + file-like

This is fine.

> (and as a bonus,
> preserve existing configurations by allowing the pairs to be 
> lists as well).

This not so much.  I guess my example was poorly chosen, but at 
least deprecate the old style, as jpoiret also suggests.  That 
does not mean you need to instantly break old configurations.

Kind regards,

T G-R

[signature.asc (application/pgp-signature, inline)]

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Josselin Poiret <dev <at> jpoiret.xyz>
Cc: 60014 <at> debbugs.gnu.org, mirai <at> makinata.eu
Subject: Re: bug#60014: [PATCH] activation: make install-special-file match
 against pairs as well.
Date: Tue, 20 Dec 2022 15:47:10 +0100

Hi,

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

> Is this patch related to some specific problem you're running into?  I
> personally would prefer keeping the special file interface as-is, and
> not mix two different kinds of entries: lists with 2 elements, and
> pairs.  That would avoid having to manage even more edge-cases down the
> line if some more processing is needed.

I agree.  This is a public-facing interface so we should keep it as-is;
extending it to support pairs in addition to two-list elements would
likely bring confusion and bugs.

I’m not entirely sure why we settled on two-list elements rather than
pairs back then, but I think it’s OK.

Closing?

Ludo’.

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

From: mirai <mirai <at> makinata.eu>
To: Ludovic Courtès <ludo <at> gnu.org>,
 Josselin Poiret <dev <at> jpoiret.xyz>
Cc: 60014 <at> debbugs.gnu.org
Subject: Re: bug#60014: [PATCH] activation: make install-special-file match
 against pairs as well.
Date: Wed, 21 Dec 2022 13:20:51 +0000

Hi,

While thinking about this, I've noticed that using lists as "pairs"
is a pattern that is common in the existing guix code, with openssh-service-type
'authorized-keys' field and G-Expressions 'file-union' as examples.

Given the "entrenched" list usage, I don't think it's worth the effort to
change the whole system to use pairs at this point (or maybe allow it as it
probably just creates more confusion).

I will amend the special-files-service-type doc entry to clarify
that it expects two-element lists instead.

Bruno


On 2022-12-20 14:47, Ludovic Courtès wrote:
> Hi,
> 
> Josselin Poiret <dev <at> jpoiret.xyz> skribis:
> 
>> Is this patch related to some specific problem you're running into?  I
>> personally would prefer keeping the special file interface as-is, and
>> not mix two different kinds of entries: lists with 2 elements, and
>> pairs.  That would avoid having to manage even more edge-cases down the
>> line if some more processing is needed.
> 
> I agree.  This is a public-facing interface so we should keep it as-is;
> extending it to support pairs in addition to two-list elements would
> likely bring confusion and bugs.
> 
> I’m not entirely sure why we settled on two-list elements rather than
> pairs back then, but I think it’s OK.
> 
> Closing?
> 
> Ludo’.

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

From: mirai <at> makinata.eu
To: 60014 <at> debbugs.gnu.org
Cc: Bruno Victal <mirai <at> makinata.eu>
Subject: [PATCH v2] doc: Clarify special-files-service-type expected value.
Date: Wed, 21 Dec 2022 13:31:44 +0000

From: Bruno Victal <mirai <at> makinata.eu>

* doc/guix.texi (Services, Base Services): Clarify special-files-service-type
expected value.
---
 doc/guix.texi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index fd03da8c97..a9b6e1231d 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -17753,7 +17753,7 @@ This is the service that sets up ``special files'' such as
 @file{/bin/sh}; an instance of it is part of @code{%base-services}.
 
 The value associated with @code{special-files-service-type} services
-must be a list of tuples where the first element is the ``special file''
+must be a list of two-element lists where the first element is the ``special file''
 and the second element is its target.  By default it is:
 
 @cindex @file{/bin/sh}

base-commit: 7833acab0da02335941974608510c02e2d1d8069
-- 
2.38.1

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

From: Bruno Victal <mirai <at> makinata.eu>
To: 60014 <at> debbugs.gnu.org
Subject: Re: [PATCH v2] doc: Clarify special-files-service-type expected value.
Date: Sat, 18 Feb 2023 02:33:08 +0000

bump

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

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: mirai <at> makinata.eu
Cc: 60014-done <at> debbugs.gnu.org
Subject: Re: bug#60014: [PATCH v2] doc: Clarify special-files-service-type
 expected value.
Date: Tue, 21 Mar 2023 10:15:15 -0400

Hello,

mirai <at> makinata.eu writes:

> From: Bruno Victal <mirai <at> makinata.eu>
>
> * doc/guix.texi (Services, Base Services): Clarify special-files-service-type
> expected value.
> ---
>  doc/guix.texi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index fd03da8c97..a9b6e1231d 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -17753,7 +17753,7 @@ This is the service that sets up ``special files'' such as
>  @file{/bin/sh}; an instance of it is part of @code{%base-services}.
>  
>  The value associated with @code{special-files-service-type} services
> -must be a list of tuples where the first element is the ``special file''
> +must be a list of two-element lists where the first element is the ``special file''
>  and the second element is its target.  By default it is:

Applied!

-- 
Thanks,
Maxim

Previous Next

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