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

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

[Andrew Tropin via Guix-patches via]

After rewriting from car/cdr to match-lambda in v2 of this patch:
https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard@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

[bug#71112] [PATCH 1/1] services: home: Use pairs instead of lists.

[Andrew Tropin via Guix-patches via]

* 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

[bug#71111] [PATCH 1/1] services: home: Use pairs instead of lists.

[Andrew Tropin via Guix-patches via]

* 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

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

[Richard Sent]

Andrew Tropin <andrew@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@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.

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

[Andrew Tropin via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 3135 bytes --]

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

> Andrew Tropin <andrew@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@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Zheng Junjie]

[-- Attachment #1: Type: text/plain, Size: 1012 bytes --]

Andrew Tropin via Guix-patches via <guix-patches@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@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Andrew Tropin via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 1404 bytes --]

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

> Andrew Tropin via Guix-patches via <guix-patches@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@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Maxim Cournoyer]

Hi,

Andrew Tropin <andrew@trop.in> writes:

> On 2024-05-23 11:38, Zheng Junjie wrote:
>
>> Andrew Tropin via Guix-patches via <guix-patches@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@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

[bug#71111] [PATCH v2 0/1] services: home: Use pairs instead of lists.

[Andrew Tropin via Guix-patches via]

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

[bug#71111] [PATCH v2 1/1] services: home: Use pairs instead of lists.

[Andrew Tropin via Guix-patches via]

* 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

[bug#71111] [PATCH v2 1/1] services: home: Use pairs instead of lists.

[Ludovic Courtès]

Hi Andrew,

Andrew Tropin <andrew@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’.

[bug#71111] [PATCH v2 1/1] services: home: Use pairs instead of lists.

[Andrew Tropin via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 1330 bytes --]

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

> Hi Andrew,
>
> Andrew Tropin <andrew@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@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Andrew Tropin via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 1062 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Ludovic Courtès]

Hi Andrew,

Andrew Tropin <andrew@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@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

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

[Efraim Flashner]

[-- Attachment #1: Type: text/plain, Size: 2722 bytes --]

On Sun, Jun 02, 2024 at 12:15:14PM +0200, Ludovic Courtès wrote:
> Hi Andrew,
> 
> Andrew Tropin <andrew@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@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@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Andrew Tropin via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 2690 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Andrew Tropin via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 2507 bytes --]

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

> Hi Andrew,
>
> Andrew Tropin <andrew@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@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]