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

Done

Details

6 participants

Andrew Tropin
Efraim Flashner
Ludovic Courtès
Maxim Cournoyer
Richard Sent
Zheng Junjie

Owner: unassigned

Submitted by: Andrew Tropin

Severity: normal

Andrew Tropin wrote on 22 May 12:02 +0200

Recipients:(address . guix-patches@gnu.org)

Message-ID:cover.1716372146.git.andrew@trop.in

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

Andrew Tropin wrote on 22 May 13:06 +0200

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

Recipients:(address . guix-patches@gnu.org)

Message-ID:878r02f6ls.fsf@trop.in

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

Toggle diff (50 lines)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

Richard Sent wrote on 22 May 23:33 +0200

Re: [PATCH 0/1] services: home: Use pairs instead of lists.

Recipients:(name . Andrew Tropin)(address . andrew@trop.in)

Message-ID:87cypdwmyi.fsf@freakingpenguin.com

Andrew Tropin <andrew@trop.in> writes:

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

Toggle snippet (11 lines)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

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

following to match:

Toggle snippet (4 lines)

(simple-service 'my-extra-home home-service-type

`(("bob" "bill" "bertha" "billyanne" ,my-extra-home ,my-home-2)))

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?

Toggle snippet (14 lines)(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))

Maybe I'm just being silly. ?

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

Zheng Junjie wrote on 23 May 05:38 +0200

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

Recipients:(name . Andrew Tropin via Guix-patches via)(address . guix-patches@gnu.org)

Message-ID:87sey9rye0.fsf@iscas.ac.cn

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

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

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

Andrew Tropin wrote on 23 May 07:43 +0200

Recipients:

Message-ID:877cfl13sc.fsf@trop.in

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

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

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

Andrew Tropin wrote on 23 May 07:45 +0200

Re: [PATCH 0/1] services: home: Use pairs instead of lists.

Recipients:(name . Richard Sent)(address . richard@freakingpenguin.com)

Message-ID:874jap13q9.fsf@trop.in

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

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

Toggle quote (3 lines)

> Maybe I'm just being silly. ?

-- 
Best regards,
Andrew Tropin

Andrew Tropin wrote on 23 May 07:53 +0200

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

Recipients:(address . guix-patches@gnu.org)(name . Andrew Tropin)(address . andrew@trop.in)

Message-ID:cover.1716443624.git.andrew@trop.in

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

Andrew Tropin wrote on 23 May 07:53 +0200

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

Recipients:(address . guix-patches@gnu.org)

Message-ID:c89139f9177259e04e17df162eb6020a41b58294.1716443624.git.andrew@trop.in

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

Toggle diff (61 lines)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

Ludovic Courtès wrote on 23 May 11:16 +0200

Recipients:(name . Andrew Tropin)(address . andrew@trop.in)

Message-ID:87h6eoc2h4.fsf@gnu.org

Hi Andrew,

Andrew Tropin <andrew@trop.in> skribis:

Toggle quote (5 lines)

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

Andrew Tropin wrote on 23 May 15:06 +0200

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:87pltc65jr.fsf@trop.in

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

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

Toggle snippet (13 lines)

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.

Toggle quote (3 lines)

> 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

Maxim Cournoyer wrote on 23 May 18:02 +0200

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

Recipients:(name . Andrew Tropin)(address . andrew@trop.in)

Message-ID:874jaojz31.fsf@gmail.com

Hi,

Andrew Tropin <andrew@trop.in> writes:

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

Andrew Tropin wrote on 2 Jun 11:50 +0200

Recipients:

Message-ID:87ttib1xmz.fsf@trop.in

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

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

Ludovic Courtès wrote on 2 Jun 12:15 +0200

Recipients:(name . Andrew Tropin)(address . andrew@trop.in)

Message-ID:87ikyrlkgd.fsf@gnu.org

Hi Andrew,

Andrew Tropin <andrew@trop.in> skribis:

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

Efraim Flashner wrote on 2 Jun 12:37 +0200

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:ZlxLbtNpW1puijYR@3900XT

On Sun, Jun 02, 2024 at 12:15:14PM +0200, Ludovic Courtès wrote:

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

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

Andrew Tropin wrote on 2 Jun 12:57 +0200

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)

Message-ID:871q5f1ujr.fsf@trop.in

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

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

Toggle quote (3 lines)

> 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

Andrew Tropin wrote on 2 Jun 13:12 +0200

Recipients:

Message-ID:87y17nzjgu.fsf@trop.in

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

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

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

Your comment

This issue is archived.

To comment on this conversation send an email to 71111@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it

mumi current 71111

Then, you may apply the latest patchset in this issue (with sign off)

mumi am -- -s

Or, compose a reply to this issue

mumi compose

Or, send patches to this issue

mumi send-email *.patch

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date