diff mbox series

[bug#50967,13/14] home: services: configuration: Support file-like objects.

Message ID 20211002163834.29583-13-go.wigust@gmail.com
State Accepted
Headers show
Series [bug#50967,01/14] tests: Add tests for guix home cli. | expand

Commit Message

Oleg Pykhalov Oct. 2, 2021, 4:38 p.m. UTC 
* gnu/home/services/configuration.scm (interpose): Include content of files.
(string-or-gexp?): Rename to 'file-or-string-or-gexp?' and check for file-like
object.
(serialize-string-or-gexp): Rename to 'serialize-file-or-string-or-gexp'.
(text-config?): Call 'file-or-string-or-gexp?' intead of 'string-or-gexp?'.
* guix/scripts/home/import.scm:
(generate-bash-module+configuration): Don't call slurp-file-gexp.
---
 gnu/home/services/configuration.scm | 14 ++++++++++----
 guix/scripts/home/import.scm        |  8 +++-----
 2 files changed, 13 insertions(+), 9 deletions(-)

Comments

Xinglu Chen Oct. 2, 2021, 6:35 p.m. UTC | #1 
On Sat, Oct 02 2021, Oleg Pykhalov wrote:

> * gnu/home/services/configuration.scm (interpose): Include content of files.
> (string-or-gexp?): Rename to 'file-or-string-or-gexp?' and check for file-like
> object.

I would call it ‘file-like-or-string-or-gexp?’, just ‘files’ doesn’t
really make it clear that it should be a “file-like object”.

> (serialize-string-or-gexp): Rename to 'serialize-file-or-string-or-gexp'.
> (text-config?): Call 'file-or-string-or-gexp?' intead of 'string-or-gexp?'.
> * guix/scripts/home/import.scm:
> (generate-bash-module+configuration): Don't call slurp-file-gexp.
> ---
>  gnu/home/services/configuration.scm | 14 ++++++++++----
>  guix/scripts/home/import.scm        |  8 +++-----
>  2 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/gnu/home/services/configuration.scm b/gnu/home/services/configuration.scm
> index 5e7743e7d6..39db7a5693 100644
> --- a/gnu/home/services/configuration.scm
> +++ b/gnu/home/services/configuration.scm
> @@ -59,7 +59,12 @@ DELIMITER interposed LS.  Support 'infix and 'suffix GRAMMAR values."
>        (G_ "The GRAMMAR value must be 'infix or 'suffix, but ~a provided.")
>        grammar)))
>    (fold-right (lambda (e acc)
> -		(cons e
> +		(cons (if (file-like? e)
> +                          #~(begin
> +                              (use-modules (ice-9 rdelim))
> +                              (with-fluids ((%default-port-encoding "UTF-8"))
> +                                (with-input-from-file #$e read-string)))
> +                          e)
>  		      (if (and (null? acc) (eq? grammar 'infix))
>  			  acc
>  			  (cons delimiter acc))))
> @@ -79,11 +84,12 @@ the list result in @code{#t} when applying PRED? on them."
>  
>  (define alist? list?)
>  
> -(define (string-or-gexp? sg) (or (string? sg) (gexp? sg)))
> -(define (serialize-string-or-gexp field-name val) "")
> +(define (file-or-string-or-gexp? fsg)
> +  (or (string? fsg) (gexp? fsg) (file-like? fsg)))
> +(define (serialize-file-or-string-or-gexp field-name val) "")

This could defined as ‘empty-serializer’ from (gnu services
configuration).

>  
>  (define (text-config? config)
> -  (and (list? config) (every string-or-gexp? config)))
> +  (and (list? config) (every file-or-string-or-gexp? config)))

‘text-config?’ could be defined as (list-of file-or-string-or-gexp?)

>  (define (serialize-text-config field-name val)
>    #~(string-append #$@(interpose val "\n" 'suffix)))
>  
> diff --git a/guix/scripts/home/import.scm b/guix/scripts/home/import.scm
> index c977ec3861..611f580e85 100644
> --- a/guix/scripts/home/import.scm
> +++ b/guix/scripts/home/import.scm
> @@ -46,17 +46,15 @@
>                   (home-bash-configuration
>                    ,@(if (file-exists? rc)
>                          `((bashrc
> -                           (list (slurp-file-gexp (local-file ,rc)))))
> +                           (list (local-file ,rc))))
>                          '())
>                    ,@(if (file-exists? profile)
>                          `((bash-profile
> -                           (list (slurp-file-gexp
> -                                  (local-file ,profile)))))
> +                           (list (local-file ,profile))))
>                          '())
>                    ,@(if (file-exists? logout)
>                          `((bash-logout
> -                           (list (slurp-file-gexp
> -                                  (local-file ,logout)))))
> +                           (list (local-file ,logout))))
>                          '()))))))
>  
>  
> -- 
> 2.33.0
Ludovic Courtès Oct. 4, 2021, 2:04 p.m. UTC | #2 
Xinglu Chen <public@yoctocell.xyz> skribis:

> On Sat, Oct 02 2021, Oleg Pykhalov wrote:
>
>> * gnu/home/services/configuration.scm (interpose): Include content of files.
>> (string-or-gexp?): Rename to 'file-or-string-or-gexp?' and check for file-like
>> object.
>
> I would call it ‘file-like-or-string-or-gexp?’, just ‘files’ doesn’t
> really make it clear that it should be a “file-like object”.

As a matter of API, I would make it monomorphic: accept a file-like
object, period.  This is what’s done for System services (and
polymorphic APIs are rare in general in Guix).

‘plain-file’ and ‘scheme-file’ allow users to “convert” a string or a
gexp into a file-like object.

WDYT?

Ludo’.
Andrew Tropin Oct. 6, 2021, 8:15 a.m. UTC | #3 
On 2021-10-04 16:04, Ludovic Courtès wrote:

> Xinglu Chen <public@yoctocell.xyz> skribis:
>
>> On Sat, Oct 02 2021, Oleg Pykhalov wrote:
>>
>>> * gnu/home/services/configuration.scm (interpose): Include content of files.
>>> (string-or-gexp?): Rename to 'file-or-string-or-gexp?' and check for file-like
>>> object.
>>
>> I would call it ‘file-like-or-string-or-gexp?’, just ‘files’ doesn’t
>> really make it clear that it should be a “file-like object”.
>
> As a matter of API, I would make it monomorphic: accept a file-like
> object, period.  This is what’s done for System services (and
> polymorphic APIs are rare in general in Guix).

At least some of system services are far from ideal, recently I tried to
add rtmp section to nginx configuration using nginx system service.  I
had two options and both does look hacky, I could use extra-content
starting with closing curly bracket:

--8<---------------cut here---------------start------------->8---
(service nginx-service-type
         (nginx-configuration
          (modules
           (list
            (file-append nginx-rtmp-module "\
/etc/nginx/modules/ngx_rtmp_module.so")))
          (extra-content
           (format #f "\
}
rtmp {
        server {
                listen 1935;
                chunk_size 4096;

                application live {
                        live on;
                        record off;
                        push rtmp://a.rtmp.youtube.com/live2/~a;
                        push rtmp://diode.zone:1935/live/~a;
                }
        }
" youtube-key peertube-key))

          (server-blocks
           (list (nginx-server-configuration
                  ;; (locations
                  ;;  (list
                  ;;   (nginx-location-configuration
                  ;;    (uri "/stat")
                  ;;    (body '("rtmp_stat all;"
                  ;;            "rtmp_stat_stylesheet stat.xsl;")))))
                  (server-name `(,ip))
                  (listen '("8088"))
                  (root "/var/www/"))))))
--8<---------------cut here---------------end--------------->8---

or use file field of nginx-configuration record and generate the whole
configuration myself inside computed-file, loosing all the benifits of
other nginx-configuration fields.

Personally, I don't find both of these approaches appealing and
convenient.  Maybe it's an issue of exact system service, but the way
the configuration for this service is implemented is getting in the way
of the user.

>
> ‘plain-file’ and ‘scheme-file’ allow users to “convert” a string or a
> gexp into a file-like object.
>
> WDYT?
>
> Ludo’.
>
>
>

Imagine the following use case: I want to create a home service, which
accepts a package (zsh plugin) and adds a code for loading this package
to zshrc, currently it's implemented like that:

https://git.sr.ht/~abcdw/rde/tree/69dd2baf0384c899a4a4f97bdac8bf0b6e499b82/item/gnu/home-services/shellutils.scm#L18

Exteding the service above with `(list zsh-autosuggestions)` will add
the following line to zshrc:

source /gnu/store/w7d43gk1qszplj9i0rkzqvzz6kp88qfm-zsh-autosuggestions-0.7.0/share/zsh/plugins/zsh-autosuggestions/zsh-autosuggestions.zsh

Or the same thing can be done manually by user:

--8<---------------cut here---------------start------------->8---
(service
 home-zsh-service-type
 (home-zsh-configuration
  (zshrc
   (list
    #~(string-append "source " #$zsh-autosuggestions "/share/zs../..ions.zsh")
    ;; or
    "source \\"
    (file-append zsh-autosuggestions "/share/zs../..ions.zsh")))))
--8<---------------cut here---------------end--------------->8---

gexps returns a string, file-like object returns a path to the file in
the store, kinda expected behavior.  Both implementations looks quite
simple.


Now I'll try to reimplement it with file-like objects.  The code below
is a pseudo code, but should demonstrate the overall concerns I have:

--8<---------------cut here---------------start------------->8---
;; Some generic functions
(define get-file-like-object-path (file-like)
  "Because all file-like object get inserted literally by home services,
we need a function, which returns a file, which contains a path to the
file."
  (computed-file
   "tmp-file"
   #~#$file-like))

(define fl-append-strings (lst)
  "Accepts a list of strings and file-like object, reads the content of
the file-like objects (to be consistent with behavior of home services
configuration)."
  (define file-like->str (mb-file-like)
    (if (string? mb-file-like)
        mb-file-like
        #~(begin
            (use-modules (ice-9 rdelim))
            (with-fluids ((%default-port-encoding "UTF-8"))
              (with-input-from-file #$mb-file-like read-string)))))
  (computed-file
   "tmp-file"
   #~(apply string-append '#$(map file-like->str lst))))


;; A home service, declared in home-environment.
(service
 home-zsh-service-type
 (home-zsh-configuration
  (zshrc
   (list
    (fl-append-strings
     (list
      "source "
      (get-file-like-object-path zsh-autosuggestions)
      "/share/zs../..ions.zsh"))
    ;; or
    "source \\"
    (get-file-like-object-path
     (file-append zsh-autosuggestions "/share/zs../..ions.zsh"))))))
--8<---------------cut here---------------end--------------->8---

Here we don't use gexps inside configuration and all file-like objects
are "expanded" as their content instead of path in the store.

It can work, but looks a little strange and hard to copmose.  Perhaps, I
miss something and doesn't see the whole picture, but for now expanding
file-like objects to their content and throwing out gexps doesn't look
appealing to me.

BTW, I've skimmed through the paper "Code Staging in GNU Guix" and
limitations section, still not sure what your concerns are, I'll try to
re-read the paper and your message <87pmvqckws.fsf@gnu.org> one more
time a few days later to better understand it.  If you have a spare
time, please make a simple code snippet, which demonstrates the problem
you've mentioned in the message, which will hit the users of home
services.  It will help me to figure out possible shortcommings and
better understand the problem.

Ludovic, sorry for spending your time on that, but I really need to
understand this thing better.  Thank you in advance for hepling on that.

Oleg, thank you for working on this patch series, much appreciate)
Ludovic Courtès Oct. 8, 2021, 7:56 a.m. UTC | #4 
Hi Andrew,

Andrew Tropin <andrew@trop.in> skribis:

> On 2021-10-04 16:04, Ludovic Courtès wrote:
>
>> Xinglu Chen <public@yoctocell.xyz> skribis:
>>
>>> On Sat, Oct 02 2021, Oleg Pykhalov wrote:
>>>
>>>> * gnu/home/services/configuration.scm (interpose): Include content of files.
>>>> (string-or-gexp?): Rename to 'file-or-string-or-gexp?' and check for file-like
>>>> object.
>>>
>>> I would call it ‘file-like-or-string-or-gexp?’, just ‘files’ doesn’t
>>> really make it clear that it should be a “file-like object”.
>>
>> As a matter of API, I would make it monomorphic: accept a file-like
>> object, period.  This is what’s done for System services (and
>> polymorphic APIs are rare in general in Guix).
>
> At least some of system services are far from ideal, recently I tried to
> add rtmp section to nginx configuration using nginx system service.

I agree that nginx config is problematic:

  https://issues.guix.gnu.org/37388

But IMO that’s off-topic.  :-)

> Imagine the following use case: I want to create a home service, which
> accepts a package (zsh plugin) and adds a code for loading this package
> to zshrc, currently it's implemented like that:
>
> https://git.sr.ht/~abcdw/rde/tree/69dd2baf0384c899a4a4f97bdac8bf0b6e499b82/item/gnu/home-services/shellutils.scm#L18
>
> Exteding the service above with `(list zsh-autosuggestions)` will add
> the following line to zshrc:
>
> source /gnu/store/w7d43gk1qszplj9i0rkzqvzz6kp88qfm-zsh-autosuggestions-0.7.0/share/zsh/plugins/zsh-autosuggestions/zsh-autosuggestions.zsh

OK.

Then that’s fine: you can have special code that emits those “source”
lines in .zshrc while still allowing users to provide their own
file-like object for .zshrc lines they want to add.  Again, see how
‘torrc’ is generated in ‘tor-service-type’.

I’m happy to discuss specific service examples in mode details if you
want.  Overall, I’m confident Home services don’t require any pattern
that’s not already found in one of the many System services.  :-)

HTH,
Ludo’.
Andrew Tropin Oct. 8, 2021, 10 a.m. UTC | #5 
On 2021-10-08 09:56, Ludovic Courtès wrote:

> Hi Andrew,
>
> Andrew Tropin <andrew@trop.in> skribis:
>
>> On 2021-10-04 16:04, Ludovic Courtès wrote:
>>
>>> Xinglu Chen <public@yoctocell.xyz> skribis:
>>>
>>>> On Sat, Oct 02 2021, Oleg Pykhalov wrote:
>>>>
>>>>> * gnu/home/services/configuration.scm (interpose): Include content of files.
>>>>> (string-or-gexp?): Rename to 'file-or-string-or-gexp?' and check for file-like
>>>>> object.
>>>>
>>>> I would call it ‘file-like-or-string-or-gexp?’, just ‘files’ doesn’t
>>>> really make it clear that it should be a “file-like object”.
>>>
>>> As a matter of API, I would make it monomorphic: accept a file-like
>>> object, period.  This is what’s done for System services (and
>>> polymorphic APIs are rare in general in Guix).
>>
>> At least some of system services are far from ideal, recently I tried to
>> add rtmp section to nginx configuration using nginx system service.
>
> I agree that nginx config is problematic:
>
>   https://issues.guix.gnu.org/37388
>
> But IMO that’s off-topic.  :-)
>

Even not taking into account that ngixn-configuration is not ideally
implemented, probably it still relevant:

1. Highlights the problem of current service configuration
implementation approach with records: we will need to mimic the whole
underlying configuration language with records, for some small languages
it's possible, but in general it's very thankless job, which also force
use to keep an eye on upstream configuration language, possible
extensions to it and keep our implementation in sync.  In addition to
that in most cases provided set of nested records requires destructuring
and custom serialization code.

Other system services also affected by this issue.

2. Usage of separate config-file fields with file-like object values is
problematic:

For nginx service it substitutes the whole file, for tor service it will
be inserted at the end of the file for service X it will be inserted at
the middle.


Using a slightly different approach, implemented in many home services
we get ultimate configuration flexibility, we don't need to handle
existing file as some special case, create a separate field for it and
wonder where it should be inserted, when provided.

I'll provide more details in a separate thread, but for now just take a
look at too implementations of sway services using both approaches:

https://notabug.org/jbranso/guix-config/src/master/sway-service.scm#L110
https://git.sr.ht/~abcdw/rde/tree/master/item/gnu/home-services/wm.scm#L33

>> Imagine the following use case: I want to create a home service, which
>> accepts a package (zsh plugin) and adds a code for loading this package
>> to zshrc, currently it's implemented like that:
>>
>> https://git.sr.ht/~abcdw/rde/tree/69dd2baf0384c899a4a4f97bdac8bf0b6e499b82/item/gnu/home-services/shellutils.scm#L18
>>
>> Exteding the service above with `(list zsh-autosuggestions)` will add
>> the following line to zshrc:
>>
>> source /gnu/store/w7d43gk1qszplj9i0rkzqvzz6kp88qfm-zsh-autosuggestions-0.7.0/share/zsh/plugins/zsh-autosuggestions/zsh-autosuggestions.zsh
>
> OK.
>
> Then that’s fine: you can have special code that emits those “source”
> lines in .zshrc while still allowing users to provide their own
> file-like object for .zshrc lines they want to add.  Again, see how
> ‘torrc’ is generated in ‘tor-service-type’.

From what I understand you want a separate bashrc-file,
bash-profile-file and bash-logout-file fields to be present, but where
the content of those files should be inserted?  At the beginning/end or
instead of the rest of configuration?

We already can achieve the same result by providing gexp, which will
read the content of the file-like object, a very small helper for that
(slurp-file-gexp) makes it even easier to do.  Such approach is more
flexible and doesn't seem much harder.

I remember that you had concerns about slurp-file-gexp, but still don't
understand what exactly you are concerned about.

>
> I’m happy to discuss specific service examples in mode details if you
> want.  Overall, I’m confident Home services don’t require any pattern
> that’s not already found in one of the many System services.  :-)
>
> HTH,
> Ludo’.

I'll write a few examples of service configurations and rationale behind
the design descisions for that next week, to make the discussion more
practically oriented.
Andrew Tropin Oct. 8, 2021, 10:06 a.m. UTC | #6 
On 2021-10-02 19:38, Oleg Pykhalov wrote:

> * gnu/home/services/configuration.scm (interpose): Include content of files.
> (string-or-gexp?): Rename to 'file-or-string-or-gexp?' and check for file-like
> object.
> (serialize-string-or-gexp): Rename to 'serialize-file-or-string-or-gexp'.
> (text-config?): Call 'file-or-string-or-gexp?' intead of 'string-or-gexp?'.
> * guix/scripts/home/import.scm:
> (generate-bash-module+configuration): Don't call slurp-file-gexp.
> ---
>  gnu/home/services/configuration.scm | 14 ++++++++++----
>  guix/scripts/home/import.scm        |  8 +++-----
>  2 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/gnu/home/services/configuration.scm b/gnu/home/services/configuration.scm
> index 5e7743e7d6..39db7a5693 100644
> --- a/gnu/home/services/configuration.scm
> +++ b/gnu/home/services/configuration.scm
> @@ -59,7 +59,12 @@ DELIMITER interposed LS.  Support 'infix and 'suffix GRAMMAR values."
>        (G_ "The GRAMMAR value must be 'infix or 'suffix, but ~a provided.")
>        grammar)))
>    (fold-right (lambda (e acc)
> -		(cons e
> +		(cons (if (file-like? e)
> +                          #~(begin
> +                              (use-modules (ice-9 rdelim))
> +                              (with-fluids ((%default-port-encoding "UTF-8"))
> +                                (with-input-from-file #$e read-string)))
> +                          e)
>  		      (if (and (null? acc) (eq? grammar 'infix))
>  			  acc
>  			  (cons delimiter acc))))
> @@ -79,11 +84,12 @@ the list result in @code{#t} when applying PRED? on them."
>  
>  (define alist? list?)
>  
> -(define (string-or-gexp? sg) (or (string? sg) (gexp? sg)))
> -(define (serialize-string-or-gexp field-name val) "")
> +(define (file-or-string-or-gexp? fsg)
> +  (or (string? fsg) (gexp? fsg) (file-like? fsg)))
> +(define (serialize-file-or-string-or-gexp field-name val) "")
>  
>  (define (text-config? config)
> -  (and (list? config) (every string-or-gexp? config)))
> +  (and (list? config) (every file-or-string-or-gexp? config)))
>  (define (serialize-text-config field-name val)
>    #~(string-append #$@(interpose val "\n" 'suffix)))
>  
> diff --git a/guix/scripts/home/import.scm b/guix/scripts/home/import.scm
> index c977ec3861..611f580e85 100644
> --- a/guix/scripts/home/import.scm
> +++ b/guix/scripts/home/import.scm
> @@ -46,17 +46,15 @@
>                   (home-bash-configuration
>                    ,@(if (file-exists? rc)
>                          `((bashrc
> -                           (list (slurp-file-gexp (local-file ,rc)))))
> +                           (list (local-file ,rc))))
>                          '())
>                    ,@(if (file-exists? profile)
>                          `((bash-profile
> -                           (list (slurp-file-gexp
> -                                  (local-file ,profile)))))
> +                           (list (local-file ,profile))))
>                          '())
>                    ,@(if (file-exists? logout)
>                          `((bash-logout
> -                           (list (slurp-file-gexp
> -                                  (local-file ,logout)))))
> +                           (list (local-file ,logout))))
>                          '()))))))

I think this patch requires more discussion and better to keep it
outside of this patch series.  Skimmed throught other patches, overall
LGTM.
Xinglu Chen Oct. 8, 2021, 1:45 p.m. UTC | #7 
On Wed, Oct 06 2021, Andrew Tropin wrote:

> Imagine the following use case: I want to create a home service, which
> accepts a package (zsh plugin) and adds a code for loading this package
> to zshrc, currently it's implemented like that:
>
> https://git.sr.ht/~abcdw/rde/tree/69dd2baf0384c899a4a4f97bdac8bf0b6e499b82/item/gnu/home-services/shellutils.scm#L18
>
> Exteding the service above with `(list zsh-autosuggestions)` will add
> the following line to zshrc:
>
> source /gnu/store/w7d43gk1qszplj9i0rkzqvzz6kp88qfm-zsh-autosuggestions-0.7.0/share/zsh/plugins/zsh-autosuggestions/zsh-autosuggestions.zsh
>
> Or the same thing can be done manually by user:
>
> --8<---------------cut here---------------start------------->8---
> (service
>  home-zsh-service-type
>  (home-zsh-configuration
>   (zshrc
>    (list
>     #~(string-append "source " #$zsh-autosuggestions "/share/zs../..ions.zsh")
>     ;; or
>     "source \\"
>     (file-append zsh-autosuggestions "/share/zs../..ions.zsh")))))
> --8<---------------cut here---------------end--------------->8---
>
> gexps returns a string, file-like object returns a path to the file in
> the store, kinda expected behavior.  Both implementations looks quite
> simple.
>
>
> Now I'll try to reimplement it with file-like objects.  The code below
> is a pseudo code, but should demonstrate the overall concerns I have:
>
> --8<---------------cut here---------------start------------->8---
> ;; Some generic functions
> (define get-file-like-object-path (file-like)
>   "Because all file-like object get inserted literally by home services,
> we need a function, which returns a file, which contains a path to the
> file."
>   (computed-file
>    "tmp-file"
>    #~#$file-like))
>
> (define fl-append-strings (lst)
>   "Accepts a list of strings and file-like object, reads the content of
> the file-like objects (to be consistent with behavior of home services
> configuration)."
>   (define file-like->str (mb-file-like)
>     (if (string? mb-file-like)
>         mb-file-like
>         #~(begin
>             (use-modules (ice-9 rdelim))
>             (with-fluids ((%default-port-encoding "UTF-8"))
>               (with-input-from-file #$mb-file-like read-string)))))
>   (computed-file
>    "tmp-file"
>    #~(apply string-append '#$(map file-like->str lst))))
>
>
> ;; A home service, declared in home-environment.
> (service
>  home-zsh-service-type
>  (home-zsh-configuration
>   (zshrc
>    (list
>     (fl-append-strings
>      (list
>       "source "
>       (get-file-like-object-path zsh-autosuggestions)
>       "/share/zs../..ions.zsh"))
>     ;; or
>     "source \\"
>     (get-file-like-object-path
>      (file-append zsh-autosuggestions "/share/zs../..ions.zsh"))))))
> --8<---------------cut here---------------end--------------->8---

Wouldn’t something like the following work

--8<---------------cut here---------------start------------->8---
(service home-zsh-service-type
         (home-zsh-configuration
          (zshrc
           (list (mixed-text-file
                  "zshrc"
                  "source "
                  (file-append zsh-autosuggestions "/share/zsh/..."))
                 (local-file "./some-zshrc")))))
--8<---------------cut here---------------end--------------->8---

and since ‘zshrc’ is already a list of file-like objects, we could
implement ‘serialize-text-config’ using something like
‘fl-append-strings’, which would read the contents of the two files and
append them.  That way users don’t have to deal with ‘fl-append-strings’
or ‘slurp-file-gexp’.
Andrew Tropin Oct. 8, 2021, 2:34 p.m. UTC | #8 
On 2021-10-08 15:45, Xinglu Chen wrote:

> On Wed, Oct 06 2021, Andrew Tropin wrote:
>
>> Imagine the following use case: I want to create a home service, which
>> accepts a package (zsh plugin) and adds a code for loading this package
>> to zshrc, currently it's implemented like that:
>>
>> https://git.sr.ht/~abcdw/rde/tree/69dd2baf0384c899a4a4f97bdac8bf0b6e499b82/item/gnu/home-services/shellutils.scm#L18
>>
>> Exteding the service above with `(list zsh-autosuggestions)` will add
>> the following line to zshrc:
>>
>> source /gnu/store/w7d43gk1qszplj9i0rkzqvzz6kp88qfm-zsh-autosuggestions-0.7.0/share/zsh/plugins/zsh-autosuggestions/zsh-autosuggestions.zsh
>>
>> Or the same thing can be done manually by user:
>>
>> --8<---------------cut here---------------start------------->8---
>> (service
>>  home-zsh-service-type
>>  (home-zsh-configuration
>>   (zshrc
>>    (list
>>     #~(string-append "source " #$zsh-autosuggestions "/share/zs../..ions.zsh")
>>     ;; or
>>     "source \\"
>>     (file-append zsh-autosuggestions "/share/zs../..ions.zsh")))))
>> --8<---------------cut here---------------end--------------->8---
>>
>> gexps returns a string, file-like object returns a path to the file in
>> the store, kinda expected behavior.  Both implementations looks quite
>> simple.
>>
>>
>> Now I'll try to reimplement it with file-like objects.  The code below
>> is a pseudo code, but should demonstrate the overall concerns I have:
>>
>> --8<---------------cut here---------------start------------->8---
>> ;; Some generic functions
>> (define get-file-like-object-path (file-like)
>>   "Because all file-like object get inserted literally by home services,
>> we need a function, which returns a file, which contains a path to the
>> file."
>>   (computed-file
>>    "tmp-file"
>>    #~#$file-like))
>>
>> (define fl-append-strings (lst)
>>   "Accepts a list of strings and file-like object, reads the content of
>> the file-like objects (to be consistent with behavior of home services
>> configuration)."
>>   (define file-like->str (mb-file-like)
>>     (if (string? mb-file-like)
>>         mb-file-like
>>         #~(begin
>>             (use-modules (ice-9 rdelim))
>>             (with-fluids ((%default-port-encoding "UTF-8"))
>>               (with-input-from-file #$mb-file-like read-string)))))
>>   (computed-file
>>    "tmp-file"
>>    #~(apply string-append '#$(map file-like->str lst))))
>>
>>
>> ;; A home service, declared in home-environment.
>> (service
>>  home-zsh-service-type
>>  (home-zsh-configuration
>>   (zshrc
>>    (list
>>     (fl-append-strings
>>      (list
>>       "source "
>>       (get-file-like-object-path zsh-autosuggestions)
>>       "/share/zs../..ions.zsh"))
>>     ;; or
>>     "source \\"
>>     (get-file-like-object-path
>>      (file-append zsh-autosuggestions "/share/zs../..ions.zsh"))))))
>> --8<---------------cut here---------------end--------------->8---
>
> Wouldn’t something like the following work
>
> --8<---------------cut here---------------start------------->8---
> (service home-zsh-service-type
>          (home-zsh-configuration
>           (zshrc
>            (list (mixed-text-file
>                   "zshrc"
>                   "source "
>                   (file-append zsh-autosuggestions "/share/zsh/..."))
                     ^ place1
>                  (local-file "./some-zshrc")))))
                    ^ place2
> --8<---------------cut here---------------end--------------->8---
>
> and since ‘zshrc’ is already a list of file-like objects, we could
> implement ‘serialize-text-config’ using something like
> ‘fl-append-strings’, which would read the contents of the two files and
> append them.  That way users don’t have to deal with ‘fl-append-strings’
> or ‘slurp-file-gexp’.

Yep, it looks much better than what I was trying to prototype.

Still feels inconsistent that file-like object in place1 will be
evaluated to the path in the store, but in place2 to the content of the
file.
Ludovic Courtès Oct. 9, 2021, 1:34 p.m. UTC | #9 
Hi Andrew,

Preamble: Guix Home is now committed and there are bug reports coming
in.  To me, that means our discussion needs to be focused on addressing
specific issues; we’re not going to redesign Guix services in this
thread.

Andrew Tropin <andrew@trop.in> skribis:

[...]

>> Then that’s fine: you can have special code that emits those “source”
>> lines in .zshrc while still allowing users to provide their own
>> file-like object for .zshrc lines they want to add.  Again, see how
>> ‘torrc’ is generated in ‘tor-service-type’.
>
> From what I understand you want a separate bashrc-file,
> bash-profile-file and bash-logout-file fields to be present, but where
> the content of those files should be inserted?  At the beginning/end or
> instead of the rest of configuration?
>
> We already can achieve the same result by providing gexp, which will
> read the content of the file-like object, a very small helper for that
> (slurp-file-gexp) makes it even easier to do.  Such approach is more
> flexible and doesn't seem much harder.
>
> I remember that you had concerns about slurp-file-gexp, but still don't
> understand what exactly you are concerned about.

Let me restate my concerns:

  1. Users are unlikely to fathom what this does, given the name.  It
     will end up in user configurations, yet: “slurp”?  “gexp”?

  2. ‘slurp-file-gexp’ returns code, as a gexp.  Depending on the place
     where that gexp is inserted, it may or may not work.  Consider:

       (define (foo x)
         #~(frob '(#$x)))

       (foo (slurp-file-gexp …))  ;d’oh!

  3. Use of ‘slurp-file-gexp’ and gexps in configuration records is not
     consistent with the rest of the service APIs (and I think we can
     humbly recognize that those APIs have been doing the job for a
     while already.)

Let’s just to the (call-with-input-file file get-string-all) dance in
places where it’s needed rather than let users call ‘slurp-file-gexp’.
This is roughly what the ‘tor-service-type’ example I gave does.

> I'll write a few examples of service configurations and rationale behind
> the design descisions for that next week, to make the discussion more
> practically oriented.

People are starting to use the tool and to report bugs.  So,
unfortunately, we have to sort out interface issues quickly now.

If some of the things being discussed turn out to be too complex to
address under those time constraints, we can consider taking them out
until we have a better idea on how to address them.

How does that sound?

Thanks,
Ludo’.
Andrew Tropin Oct. 14, 2021, 7:08 a.m. UTC | #10 
On 2021-10-02 19:38, Oleg Pykhalov wrote:

> * gnu/home/services/configuration.scm (interpose): Include content of files.
> (string-or-gexp?): Rename to 'file-or-string-or-gexp?' and check for file-like
> object.
> (serialize-string-or-gexp): Rename to 'serialize-file-or-string-or-gexp'.
> (text-config?): Call 'file-or-string-or-gexp?' intead of 'string-or-gexp?'.
> * guix/scripts/home/import.scm:
> (generate-bash-module+configuration): Don't call slurp-file-gexp.
> ---
>  gnu/home/services/configuration.scm | 14 ++++++++++----
>  guix/scripts/home/import.scm        |  8 +++-----
>  2 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/gnu/home/services/configuration.scm b/gnu/home/services/configuration.scm
> index 5e7743e7d6..39db7a5693 100644
> --- a/gnu/home/services/configuration.scm
> +++ b/gnu/home/services/configuration.scm
> @@ -59,7 +59,12 @@ DELIMITER interposed LS.  Support 'infix and 'suffix GRAMMAR values."
>        (G_ "The GRAMMAR value must be 'infix or 'suffix, but ~a provided.")
>        grammar)))
>    (fold-right (lambda (e acc)
> -		(cons e
> +		(cons (if (file-like? e)
> +                          #~(begin
> +                              (use-modules (ice-9 rdelim))
> +                              (with-fluids ((%default-port-encoding "UTF-8"))
> +                                (with-input-from-file #$e read-string)))

This transformation should not be a part of interpose function,
interpose does know nothing about elements type and doesn't have to
know.  This addition is semantically incorrect and also contradictionary
to docstring.  It also breaks downstream channels.

The version of change in master is different, it doesn't even check
element type.

I'm strongly against this change.  If it necessary to make
transformation of elements of the list it should be done outside of
interpose.

> +                          e)
>  		      (if (and (null? acc) (eq? grammar 'infix))
>  			  acc
>  			  (cons delimiter acc))))
> @@ -79,11 +84,12 @@ the list result in @code{#t} when applying PRED? on them."
>  
>  (define alist? list?)
>  
> -(define (string-or-gexp? sg) (or (string? sg) (gexp? sg)))
> -(define (serialize-string-or-gexp field-name val) "")
> +(define (file-or-string-or-gexp? fsg)
> +  (or (string? fsg) (gexp? fsg) (file-like? fsg)))
> +(define (serialize-file-or-string-or-gexp field-name val) "")
>  
>  (define (text-config? config)
> -  (and (list? config) (every string-or-gexp? config)))
> +  (and (list? config) (every file-or-string-or-gexp? config)))
>  (define (serialize-text-config field-name val)
>    #~(string-append #$@(interpose val "\n" 'suffix)))
>  
> diff --git a/guix/scripts/home/import.scm b/guix/scripts/home/import.scm
> index c977ec3861..611f580e85 100644
> --- a/guix/scripts/home/import.scm
> +++ b/guix/scripts/home/import.scm
> @@ -46,17 +46,15 @@
>                   (home-bash-configuration
>                    ,@(if (file-exists? rc)
>                          `((bashrc
> -                           (list (slurp-file-gexp (local-file ,rc)))))
> +                           (list (local-file ,rc))))
>                          '())
>                    ,@(if (file-exists? profile)
>                          `((bash-profile
> -                           (list (slurp-file-gexp
> -                                  (local-file ,profile)))))
> +                           (list (local-file ,profile))))
>                          '())
>                    ,@(if (file-exists? logout)
>                          `((bash-logout
> -                           (list (slurp-file-gexp
> -                                  (local-file ,logout)))))
> +                           (list (local-file ,logout))))
>                          '()))))))
Andrew Tropin Oct. 14, 2021, 8:32 a.m. UTC | #11 
On 2021-10-09 15:34, Ludovic Courtès wrote:

> Hi Andrew,
>
> Preamble: Guix Home is now committed and there are bug reports coming
> in.  To me, that means our discussion needs to be focused on addressing
> specific issues; we’re not going to redesign Guix services in this
> thread.
>
> Andrew Tropin <andrew@trop.in> skribis:
>
> [...]
>
>>> Then that’s fine: you can have special code that emits those “source”
>>> lines in .zshrc while still allowing users to provide their own
>>> file-like object for .zshrc lines they want to add.  Again, see how
>>> ‘torrc’ is generated in ‘tor-service-type’.
>>
>> From what I understand you want a separate bashrc-file,
>> bash-profile-file and bash-logout-file fields to be present, but where
>> the content of those files should be inserted?  At the beginning/end or
>> instead of the rest of configuration?
>>
>> We already can achieve the same result by providing gexp, which will
>> read the content of the file-like object, a very small helper for that
>> (slurp-file-gexp) makes it even easier to do.  Such approach is more
>> flexible and doesn't seem much harder.
>>
>> I remember that you had concerns about slurp-file-gexp, but still don't
>> understand what exactly you are concerned about.
>
> Let me restate my concerns:
>
>   1. Users are unlikely to fathom what this does, given the name.  It
>      will end up in user configurations, yet: “slurp”?  “gexp”?

Sounds like a naming conecrn, I brought "slurp" word from clojure lang,
but we can call it whatever sounds better for guile community
generate-read-whole-file-gexp or anything else.  Actually, we don't even
need to have this wrapper to be present, just allow people to do
#~(call-with-input-file file-like-object get-string-all)

>
>   2. ‘slurp-file-gexp’ returns code, as a gexp.  Depending on the place
>      where that gexp is inserted, it may or may not work.  Consider:
>
>        (define (foo x)
>          #~(frob '(#$x)))
>
>        (foo (slurp-file-gexp …))  ;d’oh!

Yep, users can make mistakes, but it's not a technical problem, also, I
see it quite unlikely to happen according to my experience supporting
Guix Home users.

>
>   3. Use of ‘slurp-file-gexp’ and gexps in configuration records is not
>      consistent with the rest of the service APIs (and I think we can
>      humbly recognize that those APIs have been doing the job for a
>      while already.)

It's not actually true, there are system services, which accept a list
of strings/gexps and state it in the documentation, some of them do it
in a less exlicit way, but still do, I found that trick looking at
system service and made it an explicit pattern for home services.  I
tried to extract and follow patterns from system services, but found
them quite inconsistent, so I took a few of them and wrote a few on my
own.

To name a few:
extra-options @ alsa-configuration
extra-config @ nix-configuration
extra-config @ xorg-configuration
extra-content @ nginx-configuration
extra-config @ httpd-config-file
contents @ httpd-virtualhost

To make guix services configurations consistent (at least new ones) I
think it would be cool to have a `Writing Service Configuration
Guideline` section.  I can share the design descisions I've made for
home services configurations and after a discussion it can end up in a
manual section giving a clean guideline, reducing subjective preferences
and increasing consistency.  If you are interested.

>
> Let’s just to the (call-with-input-file file get-string-all) dance in
> places where it’s needed rather than let users call ‘slurp-file-gexp’.
> This is roughly what the ‘tor-service-type’ example I gave does.
>
>> I'll write a few examples of service configurations and rationale behind
>> the design descisions for that next week, to make the discussion more
>> practically oriented.
>
> People are starting to use the tool and to report bugs.  So,
> unfortunately, we have to sort out interface issues quickly now.
>
> If some of the things being discussed turn out to be too complex to
> address under those time constraints, we can consider taking them out
> until we have a better idea on how to address them.
>
> How does that sound?
>
> Thanks,
> Ludo’.
diff mbox series

Patch

diff --git a/gnu/home/services/configuration.scm b/gnu/home/services/configuration.scm
index 5e7743e7d6..39db7a5693 100644
--- a/gnu/home/services/configuration.scm
+++ b/gnu/home/services/configuration.scm
@@ -59,7 +59,12 @@  DELIMITER interposed LS.  Support 'infix and 'suffix GRAMMAR values."
       (G_ "The GRAMMAR value must be 'infix or 'suffix, but ~a provided.")
       grammar)))
   (fold-right (lambda (e acc)
-		(cons e
+		(cons (if (file-like? e)
+                          #~(begin
+                              (use-modules (ice-9 rdelim))
+                              (with-fluids ((%default-port-encoding "UTF-8"))
+                                (with-input-from-file #$e read-string)))
+                          e)
 		      (if (and (null? acc) (eq? grammar 'infix))
 			  acc
 			  (cons delimiter acc))))
@@ -79,11 +84,12 @@  the list result in @code{#t} when applying PRED? on them."
 
 (define alist? list?)
 
-(define (string-or-gexp? sg) (or (string? sg) (gexp? sg)))
-(define (serialize-string-or-gexp field-name val) "")
+(define (file-or-string-or-gexp? fsg)
+  (or (string? fsg) (gexp? fsg) (file-like? fsg)))
+(define (serialize-file-or-string-or-gexp field-name val) "")
 
 (define (text-config? config)
-  (and (list? config) (every string-or-gexp? config)))
+  (and (list? config) (every file-or-string-or-gexp? config)))
 (define (serialize-text-config field-name val)
   #~(string-append #$@(interpose val "\n" 'suffix)))
 
diff --git a/guix/scripts/home/import.scm b/guix/scripts/home/import.scm
index c977ec3861..611f580e85 100644
--- a/guix/scripts/home/import.scm
+++ b/guix/scripts/home/import.scm
@@ -46,17 +46,15 @@ 
                  (home-bash-configuration
                   ,@(if (file-exists? rc)
                         `((bashrc
-                           (list (slurp-file-gexp (local-file ,rc)))))
+                           (list (local-file ,rc))))
                         '())
                   ,@(if (file-exists? profile)
                         `((bash-profile
-                           (list (slurp-file-gexp
-                                  (local-file ,profile)))))
+                           (list (local-file ,profile))))
                         '())
                   ,@(if (file-exists? logout)
                         `((bash-logout
-                           (list (slurp-file-gexp
-                                  (local-file ,logout)))))
+                           (list (local-file ,logout))))
                         '()))))))