[PATCH v2] doc: Clarify special-files-service-type expected value.

  • Open
  • quality assurance status badge
Details
4 participants
  • Josselin Poiret
  • Ludovic Courtès
  • Tobias Geerinckx-Rice
  • mirai
Owner
unassigned
Submitted by
mirai
Severity
normal
M
[PATCH] activation: make install-special-file match against pairs as well.
(address . guix-patches@gnu.org)(name . Bruno Victal)(address . mirai@makinata.eu)
43e937e1625b47a80887e68847fb8a8811d3f39f.1670867103.git.mirai@makinata.eu
From: Bruno Victal <mirai@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(-)

Toggle diff (17 lines)
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
J
J
Josselin Poiret wrote on 12 Dec 2022 21:34
(name . Bruno Victal)(address . mirai@makinata.eu)
87k02wfjk5.fsf@jpoiret.xyz
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
T
T
Tobias Geerinckx-Rice wrote on 12 Dec 2022 21:52
(name . Josselin Poiret)(address . dev@jpoiret.xyz)
87v8mgcp45.fsf@nckx
Josselin Poiret via Guix-patches via 写道:
Toggle quote (8 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iIMEARYKACsWIQT12iAyS4c9C3o4dnINsP+IT1VteQUCY5eW2g0cbWVAdG9iaWFz
LmdyAAoJEA2w/4hPVW15m4YA/0cLEmsPszjIfH/blAGdD/CmL1BCexW72y1CVFdP
eMq5AQDR/cPdCRqZTHImc1GSwi/WsY1Poc8035zOCG6NSc/JDw==
=jSFi
-----END PGP SIGNATURE-----

M
7f00edbf-8b39-0d3e-4d29-0815176d143c@makinata.eu
On 2022-12-12 20:34, Josselin Poiret wrote:
Toggle quote (8 lines)
> 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:
Toggle snippet (3 lines)
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.

I assume a pair is a reasonable interpretation of 'tuples' in this context, so I proceeded to serialize the fields with:
Toggle snippet (3 lines)
(cons "filename here" (mixed-text-file "filename" contents ...))

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.
M
cc11dcaa-3acd-dd47-0c80-1a98197c9738@makinata.eu
On 2022-12-12 20:52, Tobias Geerinckx-Rice wrote:
Toggle quote (10 lines)
> 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).
J
J
Josselin Poiret wrote on 13 Dec 2022 11:15
87h6xzfw3d.fsf@jpoiret.xyz
Hi Bruno,

mirai <mirai@makinata.eu> writes:

Toggle quote (7 lines)
> 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.

Toggle quote (8 lines)
> 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
M
4e874c04-8cad-6d72-379b-967721b82713@makinata.eu
On 2022-12-13 10:15, Josselin Poiret wrote:
Toggle quote (29 lines)
> Hi Bruno,
>
> mirai <mirai@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
as Guile's Pattern Matching page doesn't specify how to match against pairs when I was looking into it.

Toggle quote (8 lines)
> 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:
Toggle quote (4 lines)
> 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
J
T
T
Tobias Geerinckx-Rice wrote on 13 Dec 2022 21:04
(name . mirai)(address . mirai@makinata.eu)
87ilifcbgh.fsf@nckx
Heyo,

mirai 写道:
Toggle quote (3 lines)
> This patch does: take a list of tuples, where tuples are pairs
> of path + file-like

This is fine.

Toggle quote (4 lines)
> (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
-----BEGIN PGP SIGNATURE-----

iIMEARYKACsWIQT12iAyS4c9C3o4dnINsP+IT1VteQUCY5jb/g0cbWVAdG9iaWFz
LmdyAAoJEA2w/4hPVW15wu8A/j/W98VS3V1ZpmUcCUOYPp6a4VPmYU6nvJG0WK5n
HRXSAP9VWP0EQTX9hqy9qPoFIzUmqrvBa3JZ7zocWv0rSqWkCg==
=f60/
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 20 Dec 2022 15:47
Re: bug#60014: [PATCH] activation: make install-special-file match against pairs as well.
(name . Josselin Poiret)(address . dev@jpoiret.xyz)
87ili66sk1.fsf_-_@gnu.org
Hi,

Josselin Poiret <dev@jpoiret.xyz> skribis:

Toggle quote (6 lines)
> 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’.
M
(address . 60014@debbugs.gnu.org)
15a36fc9-05ce-d599-5539-166de64e9f04@makinata.eu
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:
Toggle quote (20 lines)
> Hi,
>
> Josselin Poiret <dev@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’.
M
[PATCH v2] doc: Clarify special-files-service-type expected value.
(address . 60014@debbugs.gnu.org)(name . Bruno Victal)(address . mirai@makinata.eu)
f0c786997ad95cfacee70648a22e74dc5348c823.1671629504.git.mirai@makinata.eu
From: Bruno Victal <mirai@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(-)

Toggle diff (17 lines)
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
M
(no subject)
(name . control)(address . control@debbugs.gnu.org)
4866cd86-648e-61f5-d84b-b5297fd90b98@makinata.eu
retitle 60014 [PATCH v2] doc: Clarify special-files-service-type expected value.
quit
?