GNU bug report logs - #71111
[PATCH 0/1] services: home: Use pairs instead of lists.

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 71111 in the body.
You can then email your comments to 71111 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: Andrew Tropin <andrew <at> trop.in>
To: guix-patches <at> gnu.org
Cc: Richard Sent <richard <at> freakingpenguin.com>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Andrew Tropin <andrew <at> trop.in>
Subject: [PATCH 0/1] services: home: Use pairs instead of lists.
Date: Wed, 22 May 2024 14:02:26 +0400

After rewriting from car/cdr to match-lambda in v2 of this patch:
https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard <at> freakingpenguin.com/

the format changed from pairs to lists, I didn't noticed this nuance
during review because the documentation still says that service should
be configured and extended with pairs.  Also, pairs are more
apropriate data type here.  And this match-lambda rewrite will break
downstream RDE user's setups after migrating to upstreamed version of
service.

That's why I propose to go back to pairs.

Andrew Tropin (1):
  services: home: Use pairs instead of lists.

 doc/guix.texi         | 4 ++--
 gnu/services/guix.scm | 2 +-
 gnu/tests/guix.scm    | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)


base-commit: b06a460bf5236a9d52f936f2023451051b3e622a
-- 
2.41.0

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

From: Andrew Tropin <andrew <at> trop.in>
To: guix-patches <at> gnu.org
Cc: Richard Sent <richard <at> freakingpenguin.com>,
 Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH 1/1] services: home: Use pairs instead of lists.
Date: Wed, 22 May 2024 15:06:55 +0400

* gnu/services/guix.scm: Use pairs instead of lists.
* doc/guix.texi: Update accordingly.
* gnu/tests/guix.scm: Update accordingly.

Change-Id: I0b8d3fa5b214add89bdb84a11fa20d1b319435f0
---
 doc/guix.texi         | 4 ++--
 gnu/services/guix.scm | 2 +-
 gnu/tests/guix.scm    | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 8073e3f6d4..8cc5edc805 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -39576,7 +39576,7 @@ Guix Services
 
 (operating-system
   (services (append (list (service guix-home-service-type
-                                   `(("alice" ,my-home))))
+                                   `(("alice" . ,my-home))))
                     %base-services)))
 @end lisp
 
@@ -39585,7 +39585,7 @@ Guix Services
 
 @lisp
 (simple-service 'my-extra-home home-service-type
-                `(("bob" ,my-extra-home))))
+                `(("bob" . ,my-extra-home))))
 @end lisp
 @end defvar
 
diff --git a/gnu/services/guix.scm b/gnu/services/guix.scm
index 96f5ecaac0..1f0e2a310d 100644
--- a/gnu/services/guix.scm
+++ b/gnu/services/guix.scm
@@ -696,7 +696,7 @@ (define guix-data-service-type
 
 (define (guix-home-shepherd-service config)
   (map (match-lambda
-         ((user he)
+         ((user . he)
           (shepherd-service
            (documentation "Activate Guix Home.")
            (requirement '(user-processes))
diff --git a/gnu/tests/guix.scm b/gnu/tests/guix.scm
index 12ad1bf255..6071cb018e 100644
--- a/gnu/tests/guix.scm
+++ b/gnu/tests/guix.scm
@@ -271,7 +271,7 @@ (define %guix-home-service-he
 (define %guix-home-service-os
   (simple-operating-system
    (service guix-home-service-type
-            `(("alice" ,%guix-home-service-he)))))
+            `(("alice" . ,%guix-home-service-he)))))
 
 (define (run-guix-home-service-test)
   (define os
-- 
2.41.0

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

From: Richard Sent <richard <at> freakingpenguin.com>
To: Andrew Tropin <andrew <at> trop.in>
Cc: Ludovic Courtès <ludo <at> gnu.org>, guix-patches <at> gnu.org
Subject: Re: [PATCH 0/1] services: home: Use pairs instead of lists.
Date: Wed, 22 May 2024 17:33:57 -0400

Andrew Tropin <andrew <at> trop.in> writes:

> After rewriting from car/cdr to match-lambda in v2 of this patch:
> https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard <at> freakingpenguin.com/
>
> the format changed from pairs to lists, I didn't noticed this nuance
> during review because the documentation still says that service should
> be configured and extended with pairs.  Also, pairs are more
> apropriate data type here.  And this match-lambda rewrite will break
> downstream RDE user's setups after migrating to upstreamed version of
> service.
>
> That's why I propose to go back to pairs.
>

I'm not opposed to going back to cons cell pairs. I didn't put too much
thought in a "list of two elements" vs. "cons cell" besides the match
statement being easier to handle with a list.

Would this patch have unintended side effects? I thought the . in match
conditions had a different behavior.

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> (match '(1 2 3 4)
                       ((a . b) b))
$5 = (2 3 4)
scheme@(guile-user)> (match '(1 2)
                       ((a . b) b))
$6 = (2)
scheme@(guile-user)> (match '(1 . 2)
                       ((a . b) b))
$7 = 2
--8<---------------cut here---------------end--------------->8---

So changing to this would allow for a home-service entry like the
following to match:

--8<---------------cut here---------------start------------->8---
(simple-service 'my-extra-home home-service-type
                `(("bob" "bill" "bertha" "billyanne" ,my-extra-home ,my-home-2)))
--8<---------------cut here---------------end--------------->8---

From my testing, this /will/ error (yay), but not as soon as the match
would with the current code. Instead of being caught before being passed
to the daemon, it seems to be caught while lowering the invalid
file-append object.

Personally I would prefer to catch as many errors as possible before
beginning to pass code off to the daemon where possible. Generally
speaking it feels like pre-daemon errors are easier to mentally parse.

In fairness, the current code also isn't trying particularly hard to
check that "user" is a string and "he" is a home-environment or printing
a fancy error message.

Perhaps this would work?

--8<---------------cut here---------------start------------->8---
(define-module (gnu services guix)
  ...
  #:use-module (gnu home))
...
(define (guix-home-shepherd-service config)
  (map (match-lambda
         (((? string? user) . (? home-environment? he))
          (shepherd-service
           ...
           ))
         (e (error "Invalid value for guix-home-shepherd-service: " e)))
       config))
--8<---------------cut here---------------end--------------->8---

Maybe I'm just being silly. 🙂

-- 
Take it easy,
Richard Sent
Making my computer weirder one commit at a time.

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

From: Zheng Junjie <zhengjunjie <at> iscas.ac.cn>
To: Andrew Tropin via Guix-patches via <guix-patches <at> gnu.org>
Cc: 71111 <at> debbugs.gnu.org, Maxim Cournoyer <maxim.cournoyer <at> gmail.com>,
 Richard Sent <richard <at> freakingpenguin.com>,
 Florian Pelz <pelzflorian <at> pelzflorian.de>, Andrew Tropin <andrew <at> trop.in>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Matthew Trzcinski <matt <at> excalamus.com>
Subject: Re: [bug#71111] [PATCH 0/1] services: home: Use pairs instead of
 lists.
Date: Thu, 23 May 2024 11:38:15 +0800

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

Andrew Tropin via Guix-patches via <guix-patches <at> gnu.org> writes:

> After rewriting from car/cdr to match-lambda in v2 of this patch:
> https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard <at> freakingpenguin.com/
>
> the format changed from pairs to lists, I didn't noticed this nuance
> during review because the documentation still says that service should
> be configured and extended with pairs.  Also, pairs are more
> apropriate data type here.  And this match-lambda rewrite will break
> downstream RDE user's setups after migrating to upstreamed version of
> service.
>
> That's why I propose to go back to pairs.

Maybe we can support pairs and list of length two at same time?

>
> Andrew Tropin (1):
>   services: home: Use pairs instead of lists.
>
>  doc/guix.texi         | 4 ++--
>  gnu/services/guix.scm | 2 +-
>  gnu/tests/guix.scm    | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
>
> base-commit: b06a460bf5236a9d52f936f2023451051b3e622a

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

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

From: Andrew Tropin <andrew <at> trop.in>
To: Zheng Junjie <zhengjunjie <at> iscas.ac.cn>, Andrew Tropin via Guix-patches
 via <guix-patches <at> gnu.org>
Cc: 71111 <at> debbugs.gnu.org, Maxim Cournoyer <maxim.cournoyer <at> gmail.com>,
 Richard Sent <richard <at> freakingpenguin.com>,
 Florian Pelz <pelzflorian <at> pelzflorian.de>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Matthew Trzcinski <matt <at> excalamus.com>
Subject: Re: [bug#71111] [PATCH 0/1] services: home: Use pairs instead of
 lists.
Date: Thu, 23 May 2024 09:43:47 +0400

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

On 2024-05-23 11:38, Zheng Junjie wrote:

> Andrew Tropin via Guix-patches via <guix-patches <at> gnu.org> writes:
>
>> After rewriting from car/cdr to match-lambda in v2 of this patch:
>> https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard <at> freakingpenguin.com/
>>
>> the format changed from pairs to lists, I didn't noticed this nuance
>> during review because the documentation still says that service should
>> be configured and extended with pairs.  Also, pairs are more
>> apropriate data type here.  And this match-lambda rewrite will break
>> downstream RDE user's setups after migrating to upstreamed version of
>> service.
>>
>> That's why I propose to go back to pairs.
>
> Maybe we can support pairs and list of length two at same time?

Thank you for the idea, however I think ambiguity is a bad practice,
from my early experience with guix it's more confusing rather than
helpful.  I still don't know why profile-service-type accepts list of
lists rather than alist (list of pairs).

>
>>
>> Andrew Tropin (1):
>>   services: home: Use pairs instead of lists.
>>
>>  doc/guix.texi         | 4 ++--
>>  gnu/services/guix.scm | 2 +-
>>  gnu/tests/guix.scm    | 2 +-
>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>
>>
>> base-commit: b06a460bf5236a9d52f936f2023451051b3e622a

-- 
Best regards,
Andrew Tropin

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

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

From: Andrew Tropin <andrew <at> trop.in>
To: Richard Sent <richard <at> freakingpenguin.com>
Cc: Ludovic Courtès <ludo <at> gnu.org>, guix-patches <at> gnu.org
Subject: Re: [PATCH 0/1] services: home: Use pairs instead of lists.
Date: Thu, 23 May 2024 09:45:02 +0400

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

On 2024-05-22 17:33, Richard Sent wrote:

> Andrew Tropin <andrew <at> trop.in> writes:
>
>> After rewriting from car/cdr to match-lambda in v2 of this patch:
>> https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard <at> freakingpenguin.com/
>>
>> the format changed from pairs to lists, I didn't noticed this nuance
>> during review because the documentation still says that service should
>> be configured and extended with pairs.  Also, pairs are more
>> apropriate data type here.  And this match-lambda rewrite will break
>> downstream RDE user's setups after migrating to upstreamed version of
>> service.
>>
>> That's why I propose to go back to pairs.
>>
>
> I'm not opposed to going back to cons cell pairs. I didn't put too much
> thought in a "list of two elements" vs. "cons cell" besides the match
> statement being easier to handle with a list.
>
> Would this patch have unintended side effects? I thought the . in match
> conditions had a different behavior.
>
> --8<---------------cut here---------------start------------->8---
> scheme@(guile-user)> (match '(1 2 3 4)
>                        ((a . b) b))
> $5 = (2 3 4)
> scheme@(guile-user)> (match '(1 2)
>                        ((a . b) b))
> $6 = (2)
> scheme@(guile-user)> (match '(1 . 2)
>                        ((a . b) b))
> $7 = 2
> --8<---------------cut here---------------end--------------->8---
>
> So changing to this would allow for a home-service entry like the
> following to match:
>
> --8<---------------cut here---------------start------------->8---
> (simple-service 'my-extra-home home-service-type
>                 `(("bob" "bill" "bertha" "billyanne" ,my-extra-home ,my-home-2)))
> --8<---------------cut here---------------end--------------->8---
>
> From my testing, this /will/ error (yay), but not as soon as the match
> would with the current code. Instead of being caught before being passed
> to the daemon, it seems to be caught while lowering the invalid
> file-append object.
>
> Personally I would prefer to catch as many errors as possible before
> beginning to pass code off to the daemon where possible. Generally
> speaking it feels like pre-daemon errors are easier to mentally parse.
>
> In fairness, the current code also isn't trying particularly hard to
> check that "user" is a string and "he" is a home-environment or printing
> a fancy error message.
>
> Perhaps this would work?
>
> --8<---------------cut here---------------start------------->8---
> (define-module (gnu services guix)
>   ...
>   #:use-module (gnu home))
> ...
> (define (guix-home-shepherd-service config)
>   (map (match-lambda
>          (((? string? user) . (? home-environment? he))
>           (shepherd-service
>            ...
>            ))
>          (e (error "Invalid value for guix-home-shepherd-service: " e)))
>        config))
> --8<---------------cut here---------------end--------------->8---

This idea is good, I'll incorporate this into v2.

>
> Maybe I'm just being silly. 🙂

-- 
Best regards,
Andrew Tropin

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

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

From: Andrew Tropin <andrew <at> trop.in>
To: guix-patches <at> gnu.org
Cc: Andrew Tropin <andrew <at> trop.in>
Subject: [PATCH v2 0/1] services: home: Use pairs instead of lists.
Date: Thu, 23 May 2024 09:53:43 +0400

Use more precise pattern matching and signal error if it is not a pair
of string/home-environment.

Andrew Tropin (1):
  services: home: Use pairs instead of lists.

 doc/guix.texi         | 4 ++--
 gnu/services/guix.scm | 6 ++++--
 gnu/tests/guix.scm    | 2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)


base-commit: b06a460bf5236a9d52f936f2023451051b3e622a
-- 
2.41.0

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

From: Andrew Tropin <andrew <at> trop.in>
To: guix-patches <at> gnu.org
Cc: Richard Sent <richard <at> freakingpenguin.com>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Andrew Tropin <andrew <at> trop.in>
Subject: [PATCH v2 1/1] services: home: Use pairs instead of lists.
Date: Thu, 23 May 2024 09:53:44 +0400

* gnu/services/guix.scm: Use pairs instead of lists.
* doc/guix.texi: Update accordingly.
* gnu/tests/guix.scm: Update accordingly.

Change-Id: I0b8d3fa5b214add89bdb84a11fa20d1b319435f0
---
 doc/guix.texi         | 4 ++--
 gnu/services/guix.scm | 6 ++++--
 gnu/tests/guix.scm    | 2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 8073e3f6d4..8cc5edc805 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -39576,7 +39576,7 @@ Guix Services
 
 (operating-system
   (services (append (list (service guix-home-service-type
-                                   `(("alice" ,my-home))))
+                                   `(("alice" . ,my-home))))
                     %base-services)))
 @end lisp
 
@@ -39585,7 +39585,7 @@ Guix Services
 
 @lisp
 (simple-service 'my-extra-home home-service-type
-                `(("bob" ,my-extra-home))))
+                `(("bob" . ,my-extra-home))))
 @end lisp
 @end defvar
 
diff --git a/gnu/services/guix.scm b/gnu/services/guix.scm
index 96f5ecaac0..3818749baa 100644
--- a/gnu/services/guix.scm
+++ b/gnu/services/guix.scm
@@ -696,7 +696,7 @@ (define guix-data-service-type
 
 (define (guix-home-shepherd-service config)
   (map (match-lambda
-         ((user he)
+         (((? string? user) . (? home-environment? he))
           (shepherd-service
            (documentation "Activate Guix Home.")
            (requirement '(user-processes))
@@ -710,7 +710,9 @@ (define (guix-home-shepherd-service config)
                      (list (string-append "HOME=" (passwd:dir (getpw #$user)))
                            "GUIX_SYSTEM_IS_RUNNING_HOME_ACTIVATE=t")
                      #:group (group:name (getgrgid (passwd:gid (getpw #$user))))))
-           (stop #~(make-kill-destructor)))))
+           (stop #~(make-kill-destructor))))
+         (e (error "Invalid value for guix-home, it should be in a form of
+(\"user-name\" . home-environment), but the following value is provided:\n" e)))
        config))
 
 (define guix-home-service-type
diff --git a/gnu/tests/guix.scm b/gnu/tests/guix.scm
index 12ad1bf255..6071cb018e 100644
--- a/gnu/tests/guix.scm
+++ b/gnu/tests/guix.scm
@@ -271,7 +271,7 @@ (define %guix-home-service-he
 (define %guix-home-service-os
   (simple-operating-system
    (service guix-home-service-type
-            `(("alice" ,%guix-home-service-he)))))
+            `(("alice" . ,%guix-home-service-he)))))
 
 (define (run-guix-home-service-test)
   (define os
-- 
2.41.0

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Andrew Tropin <andrew <at> trop.in>
Cc: Richard Sent <richard <at> freakingpenguin.com>,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, 71111 <at> debbugs.gnu.org,
 Florian Pelz <pelzflorian <at> pelzflorian.de>,
 Matthew Trzcinski <matt <at> excalamus.com>
Subject: Re: [bug#71111] [PATCH v2 1/1] services: home: Use pairs instead of
 lists.
Date: Thu, 23 May 2024 11:16:39 +0200

Hi Andrew,

Andrew Tropin <andrew <at> trop.in> skribis:

>  (operating-system
>    (services (append (list (service guix-home-service-type
> -                                   `(("alice" ,my-home))))
> +                                   `(("alice" . ,my-home))))

What’s the rationale for this?

In general I think we should avoid gratuitous incompatible changes.

Thanks,
Ludo’.

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

From: Andrew Tropin <andrew <at> trop.in>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Richard Sent <richard <at> freakingpenguin.com>,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, 71111 <at> debbugs.gnu.org,
 Florian Pelz <pelzflorian <at> pelzflorian.de>,
 Matthew Trzcinski <matt <at> excalamus.com>
Subject: Re: [bug#71111] [PATCH v2 1/1] services: home: Use pairs instead of
 lists.
Date: Thu, 23 May 2024 17:06:48 +0400

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

On 2024-05-23 11:16, Ludovic Courtès wrote:

> Hi Andrew,
>
> Andrew Tropin <andrew <at> trop.in> skribis:
>
>>  (operating-system
>>    (services (append (list (service guix-home-service-type
>> -                                   `(("alice" ,my-home))))
>> +                                   `(("alice" . ,my-home))))
>
> What’s the rationale for this?

--8<---------------cut here---------------start------------->8---
After rewriting from car/cdr to match-lambda in v2 of this patch:
https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard <at> freakingpenguin.com/

the format changed from pairs to lists, I didn't noticed this nuance
during review because the documentation still says that service should
be configured and extended with pairs.  Also, pairs are more
apropriate data type here.  And this match-lambda rewrite will break
downstream RDE user's setups after migrating to upstreamed version of
service.

That's why I propose to go back to pairs.
--8<---------------cut here---------------end--------------->8---

>
> In general I think we should avoid gratuitous incompatible changes.

Agree.  This API is very young, so I think it make sense to update it in
this particular case, considering rationale above.

-- 
Best regards,
Andrew Tropin

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

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

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Andrew Tropin <andrew <at> trop.in>
Cc: 71111 <at> debbugs.gnu.org, Richard Sent <richard <at> freakingpenguin.com>,
 Zheng Junjie <zhengjunjie <at> iscas.ac.cn>,
 Florian Pelz <pelzflorian <at> pelzflorian.de>,
 Matthew Trzcinski <matt <at> excalamus.com>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Andrew Tropin via Guix-patches via <guix-patches <at> gnu.org>
Subject: Re: [bug#71111] [PATCH 0/1] services: home: Use pairs instead of
 lists.
Date: Thu, 23 May 2024 12:02:42 -0400

Hi,

Andrew Tropin <andrew <at> trop.in> writes:

> On 2024-05-23 11:38, Zheng Junjie wrote:
>
>> Andrew Tropin via Guix-patches via <guix-patches <at> gnu.org> writes:
>>
>>> After rewriting from car/cdr to match-lambda in v2 of this patch:
>>> https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard <at> freakingpenguin.com/
>>>
>>> the format changed from pairs to lists, I didn't noticed this nuance
>>> during review because the documentation still says that service should
>>> be configured and extended with pairs.  Also, pairs are more
>>> apropriate data type here.  And this match-lambda rewrite will break
>>> downstream RDE user's setups after migrating to upstreamed version of
>>> service.
>>>
>>> That's why I propose to go back to pairs.
>>
>> Maybe we can support pairs and list of length two at same time?
>
> Thank you for the idea, however I think ambiguity is a bad practice,
> from my early experience with guix it's more confusing rather than
> helpful.

I agree.

-- 
Thanks,
Maxim

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

From: Andrew Tropin <andrew <at> trop.in>
To: Andrew Tropin via Guix-patches via <guix-patches <at> gnu.org>,
 71111-done <at> debbugs.gnu.org
Cc: Richard Sent <richard <at> freakingpenguin.com>,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Matthew Trzcinski <matt <at> excalamus.com>,
 Florian Pelz <pelzflorian <at> pelzflorian.de>
Subject: Re: [bug#71111] [PATCH 0/1] services: home: Use pairs instead of
 lists.
Date: Sun, 02 Jun 2024 13:50:44 +0400

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

On 2024-05-22 14:02, Andrew Tropin via Guix-patches via wrote:

> After rewriting from car/cdr to match-lambda in v2 of this patch:
> https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard <at> freakingpenguin.com/
>
> the format changed from pairs to lists, I didn't noticed this nuance
> during review because the documentation still says that service should
> be configured and extended with pairs.  Also, pairs are more
> apropriate data type here.  And this match-lambda rewrite will break
> downstream RDE user's setups after migrating to upstreamed version of
> service.
>
> That's why I propose to go back to pairs.
>
> Andrew Tropin (1):
>   services: home: Use pairs instead of lists.
>
>  doc/guix.texi         | 4 ++--
>  gnu/services/guix.scm | 2 +-
>  gnu/tests/guix.scm    | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
>
> base-commit: b06a460bf5236a9d52f936f2023451051b3e622a

Merged v2 with updated API and additional type checks.

-- 
Best regards,
Andrew Tropin

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

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Andrew Tropin <andrew <at> trop.in>
Cc: Richard Sent <richard <at> freakingpenguin.com>, 71111 <at> debbugs.gnu.org,
 Matthew Trzcinski <matt <at> excalamus.com>,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>,
 Florian Pelz <pelzflorian <at> pelzflorian.de>
Subject: Re: [bug#71111] [PATCH 0/1] services: home: Use pairs instead of
 lists.
Date: Sun, 02 Jun 2024 12:15:14 +0200

Hi Andrew,

Andrew Tropin <andrew <at> trop.in> skribis:

> On 2024-05-22 14:02, Andrew Tropin via Guix-patches via wrote:
>
>> After rewriting from car/cdr to match-lambda in v2 of this patch:
>> https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard <at> freakingpenguin.com/
>>
>> the format changed from pairs to lists, I didn't noticed this nuance
>> during review because the documentation still says that service should
>> be configured and extended with pairs.  Also, pairs are more
>> apropriate data type here.  And this match-lambda rewrite will break
>> downstream RDE user's setups after migrating to upstreamed version of
>> service.
>>
>> That's why I propose to go back to pairs.
>>
>> Andrew Tropin (1):
>>   services: home: Use pairs instead of lists.
>>
>>  doc/guix.texi         | 4 ++--
>>  gnu/services/guix.scm | 2 +-
>>  gnu/tests/guix.scm    | 2 +-
>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>
>>
>> base-commit: b06a460bf5236a9d52f936f2023451051b3e622a
>
> Merged v2 with updated API and additional type checks.

Perhaps I wasn’t clear enough when asking for clarifications¹, but I
think this change shouldn’t happen: first because it’s an incompatible
change that will break user configs, and second because it’s
inconsistent with other similar interfaces (such as ‘authorized-keys’
and <openssh-configuration>).

For these reasons, I’m in favor of reverting this change.

What do others think?

Aside, it’s unfortunate that you weren’t around to review this patch
initially, despite being one of the recipients:
<https://issues.guix.gnu.org/69781>.  I think it’s important to not give
the impression that you chime in just when an rde incompatibility comes
up.

Thanks,
Ludo’.

¹ https://issues.guix.gnu.org/71111#8

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

From: Efraim Flashner <efraim <at> flashner.co.il>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 71111 <at> debbugs.gnu.org, Maxim Cournoyer <maxim.cournoyer <at> gmail.com>,
 Richard Sent <richard <at> freakingpenguin.com>,
 Florian Pelz <pelzflorian <at> pelzflorian.de>, Andrew Tropin <andrew <at> trop.in>,
 Matthew Trzcinski <matt <at> excalamus.com>
Subject: Re: [bug#71111] [PATCH 0/1] services: home: Use pairs instead of
 lists.
Date: Sun, 2 Jun 2024 13:37:34 +0300

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

On Sun, Jun 02, 2024 at 12:15:14PM +0200, Ludovic Courtès wrote:
> Hi Andrew,
> 
> Andrew Tropin <andrew <at> trop.in> skribis:
> 
> > On 2024-05-22 14:02, Andrew Tropin via Guix-patches via wrote:
> >
> >> After rewriting from car/cdr to match-lambda in v2 of this patch:
> >> https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard <at> freakingpenguin.com/
> >>
> >> the format changed from pairs to lists, I didn't noticed this nuance
> >> during review because the documentation still says that service should
> >> be configured and extended with pairs.  Also, pairs are more
> >> apropriate data type here.  And this match-lambda rewrite will break
> >> downstream RDE user's setups after migrating to upstreamed version of
> >> service.
> >>
> >> That's why I propose to go back to pairs.
> >>
> >> Andrew Tropin (1):
> >>   services: home: Use pairs instead of lists.
> >>
> >>  doc/guix.texi         | 4 ++--
> >>  gnu/services/guix.scm | 2 +-
> >>  gnu/tests/guix.scm    | 2 +-
> >>  3 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >>
> >> base-commit: b06a460bf5236a9d52f936f2023451051b3e622a
> >
> > Merged v2 with updated API and additional type checks.
> 
> Perhaps I wasn’t clear enough when asking for clarifications¹, but I
> think this change shouldn’t happen: first because it’s an incompatible
> change that will break user configs, and second because it’s
> inconsistent with other similar interfaces (such as ‘authorized-keys’
> and <openssh-configuration>).
> 
> For these reasons, I’m in favor of reverting this change.
> 
> What do others think?

This patch also added home-environment? without adding an import of
(gnu home).

It's unfortunate that the wording for the manual says 'pair' when it's a
list, but IMO that's more of a typo in the manual than a mistake in the
code.

With a quick look I didn't see in any of my OS configs configurations
with pair notations, even with simple-service or extra-special-file,
where it would have been most likely.

I think it would be best to roll this back.

> Aside, it’s unfortunate that you weren’t around to review this patch
> initially, despite being one of the recipients:
> <https://issues.guix.gnu.org/69781>.  I think it’s important to not give
> the impression that you chime in just when an rde incompatibility comes
> up.
> 
> Thanks,
> Ludo’.
> 
> ¹ https://issues.guix.gnu.org/71111#8
> 
> 
> 

-- 
Efraim Flashner   <efraim <at> flashner.co.il>   רנשלפ םירפא
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

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

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

From: Andrew Tropin <andrew <at> trop.in>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Richard Sent <richard <at> freakingpenguin.com>, 71111 <at> debbugs.gnu.org,
 Matthew Trzcinski <matt <at> excalamus.com>,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>,
 Florian Pelz <pelzflorian <at> pelzflorian.de>
Subject: Re: [bug#71111] [PATCH 0/1] services: home: Use pairs instead of
 lists.
Date: Sun, 02 Jun 2024 14:57:28 +0400

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

On 2024-06-02 12:15, Ludovic Courtès wrote:

> Hi Andrew,
>
> Andrew Tropin <andrew <at> trop.in> skribis:
>
>> On 2024-05-22 14:02, Andrew Tropin via Guix-patches via wrote:
>>
>>> After rewriting from car/cdr to match-lambda in v2 of this patch:
>>> https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard <at> freakingpenguin.com/
>>>
>>> the format changed from pairs to lists, I didn't noticed this nuance
>>> during review because the documentation still says that service should
>>> be configured and extended with pairs.  Also, pairs are more
>>> apropriate data type here.  And this match-lambda rewrite will break
>>> downstream RDE user's setups after migrating to upstreamed version of
>>> service.
>>>
>>> That's why I propose to go back to pairs.
>>>
>>> Andrew Tropin (1):
>>>   services: home: Use pairs instead of lists.
>>>
>>>  doc/guix.texi         | 4 ++--
>>>  gnu/services/guix.scm | 2 +-
>>>  gnu/tests/guix.scm    | 2 +-
>>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>>
>>> base-commit: b06a460bf5236a9d52f936f2023451051b3e622a
>>
>> Merged v2 with updated API and additional type checks.
>
> Perhaps I wasn’t clear enough when asking for clarifications¹, but I
> think this change shouldn’t happen: first because it’s an incompatible
> change that will break user configs, and second because it’s
> inconsistent with other similar interfaces (such as ‘authorized-keys’
> and <openssh-configuration>).
>
> For these reasons, I’m in favor of reverting this change.
>
> What do others think?
>
> Aside, it’s unfortunate that you weren’t around to review this patch
> initially, despite being one of the recipients:
> <https://issues.guix.gnu.org/69781>.

We discussed the upstreaming of this service with Richard and I was
following the thread above, so I was around.  I didn't merge or comment
on it because it is literally code written by me, so it make sense to
let someone else to review and merge it.

I didn't realise that in the second revision API was changed from pairs
to lists, when destructuring was rewritten from car/cdr to match.  I
skimmed through the docs and was satisfyed and didn't wrote anything.
It came up only now, when people started reporting problems.

>  I think it’s important to not give the impression that you chime in
> just when an rde incompatibility comes up.

Not sure what you mean.

-- 
Best regards,
Andrew Tropin

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

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

From: Andrew Tropin <andrew <at> trop.in>
To: Efraim Flashner <efraim <at> flashner.co.il>, Ludovic Courtès <ludo <at> gnu.org>
Cc: Richard Sent <richard <at> freakingpenguin.com>, 71111 <at> debbugs.gnu.org,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>,
 Matthew Trzcinski <matt <at> excalamus.com>,
 Florian Pelz <pelzflorian <at> pelzflorian.de>
Subject: Re: [bug#71111] [PATCH 0/1] services: home: Use pairs instead of
 lists.
Date: Sun, 02 Jun 2024 15:12:49 +0400

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

On 2024-06-02 13:37, Efraim Flashner wrote:

> On Sun, Jun 02, 2024 at 12:15:14PM +0200, Ludovic Courtès wrote:
>> Hi Andrew,
>> 
>> Andrew Tropin <andrew <at> trop.in> skribis:
>> 
>> > On 2024-05-22 14:02, Andrew Tropin via Guix-patches via wrote:
>> >
>> >> After rewriting from car/cdr to match-lambda in v2 of this patch:
>> >> https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard <at> freakingpenguin.com/
>> >>
>> >> the format changed from pairs to lists, I didn't noticed this nuance
>> >> during review because the documentation still says that service should
>> >> be configured and extended with pairs.  Also, pairs are more
>> >> apropriate data type here.  And this match-lambda rewrite will break
>> >> downstream RDE user's setups after migrating to upstreamed version of
>> >> service.
>> >>
>> >> That's why I propose to go back to pairs.
>> >>
>> >> Andrew Tropin (1):
>> >>   services: home: Use pairs instead of lists.
>> >>
>> >>  doc/guix.texi         | 4 ++--
>> >>  gnu/services/guix.scm | 2 +-
>> >>  gnu/tests/guix.scm    | 2 +-
>> >>  3 files changed, 4 insertions(+), 4 deletions(-)
>> >>
>> >>
>> >> base-commit: b06a460bf5236a9d52f936f2023451051b3e622a
>> >
>> > Merged v2 with updated API and additional type checks.
>> 
>> Perhaps I wasn’t clear enough when asking for clarifications¹, but I
>> think this change shouldn’t happen: first because it’s an incompatible
>> change that will break user configs, and second because it’s
>> inconsistent with other similar interfaces (such as ‘authorized-keys’
>> and <openssh-configuration>).
>> 
>> For these reasons, I’m in favor of reverting this change.
>> 
>> What do others think?
>
> This patch also added home-environment? without adding an import of
> (gnu home).
>
> It's unfortunate that the wording for the manual says 'pair' when it's a
> list, but IMO that's more of a typo in the manual than a mistake in the
> code.
>
> With a quick look I didn't see in any of my OS configs configurations
> with pair notations, even with simple-service or extra-special-file,
> where it would have been most likely.
>
> I think it would be best to roll this back.

ok, reverted.

>
>> Aside, it’s unfortunate that you weren’t around to review this patch
>> initially, despite being one of the recipients:
>> <https://issues.guix.gnu.org/69781>.  I think it’s important to not give
>> the impression that you chime in just when an rde incompatibility comes
>> up.
>> 
>> Thanks,
>> Ludo’.
>> 
>> ¹ https://issues.guix.gnu.org/71111#8
>> 
>> 
>> 

-- 
Best regards,
Andrew Tropin

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

Previous Next

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