[bug#60014] activation: make install-special-file match against pairs as well.

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(-)


base-commit: 5fb5af5658b7575a945579a7cf51c193600b76bb

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 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

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.

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).

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.

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,

On 2022-12-13 10:15, Josselin Poiret wrote:
> 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
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

mirai <mirai@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

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

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’.

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@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’.