[bug#53676] [PATCH 0/5] *** PulseAudio service improvements ***

[bug#53676] [PATCH 0/5] *** PulseAudio service improvements ***

[Maxim Cournoyer]

Hello Guix,

This small series adds an easy way to drop PulseAudio configuration scripts to
/etc/pulse/default.pa.d.  It also lifts the need to reboot the machine to have
a new PulseAudio configuration active.

Maxim Cournoyer (5):
  doc: Fix typo.
  services/sound: Normalize pulseaudio-configuration accessor names.
  gnu: pulseaudio: Graft to adjust configuration.
  services: pulseaudio: Add an extra-script-files configuration field.
  services: pulseaudio: Deploy the configuration files to /etc/pulse.

 doc/guix.texi               | 29 +++++++++++++++++++++-
 gnu/packages/pulseaudio.scm | 18 ++++++++++++++
 gnu/services/sound.scm      | 49 ++++++++++++++++++++++++++++---------
 3 files changed, 84 insertions(+), 12 deletions(-)

-- 
2.34.0

[bug#53676] [PATCH 1/5] doc: Fix typo.

[Maxim Cournoyer]

* doc/guix.texi (Sound Services): Fix typo.
---
 doc/guix.texi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 94f8e5e481..a002670030 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -21382,7 +21382,7 @@ Data type representing the configuration for @code{pulseaudio-service}.
 @table @asis
 @item @code{client-conf} (default: @code{'()})
 List of settings to set in @file{client.conf}.
-Accepts a list of strings or a symbol-value pairs.  A string will be
+Accepts a list of strings or symbol-value pairs.  A string will be
 inserted as-is with a newline added.  A pair will be formatted as
 ``key = value'', again with a newline added.
 
-- 
2.34.0

[bug#53676] [PATCH 2/5] services/sound: Normalize pulseaudio-configuration accessor names.

[Maxim Cournoyer]

* gnu/services/sound.scm (<pulseaudio-configuration>): Adjust getter names to
match convention.
---
 gnu/services/sound.scm | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
index 7beca35ffe..19eccfc860 100644
--- a/gnu/services/sound.scm
+++ b/gnu/services/sound.scm
@@ -2,6 +2,7 @@
 ;;; Copyright © 2018, 2020 Oleg Pykhalov <go.wigust@gmail.com>
 ;;; Copyright © 2020 Liliana Marie Prikler <liliana.prikler@gmail.com>
 ;;; Copyright © 2020 Marius Bakke <mbakke@fastmail.com>
+;;; Copyright © 2022 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -115,16 +116,16 @@ (define alsa-service-type
 (define-record-type* <pulseaudio-configuration>
   pulseaudio-configuration make-pulseaudio-configuration
   pulseaudio-configuration?
-  (client-conf pulseaudio-client-conf
+  (client-conf pulseaudio-configuration-client-conf
                (default '()))
-  (daemon-conf pulseaudio-daemon-conf
+  (daemon-conf pulseaudio-configuration-daemon-conf
                ;; Flat volumes may cause unpleasant experiences to users
                ;; when applications inadvertently max out the system volume
                ;; (see e.g. <https://bugs.gnu.org/38172>).
                (default '((flat-volumes . no))))
-  (script-file pulseaudio-script-file
+  (script-file pulseaudio-configuration-script-file
                (default (file-append pulseaudio "/etc/pulse/default.pa")))
-  (system-script-file pulseaudio-system-script-file
+  (system-script-file pulseaudio-configuration-system-script-file
                       (default
                         (file-append pulseaudio "/etc/pulse/system.pa"))))
 
-- 
2.34.0

[bug#53676] [PATCH 3/5] gnu: pulseaudio: Graft to adjust configuration.

[Maxim Cournoyer]

* gnu/packages/pulseaudio.scm (pulseaudio/fixed): New variable.
(pulseaudio)[replacement]: Graft package with it.
---
 gnu/packages/pulseaudio.scm | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/gnu/packages/pulseaudio.scm b/gnu/packages/pulseaudio.scm
index fe028b5202..f529717ee1 100644
--- a/gnu/packages/pulseaudio.scm
+++ b/gnu/packages/pulseaudio.scm
@@ -178,6 +178,7 @@ (define-public libsamplerate
 (define-public pulseaudio
   (package
     (name "pulseaudio")
+    (replacement pulseaudio/fixed)
     (version "15.0")
     (source (origin
              (method url-fetch)
@@ -269,6 +270,23 @@ (define-public pulseaudio
     ;; 'LICENSE' for details.
     (license l:gpl2+)))
 
+(define pulseaudio/fixed
+  (package
+    (inherit pulseaudio)
+    (arguments
+     (substitute-keyword-arguments (package-arguments pulseaudio)
+       ((#:phases phases)
+        `(modify-phases ,phases
+           (add-after 'unpack 'customize-default-script
+             (lambda _
+               (call-with-port
+                (open-file "src/daemon/default.pa.in" "a")
+                (lambda (port)
+                  (format port "~%\
+### Include extra script files configured via the pulseaudio-service-type.
+.nofail
+.include /etc/pulse/default.pa.d~%")))))))))))
+
 (define-public pavucontrol
   (package
     (name "pavucontrol")
-- 
2.34.0

[bug#53676] [PATCH 4/5] services: pulseaudio: Add an extra-script-files configuration field.

[Maxim Cournoyer]

* gnu/services/sound.scm (<pulseaudio-configuration>)
[extra-script-files]: Add field.
(extra-script-files->file-union): Add procedure.
(pulseaudio-etc): Use it.
* doc/guix.texi: Document it.
---
 doc/guix.texi          | 27 +++++++++++++++++++++++++++
 gnu/services/sound.scm | 19 +++++++++++++++++--
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index a002670030..2f8df03461 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -21393,9 +21393,36 @@ List of settings to set in @file{daemon.conf}, formatted just like
 @item @code{script-file} (default: @code{(file-append pulseaudio "/etc/pulse/default.pa")})
 Script file to use as @file{default.pa}.
 
+@item @code{extra-script-files} (default: @code{'())})
+A list of file-like objects defining extra PulseAudio scripts to run at
+the initialization of the @command{pulseaudio} daemon.  For a reference
+of the available commands, refer to @command{man pulse-cli-syntax}.
+
 @item @code{system-script-file} (default: @code{(file-append pulseaudio "/etc/pulse/system.pa")})
 Script file to use as @file{system.pa}.
 @end table
+
+The example below sets the default PulseAudio card profile, the default
+sink and the default source to use for a old SoundBlaster Audigy sound
+card:
+@lisp
+(pulseaudio-configuration
+ (extra-script-files
+  (list (plain-file "configure-audigy-card"
+                    (string-append "\
+set-card-profile alsa_card.pci-0000_01_01.0 \
+  output:analog-surround-40+input:analog-mono
+set-default-source alsa_input.pci-0000_01_01.0.analog-mono
+set-default-sink alsa_output.pci-0000_01_01.0.analog-surround-40\n")))))
+@end lisp
+
+Note that @code{pulseaudio-service-type} is part of
+@code{%desktop-services}; if your operating system declaration was
+derived from one of the desktop templates, you'll want to adjust the
+above example to modify the existing @code{pulseaudio-service-type} via
+@code{modify-services} (@pxref{Service Reference,
+@code{modify-services}}), instead of defining a new one.
+
 @end deftp
 
 @deffn {Scheme Variable} ladspa-service-type
diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
index 19eccfc860..f529188a7c 100644
--- a/gnu/services/sound.scm
+++ b/gnu/services/sound.scm
@@ -34,6 +34,7 @@ (define-module (gnu services sound)
   #:use-module (gnu packages linux)
   #:use-module (gnu packages pulseaudio)
   #:use-module (ice-9 match)
+  #:use-module (srfi srfi-1)
   #:export (alsa-configuration
             alsa-service-type
 
@@ -125,6 +126,8 @@ (define-record-type* <pulseaudio-configuration>
                (default '((flat-volumes . no))))
   (script-file pulseaudio-configuration-script-file
                (default (file-append pulseaudio "/etc/pulse/default.pa")))
+  (extra-script-files pulseaudio-configuration-extra-script-files
+                      (default '()))
   (system-script-file pulseaudio-configuration-system-script-file
                       (default
                         (file-append pulseaudio "/etc/pulse/system.pa"))))
@@ -145,14 +148,26 @@ (define pulseaudio-environment
        ("PULSE_CLIENTCONFIG" . ,(apply mixed-text-file "client.conf"
                                        (map pulseaudio-conf-entry client-conf)))))))
 
+(define (extra-script-files->file-union extra-script-files)
+  "Return a G-exp obtained by processing EXTRA-SCRIPT-FILES with FILE-UNION.
+Each file is named \"snippet-n.pa\", where N is their 1-offset index."
+  (let ((labels (map (lambda (n) (format #f "snippet-~a.pa" n))
+                     (iota (length extra-script-files) 1))))
+    (file-union "default.pa.d" (zip labels extra-script-files))))
+
 (define pulseaudio-etc
   (match-lambda
-    (($ <pulseaudio-configuration> _ _ default-script-file system-script-file)
+    (($ <pulseaudio-configuration> _ _ default-script-file extra-script-files
+                                   system-script-file)
      `(("pulse"
         ,(file-union
           "pulse"
           `(("default.pa" ,default-script-file)
-            ("system.pa" ,system-script-file))))))))
+            ("system.pa" ,system-script-file)
+            ,@(if (null? extra-script-files)
+                  '()
+                  `(("default.pa.d" ,(extra-script-files->file-union
+                                      extra-script-files)))))))))))
 
 (define pulseaudio-service-type
   (service-type
-- 
2.34.0

[bug#53676] [PATCH 5/5] services: pulseaudio: Deploy the configuration files to /etc/pulse.

[Maxim Cournoyer]

* gnu/services/sound.scm (pulseaudio-environment)
[PULSE_CONFIG, PULSE_CLIENTCONFIG]: Use fix locations, and move logic to...
(pulseaudio-etc): ... this service extension.  Guard against producing empty
files.
---
 gnu/services/sound.scm | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
index f529188a7c..0877788f06 100644
--- a/gnu/services/sound.scm
+++ b/gnu/services/sound.scm
@@ -142,11 +142,11 @@ (define (pulseaudio-conf-entry arg)
 (define pulseaudio-environment
   (match-lambda
     (($ <pulseaudio-configuration> client-conf daemon-conf default-script-file)
-     `(("PULSE_CONFIG" . ,(apply mixed-text-file "daemon.conf"
-                                 "default-script-file = " default-script-file "\n"
-                                 (map pulseaudio-conf-entry daemon-conf)))
-       ("PULSE_CLIENTCONFIG" . ,(apply mixed-text-file "client.conf"
-                                       (map pulseaudio-conf-entry client-conf)))))))
+     ;; These config files kept at a fixed location, so that the following
+     ;; environment values are stable and do not require the user to reboot to
+     ;; effect their PulseAudio configuration changes.
+     '(("PULSE_CONFIG" . "/etc/pulse/daemon.conf")
+       ("PULSE_CLIENTCONFIG" . "/etc/pulse/client.conf")))))
 
 (define (extra-script-files->file-union extra-script-files)
   "Return a G-exp obtained by processing EXTRA-SCRIPT-FILES with FILE-UNION.
@@ -157,8 +157,8 @@ (define (extra-script-files->file-union extra-script-files)
 
 (define pulseaudio-etc
   (match-lambda
-    (($ <pulseaudio-configuration> _ _ default-script-file extra-script-files
-                                   system-script-file)
+    (($ <pulseaudio-configuration> client-conf daemon-conf default-script-file
+                                   extra-script-files system-script-file)
      `(("pulse"
         ,(file-union
           "pulse"
@@ -167,7 +167,18 @@ (define pulseaudio-etc
             ,@(if (null? extra-script-files)
                   '()
                   `(("default.pa.d" ,(extra-script-files->file-union
-                                      extra-script-files)))))))))))
+                                      extra-script-files))))
+            ,@(if (null? daemon-conf)
+                  '()
+                  `(("daemon.conf"
+                     ,(apply mixed-text-file "daemon.conf"
+                             "default-script-file = " default-script-file "\n"
+                             (map pulseaudio-conf-entry daemon-conf)))))
+            ,@(if (null? client-conf)
+                  '()
+                  `(("client.conf"
+                     ,(apply mixed-text-file "client.conf"
+                             (map pulseaudio-conf-entry client-conf))))))))))))
 
 (define pulseaudio-service-type
   (service-type
-- 
2.34.0

[bug#53676] [PATCH 0/5] *** PulseAudio service improvements ***

[Leo Famulari]

On Mon, Jan 31, 2022 at 11:13:52PM -0500, Maxim Cournoyer wrote:
> This small series adds an easy way to drop PulseAudio configuration scripts to
> /etc/pulse/default.pa.d.  It also lifts the need to reboot the machine to have
> a new PulseAudio configuration active.

Nice! That will be a great improvement.

> Maxim Cournoyer (5):
>   doc: Fix typo.
>   services/sound: Normalize pulseaudio-configuration accessor names.
>   gnu: pulseaudio: Graft to adjust configuration.
>   services: pulseaudio: Add an extra-script-files configuration field.
>   services: pulseaudio: Deploy the configuration files to /etc/pulse.

I don't think we should use grafts for anything besides fixing very
serious bugs, although they are definitely useful to demonstrate new
features in a patch series.

Pulseaudio only has ~1700 dependents. We can easily handle it on a
"new-features" branch or similar.

[bug#53676] [PATCH 5/5] services: pulseaudio: Deploy the configuration files to /etc/pulse.

[Liliana Marie Prikler]

Hi,

Am Montag, dem 31.01.2022 um 23:19 -0500 schrieb Maxim Cournoyer:
> * gnu/services/sound.scm (pulseaudio-environment)
> [PULSE_CONFIG, PULSE_CLIENTCONFIG]: Use fix locations, and move logic
> to...
> (pulseaudio-etc): ... this service extension.  Guard against producing
> empty files.

This patch reproduces (more or less) the initial layout we had for
pulseaudio-service-type.  However, that layout has been reported to not
work with some sandboxes.  I tried tracking down a specific bug, but
could only gather <https://issues.guix.gnu.org/42118#3>.

> Due to a bug with webkit sandboxing, we no longer put daemon.conf
> into /etc/pulse (my bad), but rather set PULSE_CONFIG to directly
> point to it.

In other words, we should check whether Epiphany still plays sound
properly with this patch applied.

Cheers

[bug#53676] [PATCH 3/5] gnu: pulseaudio: Graft to adjust configuration.

[Liliana Marie Prikler]

Am Montag, dem 31.01.2022 um 23:19 -0500 schrieb Maxim Cournoyer:
> * gnu/packages/pulseaudio.scm (pulseaudio/fixed): New variable.
> (pulseaudio)[replacement]: Graft package with it.
> ---
>  gnu/packages/pulseaudio.scm | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/gnu/packages/pulseaudio.scm
> b/gnu/packages/pulseaudio.scm
> index fe028b5202..f529717ee1 100644
> --- a/gnu/packages/pulseaudio.scm
> +++ b/gnu/packages/pulseaudio.scm
> @@ -178,6 +178,7 @@ (define-public libsamplerate
>  (define-public pulseaudio
>    (package
>      (name "pulseaudio")
> +    (replacement pulseaudio/fixed)
>      (version "15.0")
>      (source (origin
>               (method url-fetch)
> @@ -269,6 +270,23 @@ (define-public pulseaudio
>      ;; 'LICENSE' for details.
>      (license l:gpl2+)))
>  
> +(define pulseaudio/fixed
> +  (package
> +    (inherit pulseaudio)
> +    (arguments
> +     (substitute-keyword-arguments (package-arguments pulseaudio)
> +       ((#:phases phases)
> +        `(modify-phases ,phases
> +           (add-after 'unpack 'customize-default-script
> +             (lambda _
> +               (call-with-port
> +                (open-file "src/daemon/default.pa.in" "a")
> +                (lambda (port)
> +                  (format port "~%\
> +### Include extra script files configured via the pulseaudio-
> service-type.
> +.nofail
> +.include /etc/pulse/default.pa.d~%")))))))))))
> +
Note that there should be a .fail afterwards.  

As Leo pointed out, we shouldn't do too many "feature grafts", so
instead it might be worth moving this part to pulseaudio-service-type
in some way, no?

[bug#53676] [PATCH 2/5] services/sound: Normalize pulseaudio-configuration accessor names.

[Liliana Marie Prikler]

Am Montag, dem 31.01.2022 um 23:19 -0500 schrieb Maxim Cournoyer:
> * gnu/services/sound.scm (<pulseaudio-configuration>): Adjust getter
> names to match convention.
> ---
>  gnu/services/sound.scm | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
> index 7beca35ffe..19eccfc860 100644
> --- a/gnu/services/sound.scm
> +++ b/gnu/services/sound.scm
> @@ -2,6 +2,7 @@
>  ;;; Copyright © 2018, 2020 Oleg Pykhalov <go.wigust@gmail.com>
>  ;;; Copyright © 2020 Liliana Marie Prikler
> <liliana.prikler@gmail.com>
>  ;;; Copyright © 2020 Marius Bakke <mbakke@fastmail.com>
> +;;; Copyright © 2022 Maxim Cournoyer <maxim.cournoyer@gmail.com>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -115,16 +116,16 @@ (define alsa-service-type
>  (define-record-type* <pulseaudio-configuration>
>    pulseaudio-configuration make-pulseaudio-configuration
>    pulseaudio-configuration?
> -  (client-conf pulseaudio-client-conf
> +  (client-conf pulseaudio-configuration-client-conf
>                 (default '()))
> -  (daemon-conf pulseaudio-daemon-conf
> +  (daemon-conf pulseaudio-configuration-daemon-conf
>                 ;; Flat volumes may cause unpleasant experiences to
> users
>                 ;; when applications inadvertently max out the system
> volume
>                 ;; (see e.g. <https://bugs.gnu.org/38172>).
>                 (default '((flat-volumes . no))))
> -  (script-file pulseaudio-script-file
> +  (script-file pulseaudio-configuration-script-file
>                 (default (file-append pulseaudio
> "/etc/pulse/default.pa")))
> -  (system-script-file pulseaudio-system-script-file
> +  (system-script-file pulseaudio-configuration-system-script-file
>                        (default
>                          (file-append pulseaudio
> "/etc/pulse/system.pa"))))
I don't see calling code adjusted anywhere.  Is this because we only
use match to access this records fields?

On a related note, would it make sense to port this over to (define-
configuration)?

[bug#53676] [PATCH 1/5] doc: Fix typo.

[Liliana Marie Prikler]

Am Montag, dem 31.01.2022 um 23:19 -0500 schrieb Maxim Cournoyer:
> * doc/guix.texi (Sound Services): Fix typo.
> ---
>  doc/guix.texi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 94f8e5e481..a002670030 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -21382,7 +21382,7 @@ Data type representing the configuration for
> @code{pulseaudio-service}.
>  @table @asis
>  @item @code{client-conf} (default: @code{'()})
>  List of settings to set in @file{client.conf}.
> -Accepts a list of strings or a symbol-value pairs.  A string will be
> +Accepts a list of strings or symbol-value pairs.  A string will be
>  inserted as-is with a newline added.  A pair will be formatted as
>  ``key = value'', again with a newline added.
LGTM.

[bug#53676] [PATCH 4/5] services: pulseaudio: Add an extra-script-files configuration field.

[Liliana Marie Prikler]

Hi,

Am Montag, dem 31.01.2022 um 23:19 -0500 schrieb Maxim Cournoyer:
> * gnu/services/sound.scm (<pulseaudio-configuration>)
> [extra-script-files]: Add field.
> (extra-script-files->file-union): Add procedure.
> (pulseaudio-etc): Use it.
> * doc/guix.texi: Document it.
> ---
>  doc/guix.texi          | 27 +++++++++++++++++++++++++++
>  gnu/services/sound.scm | 19 +++++++++++++++++--
>  2 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/guix.texi b/doc/guix.texi
> index a002670030..2f8df03461 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -21393,9 +21393,36 @@ List of settings to set in
> @file{daemon.conf}, formatted just like
>  @item @code{script-file} (default: @code{(file-append pulseaudio
> "/etc/pulse/default.pa")})
>  Script file to use as @file{default.pa}.
>  
> +@item @code{extra-script-files} (default: @code{'())})
> +A list of file-like objects defining extra PulseAudio scripts to run
> at
> +the initialization of the @command{pulseaudio} daemon.  For a
> reference
> +of the available commands, refer to @command{man pulse-cli-syntax}.
> +
>  @item @code{system-script-file} (default: @code{(file-append
> pulseaudio "/etc/pulse/system.pa")})
>  Script file to use as @file{system.pa}.
>  @end table
> +
> +The example below sets the default PulseAudio card profile, the
> default
> +sink and the default source to use for a old SoundBlaster Audigy
> sound
> +card:
> +@lisp
> +(pulseaudio-configuration
> + (extra-script-files
> +  (list (plain-file "configure-audigy-card"
> +                    (string-append "\
> +set-card-profile alsa_card.pci-0000_01_01.0 \
> +  output:analog-surround-40+input:analog-mono
> +set-default-source alsa_input.pci-0000_01_01.0.analog-mono
> +set-default-sink alsa_output.pci-0000_01_01.0.analog-surround-
> 40\n")))))
> +@end lisp
> +
> +Note that @code{pulseaudio-service-type} is part of
> +@code{%desktop-services}; if your operating system declaration was
> +derived from one of the desktop templates, you'll want to adjust the
> +above example to modify the existing @code{pulseaudio-service-type}
> via
> +@code{modify-services} (@pxref{Service Reference,
> +@code{modify-services}}), instead of defining a new one.
> +
>  @end deftp
>  
>  @deffn {Scheme Variable} ladspa-service-type
> diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
> index 19eccfc860..f529188a7c 100644
> --- a/gnu/services/sound.scm
> +++ b/gnu/services/sound.scm
> @@ -34,6 +34,7 @@ (define-module (gnu services sound)
>    #:use-module (gnu packages linux)
>    #:use-module (gnu packages pulseaudio)
>    #:use-module (ice-9 match)
> +  #:use-module (srfi srfi-1)
>    #:export (alsa-configuration
>              alsa-service-type
>  
> @@ -125,6 +126,8 @@ (define-record-type* <pulseaudio-configuration>
>                 (default '((flat-volumes . no))))
>    (script-file pulseaudio-configuration-script-file
>                 (default (file-append pulseaudio
> "/etc/pulse/default.pa")))
> +  (extra-script-files pulseaudio-configuration-extra-script-files
> +                      (default '()))
>    (system-script-file pulseaudio-configuration-system-script-file
>                        (default
>                          (file-append pulseaudio
> "/etc/pulse/system.pa"))))
> @@ -145,14 +148,26 @@ (define pulseaudio-environment
>         ("PULSE_CLIENTCONFIG" . ,(apply mixed-text-file "client.conf"
>                                         (map pulseaudio-conf-entry
> client-conf)))))))
>  
> +(define (extra-script-files->file-union extra-script-files)
> +  "Return a G-exp obtained by processing EXTRA-SCRIPT-FILES with
> FILE-UNION.
> +Each file is named \"snippet-n.pa\", where N is their 1-offset
> index."
> +  (let ((labels (map (lambda (n) (format #f "snippet-~a.pa" n))
> +                     (iota (length extra-script-files) 1))))
> +    (file-union "default.pa.d" (zip labels extra-script-files))))
> +
>  (define pulseaudio-etc
>    (match-lambda
> -    (($ <pulseaudio-configuration> _ _ default-script-file system-
> script-file)
> +    (($ <pulseaudio-configuration> _ _ default-script-file extra-
> script-files
> +                                   system-script-file)
>       `(("pulse"
>          ,(file-union
>            "pulse"
>            `(("default.pa" ,default-script-file)
> -            ("system.pa" ,system-script-file))))))))
> +            ("system.pa" ,system-script-file)
> +            ,@(if (null? extra-script-files)
> +                  '()
> +                  `(("default.pa.d" ,(extra-script-files->file-union
> +                                      extra-script-files)))))))))))
>  
>  (define pulseaudio-service-type
>    (service-type
Is there a particular use-case for this (other than working around the
location issue of default.pa et al.)?  If not, I'd rather make it s.t.
our other files can more easily be stitched together in-place.

Also, assuming that we're using file-like objects here, I think we
should use the store name minus prefix and hash for the file name. 
E.g. if Alice adds soundblaster.pa, it'd make sense to label it
soundblaster.pa, so that changes to snippet order don't mess up any
configuration referring to those files.

Cheers

[bug#53676] [PATCH 0/5] *** PulseAudio service improvements ***

[Maxim Cournoyer]

Hi Leo,

Leo Famulari <leo@famulari.name> writes:

> On Mon, Jan 31, 2022 at 11:13:52PM -0500, Maxim Cournoyer wrote:
>> This small series adds an easy way to drop PulseAudio configuration scripts to
>> /etc/pulse/default.pa.d.  It also lifts the need to reboot the machine to have
>> a new PulseAudio configuration active.
>
> Nice! That will be a great improvement.
>
>> Maxim Cournoyer (5):
>>   doc: Fix typo.
>>   services/sound: Normalize pulseaudio-configuration accessor names.
>>   gnu: pulseaudio: Graft to adjust configuration.
>>   services: pulseaudio: Add an extra-script-files configuration field.
>>   services: pulseaudio: Deploy the configuration files to /etc/pulse.
>
> I don't think we should use grafts for anything besides fixing very
> serious bugs, although they are definitely useful to demonstrate new
> features in a patch series.
>
> Pulseaudio only has ~1700 dependents. We can easily handle it on a
> "new-features" branch or similar.

True; I was thinking to proceed this way and ungraft it (as well as
other grafted packages) in the branch that will need to be made soon to
fix the Rust CVE.  A specialized world rebuild branch.

Maxim

[bug#53676] [PATCH 2/5] services/sound: Normalize pulseaudio-configuration accessor names.

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Am Montag, dem 31.01.2022 um 23:19 -0500 schrieb Maxim Cournoyer:
>> * gnu/services/sound.scm (<pulseaudio-configuration>): Adjust getter
>> names to match convention.
>> ---
>>  gnu/services/sound.scm | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>> 
>> diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
>> index 7beca35ffe..19eccfc860 100644
>> --- a/gnu/services/sound.scm
>> +++ b/gnu/services/sound.scm
>> @@ -2,6 +2,7 @@
>>  ;;; Copyright © 2018, 2020 Oleg Pykhalov <go.wigust@gmail.com>
>>  ;;; Copyright © 2020 Liliana Marie Prikler
>> <liliana.prikler@gmail.com>
>>  ;;; Copyright © 2020 Marius Bakke <mbakke@fastmail.com>
>> +;;; Copyright © 2022 Maxim Cournoyer <maxim.cournoyer@gmail.com>
>>  ;;;
>>  ;;; This file is part of GNU Guix.
>>  ;;;
>> @@ -115,16 +116,16 @@ (define alsa-service-type
>>  (define-record-type* <pulseaudio-configuration>
>>    pulseaudio-configuration make-pulseaudio-configuration
>>    pulseaudio-configuration?
>> -  (client-conf pulseaudio-client-conf
>> +  (client-conf pulseaudio-configuration-client-conf
>>                 (default '()))
>> -  (daemon-conf pulseaudio-daemon-conf
>> +  (daemon-conf pulseaudio-configuration-daemon-conf
>>                 ;; Flat volumes may cause unpleasant experiences to
>> users
>>                 ;; when applications inadvertently max out the system
>> volume
>>                 ;; (see e.g. <https://bugs.gnu.org/38172>).
>>                 (default '((flat-volumes . no))))
>> -  (script-file pulseaudio-script-file
>> +  (script-file pulseaudio-configuration-script-file
>>                 (default (file-append pulseaudio
>> "/etc/pulse/default.pa")))
>> -  (system-script-file pulseaudio-system-script-file
>> +  (system-script-file pulseaudio-configuration-system-script-file
>>                        (default
>>                          (file-append pulseaudio
>> "/etc/pulse/system.pa"))))
> I don't see calling code adjusted anywhere.  Is this because we only
> use match to access this records fields?

The bindings are not public, so they shouldn't be used elsewhere;
internally only match seems to be used yes.

> On a related note, would it make sense to port this over to (define-
> configuration)?

Agreed.  I'd prefer to keep this effort separate from this series
though.  Also, I still want to take some time to review the newly
introduced Guix records sanitizers; I feel they should perhaps be
leveraged in define-configuration (part of the appeal of
define-configuration is serialization, the other part being input
validation, which is what sanitizers seem to be designed for).

Maxim

[bug#53676] [PATCH 3/5] gnu: pulseaudio: Graft to adjust configuration.

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Am Montag, dem 31.01.2022 um 23:19 -0500 schrieb Maxim Cournoyer:
>> * gnu/packages/pulseaudio.scm (pulseaudio/fixed): New variable.
>> (pulseaudio)[replacement]: Graft package with it.
>> ---
>>  gnu/packages/pulseaudio.scm | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>> 
>> diff --git a/gnu/packages/pulseaudio.scm
>> b/gnu/packages/pulseaudio.scm
>> index fe028b5202..f529717ee1 100644
>> --- a/gnu/packages/pulseaudio.scm
>> +++ b/gnu/packages/pulseaudio.scm
>> @@ -178,6 +178,7 @@ (define-public libsamplerate
>>  (define-public pulseaudio
>>    (package
>>      (name "pulseaudio")
>> +    (replacement pulseaudio/fixed)
>>      (version "15.0")
>>      (source (origin
>>               (method url-fetch)
>> @@ -269,6 +270,23 @@ (define-public pulseaudio
>>      ;; 'LICENSE' for details.
>>      (license l:gpl2+)))
>>  
>> +(define pulseaudio/fixed
>> +  (package
>> +    (inherit pulseaudio)
>> +    (arguments
>> +     (substitute-keyword-arguments (package-arguments pulseaudio)
>> +       ((#:phases phases)
>> +        `(modify-phases ,phases
>> +           (add-after 'unpack 'customize-default-script
>> +             (lambda _
>> +               (call-with-port
>> +                (open-file "src/daemon/default.pa.in" "a")
>> +                (lambda (port)
>> +                  (format port "~%\
>> +### Include extra script files configured via the pulseaudio-
>> service-type.
>> +.nofail
>> +.include /etc/pulse/default.pa.d~%")))))))))))
>> +
> Note that there should be a .fail afterwards.  

Hmm.  I simply duplicated the existing two lines used by PulseAudio
itself.  I believe they do not care because it appears completely at the
end of the file.

> As Leo pointed out, we shouldn't do too many "feature grafts", so
> instead it might be worth moving this part to pulseaudio-service-type
> in some way, no?

I'd prefer to keep it like this, for simplicity; we need to rebuild the
world soon anyway to fix a Rust CVE, so we can batch things easily.

Maxim

[bug#53676] [PATCH 4/5] services: pulseaudio: Add an extra-script-files configuration field.

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Hi,
>
> Am Montag, dem 31.01.2022 um 23:19 -0500 schrieb Maxim Cournoyer:
>> * gnu/services/sound.scm (<pulseaudio-configuration>)
>> [extra-script-files]: Add field.
>> (extra-script-files->file-union): Add procedure.
>> (pulseaudio-etc): Use it.
>> * doc/guix.texi: Document it.
>> ---
>>  doc/guix.texi          | 27 +++++++++++++++++++++++++++
>>  gnu/services/sound.scm | 19 +++++++++++++++++--
>>  2 files changed, 44 insertions(+), 2 deletions(-)
>> 
>> diff --git a/doc/guix.texi b/doc/guix.texi
>> index a002670030..2f8df03461 100644
>> --- a/doc/guix.texi
>> +++ b/doc/guix.texi
>> @@ -21393,9 +21393,36 @@ List of settings to set in
>> @file{daemon.conf}, formatted just like
>>  @item @code{script-file} (default: @code{(file-append pulseaudio
>> "/etc/pulse/default.pa")})
>>  Script file to use as @file{default.pa}.
>>  
>> +@item @code{extra-script-files} (default: @code{'())})
>> +A list of file-like objects defining extra PulseAudio scripts to run
>> at
>> +the initialization of the @command{pulseaudio} daemon.  For a
>> reference
>> +of the available commands, refer to @command{man pulse-cli-syntax}.
>> +
>>  @item @code{system-script-file} (default: @code{(file-append
>> pulseaudio "/etc/pulse/system.pa")})
>>  Script file to use as @file{system.pa}.
>>  @end table
>> +
>> +The example below sets the default PulseAudio card profile, the
>> default
>> +sink and the default source to use for a old SoundBlaster Audigy
>> sound
>> +card:
>> +@lisp
>> +(pulseaudio-configuration
>> + (extra-script-files
>> +  (list (plain-file "configure-audigy-card"
>> +                    (string-append "\
>> +set-card-profile alsa_card.pci-0000_01_01.0 \
>> +  output:analog-surround-40+input:analog-mono
>> +set-default-source alsa_input.pci-0000_01_01.0.analog-mono
>> +set-default-sink alsa_output.pci-0000_01_01.0.analog-surround-
>> 40\n")))))
>> +@end lisp
>> +
>> +Note that @code{pulseaudio-service-type} is part of
>> +@code{%desktop-services}; if your operating system declaration was
>> +derived from one of the desktop templates, you'll want to adjust the
>> +above example to modify the existing @code{pulseaudio-service-type}
>> via
>> +@code{modify-services} (@pxref{Service Reference,
>> +@code{modify-services}}), instead of defining a new one.
>> +
>>  @end deftp
>>  
>>  @deffn {Scheme Variable} ladspa-service-type
>> diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
>> index 19eccfc860..f529188a7c 100644
>> --- a/gnu/services/sound.scm
>> +++ b/gnu/services/sound.scm
>> @@ -34,6 +34,7 @@ (define-module (gnu services sound)
>>    #:use-module (gnu packages linux)
>>    #:use-module (gnu packages pulseaudio)
>>    #:use-module (ice-9 match)
>> +  #:use-module (srfi srfi-1)
>>    #:export (alsa-configuration
>>              alsa-service-type
>>  
>> @@ -125,6 +126,8 @@ (define-record-type* <pulseaudio-configuration>
>>                 (default '((flat-volumes . no))))
>>    (script-file pulseaudio-configuration-script-file
>>                 (default (file-append pulseaudio
>> "/etc/pulse/default.pa")))
>> +  (extra-script-files pulseaudio-configuration-extra-script-files
>> +                      (default '()))
>>    (system-script-file pulseaudio-configuration-system-script-file
>>                        (default
>>                          (file-append pulseaudio
>> "/etc/pulse/system.pa"))))
>> @@ -145,14 +148,26 @@ (define pulseaudio-environment
>>         ("PULSE_CLIENTCONFIG" . ,(apply mixed-text-file "client.conf"
>>                                         (map pulseaudio-conf-entry
>> client-conf)))))))
>>  
>> +(define (extra-script-files->file-union extra-script-files)
>> +  "Return a G-exp obtained by processing EXTRA-SCRIPT-FILES with
>> FILE-UNION.
>> +Each file is named \"snippet-n.pa\", where N is their 1-offset
>> index."
>> +  (let ((labels (map (lambda (n) (format #f "snippet-~a.pa" n))
>> +                     (iota (length extra-script-files) 1))))
>> +    (file-union "default.pa.d" (zip labels extra-script-files))))
>> +
>>  (define pulseaudio-etc
>>    (match-lambda
>> -    (($ <pulseaudio-configuration> _ _ default-script-file system-
>> script-file)
>> +    (($ <pulseaudio-configuration> _ _ default-script-file extra-
>> script-files
>> +                                   system-script-file)
>>       `(("pulse"
>>          ,(file-union
>>            "pulse"
>>            `(("default.pa" ,default-script-file)
>> -            ("system.pa" ,system-script-file))))))))
>> +            ("system.pa" ,system-script-file)
>> +            ,@(if (null? extra-script-files)
>> +                  '()
>> +                  `(("default.pa.d" ,(extra-script-files->file-union
>> +                                      extra-script-files)))))))))))
>>  
>>  (define pulseaudio-service-type
>>    (service-type
> Is there a particular use-case for this (other than working around the
> location issue of default.pa et al.)?  If not, I'd rather make it s.t.
> our other files can more easily be stitched together in-place.

You mean, a use case for extra-script-files?  Sorry, I missed something
in the "make it s.t. our other [...]"; what does "s.t." stands for?

My use case is the one I documented in the manual; setting a default
card profile for example.  Also choosing the default sink and source of
a card; this can be done in client.conf but that doesn't get reflected
anywhere on the state of a running pulseaudio server it seems, contrary
to calling 'set-default-sink ...', which takes effect server-side.

> Also, assuming that we're using file-like objects here, I think we
> should use the store name minus prefix and hash for the file name. 
> E.g. if Alice adds soundblaster.pa, it'd make sense to label it
> soundblaster.pa, so that changes to snippet order don't mess up any
> configuration referring to those files.

I actually wanted to do that but decided against since there's no clean
API to retrieve the name of a G-Exp file-like object (it could be done,
currently, but it'd be messy and fragile, it seems).

But good observation, I wanted to document that the extra script files
are loaded in the order they are listed.

Maxim

[bug#53676] [PATCH 4/5] services: pulseaudio: Add an extra-script-files configuration field.

[Liliana Marie Prikler]

Hi,

Am Dienstag, dem 01.02.2022 um 15:27 -0500 schrieb Maxim Cournoyer:
> [...]
> > Is there a particular use-case for this (other than working around
> > the location issue of default.pa et al.)?  If not, I'd rather make
> > it s.t. our other files can more easily be stitched together in-
> > place.
> 
> You mean, a use case for extra-script-files?  
Yes.

> Sorry, I missed something in the "make it s.t. our other [...]"; what
> does "s.t." stands for?
"such that" or "so that".  Pretty common among mathematicians, I think
🙃

> My use case is the one I documented in the manual; setting a default
> card profile for example.  Also choosing the default sink and source
> of a card; this can be done in client.conf but that doesn't get
> reflected anywhere on the state of a running pulseaudio server it
> seems, contrary to calling 'set-default-sink ...', which takes effect
> server-side.
And you can't do this inside default.pa, because ... ?

> > Also, assuming that we're using file-like objects here, I think we
> > should use the store name minus prefix and hash for the file name. 
> > E.g. if Alice adds soundblaster.pa, it'd make sense to label it
> > soundblaster.pa, so that changes to snippet order don't mess up any
> > configuration referring to those files.
> 
> I actually wanted to do that but decided against since there's no
> clean API to retrieve the name of a G-Exp file-like object (it could
> be done, currently, but it'd be messy and fragile, it seems).
> 
> But good observation, I wanted to document that the extra script
> files are loaded in the order they are listed.
Isn't that what "strip-store-file-name" from (guix build utils) does?
(Let's ignore hard-coded hash length...)

Cheers

[bug#53676] [PATCH 2/5] services/sound: Normalize pulseaudio-configuration accessor names.

[Liliana Marie Prikler]

Am Dienstag, dem 01.02.2022 um 15:18 -0500 schrieb Maxim Cournoyer:
> Hi Liliana,
> 
> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
> 
> > Am Montag, dem 31.01.2022 um 23:19 -0500 schrieb Maxim Cournoyer:
> > 
> > > * gnu/services/sound.scm (<pulseaudio-configuration>): Adjust
> > > getter
> > > names to match convention.
> > > ---
> > >  gnu/services/sound.scm | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
> > > index 7beca35ffe..19eccfc860 100644
> > > --- a/gnu/services/sound.scm
> > > +++ b/gnu/services/sound.scm
> > > @@ -2,6 +2,7 @@
> > >  ;;; Copyright © 2018, 2020 Oleg Pykhalov <go.wigust@gmail.com>
> > >  ;;; Copyright © 2020 Liliana Marie Prikler
> > > <liliana.prikler@gmail.com>
> > >  ;;; Copyright © 2020 Marius Bakke <mbakke@fastmail.com>
> > > +;;; Copyright © 2022 Maxim Cournoyer <maxim.cournoyer@gmail.com>
> > >  ;;;
> > >  ;;; This file is part of GNU Guix.
> > >  ;;;
> > > @@ -115,16 +116,16 @@ (define alsa-service-type
> > >  (define-record-type* <pulseaudio-configuration>
> > >    pulseaudio-configuration make-pulseaudio-configuration
> > >    pulseaudio-configuration?
> > > -  (client-conf pulseaudio-client-conf
> > > +  (client-conf pulseaudio-configuration-client-conf
> > >                 (default '()))
> > > -  (daemon-conf pulseaudio-daemon-conf
> > > +  (daemon-conf pulseaudio-configuration-daemon-conf
> > >                 ;; Flat volumes may cause unpleasant experiences
> > > to
> > > users
> > >                 ;; when applications inadvertently max out the
> > > system
> > > volume
> > >                 ;; (see e.g. <https://bugs.gnu.org/38172>).
> > >                 (default '((flat-volumes . no))))
> > > -  (script-file pulseaudio-script-file
> > > +  (script-file pulseaudio-configuration-script-file
> > >                 (default (file-append pulseaudio
> > > "/etc/pulse/default.pa")))
> > > -  (system-script-file pulseaudio-system-script-file
> > > +  (system-script-file pulseaudio-configuration-system-script-
> > > file
> > >                        (default
> > >                          (file-append pulseaudio
> > > "/etc/pulse/system.pa"))))
> > I don't see calling code adjusted anywhere.  Is this because we
> > only use match to access this records fields?
> 
> The bindings are not public, so they shouldn't be used elsewhere;
> internally only match seems to be used yes.
Good.

> > On a related note, would it make sense to port this over to
> > (define-configuration)?
> 
> Agreed.  I'd prefer to keep this effort separate from this series
> though.  Also, I still want to take some time to review the newly
> introduced Guix records sanitizers; I feel they should perhaps be
> leveraged in define-configuration (part of the appeal of
> define-configuration is serialization, the other part being input
> validation, which is what sanitizers seem to be designed for).
Fair enough.

[bug#53676] [PATCH 3/5] gnu: pulseaudio: Graft to adjust configuration.

[Liliana Marie Prikler]

Am Dienstag, dem 01.02.2022 um 15:20 -0500 schrieb Maxim Cournoyer:
> Hi Liliana,
> 
> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
> 
> > Am Montag, dem 31.01.2022 um 23:19 -0500 schrieb Maxim Cournoyer:
> > > * gnu/packages/pulseaudio.scm (pulseaudio/fixed): New variable.
> > > (pulseaudio)[replacement]: Graft package with it.
> > > ---
> > >  gnu/packages/pulseaudio.scm | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > > 
> > > diff --git a/gnu/packages/pulseaudio.scm
> > > b/gnu/packages/pulseaudio.scm
> > > index fe028b5202..f529717ee1 100644
> > > --- a/gnu/packages/pulseaudio.scm
> > > +++ b/gnu/packages/pulseaudio.scm
> > > @@ -178,6 +178,7 @@ (define-public libsamplerate
> > >  (define-public pulseaudio
> > >    (package
> > >      (name "pulseaudio")
> > > +    (replacement pulseaudio/fixed)
> > >      (version "15.0")
> > >      (source (origin
> > >               (method url-fetch)
> > > @@ -269,6 +270,23 @@ (define-public pulseaudio
> > >      ;; 'LICENSE' for details.
> > >      (license l:gpl2+)))
> > >  
> > > +(define pulseaudio/fixed
> > > +  (package
> > > +    (inherit pulseaudio)
> > > +    (arguments
> > > +     (substitute-keyword-arguments (package-arguments
> > > pulseaudio)
> > > +       ((#:phases phases)
> > > +        `(modify-phases ,phases
> > > +           (add-after 'unpack 'customize-default-script
> > > +             (lambda _
> > > +               (call-with-port
> > > +                (open-file "src/daemon/default.pa.in" "a")
> > > +                (lambda (port)
> > > +                  (format port "~%\
> > > +### Include extra script files configured via the pulseaudio-
> > > service-type.
> > > +.nofail
> > > +.include /etc/pulse/default.pa.d~%")))))))))))
> > > +
> > Note that there should be a .fail afterwards.  
> 
> Hmm.  I simply duplicated the existing two lines used by PulseAudio
> itself.  I believe they do not care because it appears completely at
> the end of the file.
I think we would need to care though, because one could write a gexp
that appends to default.pa, but then has unclear semantics.

> > As Leo pointed out, we shouldn't do too many "feature grafts", so
> > instead it might be worth moving this part to pulseaudio-service-
> > type in some way, no?
> 
> I'd prefer to keep it like this, for simplicity; we need to rebuild
> the world soon anyway to fix a Rust CVE, so we can batch things
> easily.
Can you define "simplicity" here?  In my opinion, services/stuff.scm or
/etc/config.scm provide an easier point of change/extension than
packages do -- particularly also because pulseaudio-service-type (even
with this patch set) does not allow changing the pulseaudio package.

For the record, if you're wondering why pulseaudio-service-type doesn't
have a configuration knob for adding the pulseaudio package: pulseaudio
is currently managed per user session through dbus, not shepherd.  It'd
be nice to have systemd-levels of control over user services, but we're
not there yet.

Cheers

[bug#53676] [PATCH 4/5] services: pulseaudio: Add an extra-script-files configuration field.

[Maxim Cournoyer]

Hello,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Hi,
>
> Am Dienstag, dem 01.02.2022 um 15:27 -0500 schrieb Maxim Cournoyer:
>> [...]
>> > Is there a particular use-case for this (other than working around
>> > the location issue of default.pa et al.)?  If not, I'd rather make
>> > it s.t. our other files can more easily be stitched together in-
>> > place.
>> 
>> You mean, a use case for extra-script-files?  
> Yes.
>
>> Sorry, I missed something in the "make it s.t. our other [...]"; what
>> does "s.t." stands for?
> "such that" or "so that".  Pretty common among mathematicians, I think
> 🙃

Ah!

>> My use case is the one I documented in the manual; setting a default
>> card profile for example.  Also choosing the default sink and source
>> of a card; this can be done in client.conf but that doesn't get
>> reflected anywhere on the state of a running pulseaudio server it
>> seems, contrary to calling 'set-default-sink ...', which takes effect
>> server-side.
> And you can't do this inside default.pa, because ... ?

I could; but what I want is to *extend*, rather than *replace* the
default.pa script; the native PulseAudio mechanism to do so is to put
files under '/etc/default.pa.d'.  We could simply tell people to use
extra-special-file service to achieve that, but that's less discoverable
than having a convenient, documented field to do so :-).

>> > Also, assuming that we're using file-like objects here, I think we
>> > should use the store name minus prefix and hash for the file name. 
>> > E.g. if Alice adds soundblaster.pa, it'd make sense to label it
>> > soundblaster.pa, so that changes to snippet order don't mess up any
>> > configuration referring to those files.
>> 
>> I actually wanted to do that but decided against since there's no
>> clean API to retrieve the name of a G-Exp file-like object (it could
>> be done, currently, but it'd be messy and fragile, it seems).
>> 
>> But good observation, I wanted to document that the extra script
>> files are loaded in the order they are listed.
> Isn't that what "strip-store-file-name" from (guix build utils) does?
> (Let's ignore hard-coded hash length...)

'strip-store-file-name' would be able to get the name from the store item
(built derivation), but file-union takes a "two-element list where the
first element is the file name to use in the new directory, and the
second element is a gexp denoting the target file", e.g., before the
file-like object is built.  I don't see an easy way to make it work.

Thanks,

Maxim

[bug#53676] [PATCH 3/5] gnu: pulseaudio: Graft to adjust configuration.

[Maxim Cournoyer]

Hello again,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Am Dienstag, dem 01.02.2022 um 15:20 -0500 schrieb Maxim Cournoyer:
>> Hi Liliana,
>> 
>> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
>> 
>> > Am Montag, dem 31.01.2022 um 23:19 -0500 schrieb Maxim Cournoyer:
>> > > * gnu/packages/pulseaudio.scm (pulseaudio/fixed): New variable.
>> > > (pulseaudio)[replacement]: Graft package with it.
>> > > ---
>> > >  gnu/packages/pulseaudio.scm | 18 ++++++++++++++++++
>> > >  1 file changed, 18 insertions(+)
>> > > 
>> > > diff --git a/gnu/packages/pulseaudio.scm
>> > > b/gnu/packages/pulseaudio.scm
>> > > index fe028b5202..f529717ee1 100644
>> > > --- a/gnu/packages/pulseaudio.scm
>> > > +++ b/gnu/packages/pulseaudio.scm
>> > > @@ -178,6 +178,7 @@ (define-public libsamplerate
>> > >  (define-public pulseaudio
>> > >    (package
>> > >      (name "pulseaudio")
>> > > +    (replacement pulseaudio/fixed)
>> > >      (version "15.0")
>> > >      (source (origin
>> > >               (method url-fetch)
>> > > @@ -269,6 +270,23 @@ (define-public pulseaudio
>> > >      ;; 'LICENSE' for details.
>> > >      (license l:gpl2+)))
>> > >  
>> > > +(define pulseaudio/fixed
>> > > +  (package
>> > > +    (inherit pulseaudio)
>> > > +    (arguments
>> > > +     (substitute-keyword-arguments (package-arguments
>> > > pulseaudio)
>> > > +       ((#:phases phases)
>> > > +        `(modify-phases ,phases
>> > > +           (add-after 'unpack 'customize-default-script
>> > > +             (lambda _
>> > > +               (call-with-port
>> > > +                (open-file "src/daemon/default.pa.in" "a")
>> > > +                (lambda (port)
>> > > +                  (format port "~%\
>> > > +### Include extra script files configured via the pulseaudio-
>> > > service-type.
>> > > +.nofail
>> > > +.include /etc/pulse/default.pa.d~%")))))))))))
>> > > +
>> > Note that there should be a .fail afterwards.  
>> 
>> Hmm.  I simply duplicated the existing two lines used by PulseAudio
>> itself.  I believe they do not care because it appears completely at
>> the end of the file.
> I think we would need to care though, because one could write a gexp
> that appends to default.pa, but then has unclear semantics.

If someone was to append something to default.pa (the exact one shipped
with PulseAudio), they'd have to add the .fail themselves to undo
PulseAudio's own .nofail, right?  I don't see why we should go out of
our way to change that.

With the proposed 'extra-script-files', I'd argue that appending
something to default.pa should be considered an anti-pattern; as the new
field would be the more natural option to *extend* 'default.pa' (and
having a field to override default.pa is still useful if you don't like
any of the default behavior).

>> > As Leo pointed out, we shouldn't do too many "feature grafts", so
>> > instead it might be worth moving this part to pulseaudio-service-
>> > type in some way, no?
>> 
>> I'd prefer to keep it like this, for simplicity; we need to rebuild
>> the world soon anyway to fix a Rust CVE, so we can batch things
>> easily.
> Can you define "simplicity" here?  In my opinion, services/stuff.scm or
> /etc/config.scm provide an easier point of change/extension than
> packages do -- particularly also because pulseaudio-service-type (even
> with this patch set) does not allow changing the pulseaudio package.

The default behavior of default.pa is to allow loading extra files from
from 'pulsesysconfdir', which in our case corresponds to output/etc of
pulseaudio; e.g.:

--8<---------------cut here---------------start------------->8---
### Allow including a default.pa.d directory, which if present, can be used
### for additional configuration snippets.
.nofail
.include /gnu/store/7xwgz4bavb1i8sfx1lm55hlrr3ngjkdx-pulseaudio-15.0/etc/pulse/default.pa.d
--8<---------------cut here---------------end--------------->8---

That's not very useful, but is preserved in case pulseaudio ever decides
to drop their own scripts in there.  Adjusting this path is more natural
and straightforwardly done from the package description than from the
service, in my opinion.

Does that make sense?

Thanks,

Maxim

[bug#53676] [PATCH 4/5] services: pulseaudio: Add an extra-script-files configuration field.

[Liliana Marie Prikler]

Hi,

Am Dienstag, dem 01.02.2022 um 22:44 -0500 schrieb Maxim Cournoyer:
> > > My use case is the one I documented in the manual; setting a
> > > default
> > > card profile for example.  Also choosing the default sink and
> > > source
> > > of a card; this can be done in client.conf but that doesn't get
> > > reflected anywhere on the state of a running pulseaudio server it
> > > seems, contrary to calling 'set-default-sink ...', which takes
> > > effect
> > > server-side.
> > And you can't do this inside default.pa, because ... ?
> 
> I could; but what I want is to *extend*, rather than *replace* the
> default.pa script; the native PulseAudio mechanism to do so is to put
> files under '/etc/default.pa.d'.  We could simply tell people to use
> extra-special-file service to achieve that, but that's less
> discoverable than having a convenient, documented field to do so :-).
I still don't understand what the big difference would be when it comes
to Guix.  You can already split your configuration over several modules
and include the bits you want, it doesn't particularly have to be the
way pulseaudio hacks around the lack of such functionality in
traditional distros.

Again, I might be missing a use case in which pulseaudio's style makes
more sense, but there appears little reason to create these directories
simply for the sake of it.

> > > > Also, assuming that we're using file-like objects here, I think
> > > > we should use the store name minus prefix and hash for the file
> > > > name. 
> > > > E.g. if Alice adds soundblaster.pa, it'd make sense to label it
> > > > soundblaster.pa, so that changes to snippet order don't mess up
> > > > any configuration referring to those files.
> > > 
> > > I actually wanted to do that but decided against since there's no
> > > clean API to retrieve the name of a G-Exp file-like object (it
> > > could be done, currently, but it'd be messy and fragile, it
> > > seems).
> > > 
> > > But good observation, I wanted to document that the extra script
> > > files are loaded in the order they are listed.
> > Isn't that what "strip-store-file-name" from (guix build utils)
> > does?
> > (Let's ignore hard-coded hash length...)
> 
> 'strip-store-file-name' would be able to get the name from the store
> item (built derivation), but file-union takes a "two-element list
> where the first element is the file name to use in the new directory,
> and the second element is a gexp denoting the target file", e.g.,
> before the file-like object is built.  I don't see an easy way to
> make it work.
For the record, I do think we'd like to use file-like objects here, not
raw gexps.  If that fails, why not expose the name to gexp mapping
completely?  I don't know why you'd want to take away that control.

[bug#53676] [PATCH 3/5] gnu: pulseaudio: Graft to adjust configuration.

[Liliana Marie Prikler]

Hi,

Am Dienstag, dem 01.02.2022 um 23:30 -0500 schrieb Maxim Cournoyer:
> If someone was to append something to default.pa (the exact one shipped
> with PulseAudio), they'd have to add the .fail themselves to undo
> PulseAudio's own .nofail, right?  I don't see why we should go out of
> our way to change that.
Didn't you add that .nofail on your own?  If not, why include the
directive?

> With the proposed 'extra-script-files', I'd argue that appending
> something to default.pa should be considered an anti-pattern; as the
> new field would be the more natural option to *extend* 'default.pa'
> (and having a field to override default.pa is still useful if you don't
> like any of the default behavior).
I don't think you're making a good case here.  Why do you want
appending to default.pa to be an anti-pattern?
> > > 

> > Can you define "simplicity" here?  In my opinion, services/stuff.scm
> > or
> > /etc/config.scm provide an easier point of change/extension than
> > packages do -- particularly also because pulseaudio-service-type
> > (even with this patch set) does not allow changing the pulseaudio
> > package.
> 
> The default behavior of default.pa is to allow loading extra files from
> 'pulsesysconfdir', which in our case corresponds to output/etc
> of pulseaudio; e.g.:
> 
> --8<---------------cut here---------------start------------->8---
> ### Allow including a default.pa.d directory, which if present, can be
> used
> ### for additional configuration snippets.
> .nofail
> .include /gnu/store/7xwgz4bavb1i8sfx1lm55hlrr3ngjkdx-pulseaudio-
> 15.0/etc/pulse/default.pa.d
> --8<---------------cut here---------------end--------------->8---
> 
> That's not very useful, but is preserved in case pulseaudio ever
> decides to drop their own scripts in there.  Adjusting this path is
> more natural and straightforwardly done from the package description
> than from the service, in my opinion.
Well, by Hyrum's Law we can be sure that someone inherited pulseaudio
to put files into pulsesysconfdir.  That aside, I think substitute*
would be expressing your intent better here, because what you actually
want is to match that line and then append an .include
/etc/pulse/default.pa.d hardcoded.

I still don't agree that that's a good idea, however.  Particularly, it
would lead to including files from an "old distro" that was infected
with Guix when that probably wasn't asked for.  If at all enabled, I'd
prefer if pulseaudio-service-type magically inserted that snippet for
configurations that add files to default.pa.d.

Note also that default.pa.d has no history [1] in traditional distros,
so it's a feature that likely won't be missed by anyone, at least not
out of nostalgia.  In addition, I'd be careful with claims towards our
intent of including this snippet at all.  As far as I know, it simply
wasn't removed, which might just as well mean that it didn't break the
build for anyone.

Cheers

[1]
https://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/src/daemon/default.pa.in?h=v14.2#n175

[bug#53676] [PATCH 5/5] services: pulseaudio: Deploy the configuration files to /etc/pulse.

[Jack Hill]

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

On Tue, 1 Feb 2022, Liliana Marie Prikler wrote:

> Hi,
>
> Am Montag, dem 31.01.2022 um 23:19 -0500 schrieb Maxim Cournoyer:
>> * gnu/services/sound.scm (pulseaudio-environment)
>> [PULSE_CONFIG, PULSE_CLIENTCONFIG]: Use fix locations, and move logic
>> to...
>> (pulseaudio-etc): ... this service extension.  Guard against producing
>> empty files.
>
> This patch reproduces (more or less) the initial layout we had for
> pulseaudio-service-type.  However, that layout has been reported to not
> work with some sandboxes.  I tried tracking down a specific bug, but
> could only gather <https://issues.guix.gnu.org/42118#3>.
>
>> Due to a bug with webkit sandboxing, we no longer put daemon.conf
>> into /etc/pulse (my bad), but rather set PULSE_CONFIG to directly
>> point to it.
>
> In other words, we should check whether Epiphany still plays sound
> properly with this patch applied.
>
> Cheers

I reported the original bugs for this in Guix [0] and WebKitGTK [1], so it 
was easy for me to find the references; hope they help! Unfortunately, it 
doesn't look like the WebKitGTK bug has been fixed (probably waiting on a 
C++ hacker). Note that the symptom I saw wasn't just that sound didn't 
work, but that the sandboxed processes crashed, so no web content was 
rendered.

[0] https://issues.guix.gnu.org/40837
[1] https://bugs.webkit.org/show_bug.cgi?id=211131

Unfortunately, I haven't had time to test this series.

Sorry!
Jack

[bug#53676] [PATCH 0/5] *** PulseAudio service improvements ***

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Hi,
>
> Am Dienstag, dem 01.02.2022 um 23:30 -0500 schrieb Maxim Cournoyer:
>> If someone was to append something to default.pa (the exact one shipped
>> with PulseAudio), they'd have to add the .fail themselves to undo
>> PulseAudio's own .nofail, right?  I don't see why we should go out of
>> our way to change that.
> Didn't you add that .nofail on your own?  If not, why include the
> directive?

You are right that it's not needed.  I've reviewed how that's done, see
below.

>> With the proposed 'extra-script-files', I'd argue that appending
>> something to default.pa should be considered an anti-pattern; as the
>> new field would be the more natural option to *extend* 'default.pa'
>> (and having a field to override default.pa is still useful if you don't
>> like any of the default behavior).
> I don't think you're making a good case here.  Why do you want
> appending to default.pa to be an anti-pattern?

Basically, to keep things as simple as they can be.  I'm expecting that
extending the default.pa file must be a more common use case than
hacking it up, justifying the 'extra-script-files' simple entry point
catered for this use case.  Compare:

--8<---------------cut here---------------start------------->8---
(script-file (computed-file "default.pa"
                            #~(begin
                                (copy-file #$(file-apend pulseaudio "/etc/default.pa")
                                           #$output)
                                (call-with-port
                                  (open-file #$output "a")
                                    (lambda (port)
                                      (format port "~%\
set-card-profile alsa_card.pci-0000_01_01.0 output:analog-surround-40+input:analog-mono
set-default-source alsa_input.pci-0000_01_01.0.analog-mono
set-default-sink alsa_output.pci-0000_01_01.0.analog-surround-40~%"))))))
--8<---------------cut here---------------end--------------->8---

to:

--8<---------------cut here---------------start------------->8---
(extra-script-files
  (list (plain-file "configure-audigy-card"
          (string-append "\
set-card-profile alsa_card.pci-0000_01_01.0 output:analog-surround-40+input:analog-mono
set-default-source alsa_input.pci-0000_01_01.0.analog-mono
set-default-sink alsa_output.pci-0000_01_01.0.analog-surround-40\n"))))
--8<---------------cut here---------------end--------------->8---

The later seems simpler, especially for someone starting with Guix and
not very familiar with Guile and G-expressions.

[...]

> That aside, I think substitute* would be expressing your intent better
> here, because what you actually want is to match that line and then
> append an .include /etc/pulse/default.pa.d hardcoded.

Good idea; I've made the change, like so:

--8<---------------cut here---------------start------------->8---
modified   gnu/packages/pulseaudio.scm
@@ -279,13 +279,12 @@ (define pulseaudio/fixed
         `(modify-phases ,phases
            (add-after 'unpack 'customize-default-script
              (lambda _
-               (call-with-port
-                (open-file "src/daemon/default.pa.in" "a")
-                (lambda (port)
-                  (format port "~%\
-### Include extra script files configured via the pulseaudio-service-type.
-.nofail
-.include /etc/pulse/default.pa.d~%")))))))))))
+               (substitute* "src/daemon/default.pa.in"
+                 (("^\\.include.*default.pa.d.*" anchor)
+                  (string-append
+                   ;; Honor PulseAudio script extensions found under
+                   ;; /etc/pulse/default.pa.d.
+                   anchor ".include /etc/pulse/default.pa.d\n")))))))))))
 
 (define-public pavucontrol
   (package
--8<---------------cut here---------------end--------------->8---

> I still don't agree that that's a good idea, however.  Particularly, it
> would lead to including files from an "old distro" that was infected
> with Guix when that probably wasn't asked for.  If at all enabled, I'd
> prefer if pulseaudio-service-type magically inserted that snippet for
> configurations that add files to default.pa.d.

There are pros and cons; people might be find it handy that a
Guix-installed pulseaudio also honors their user scripts living under
/etc/pulse/default.pa.d.  It seems low risk to me; not worth the extra
complexity in my opinion.

Thanks,

Maxim

[bug#53676] [PATCH 4/5] services: pulseaudio: Add an extra-script-files configuration field.

[Maxim Cournoyer]

Hi Liliana,

[...]

>> 'strip-store-file-name' would be able to get the name from the store
>> item (built derivation), but file-union takes a "two-element list
>> where the first element is the file name to use in the new directory,
>> and the second element is a gexp denoting the target file", e.g.,
>> before the file-like object is built.  I don't see an easy way to
>> make it work.
> For the record, I do think we'd like to use file-like objects here, not
> raw gexps.  If that fails, why not expose the name to gexp mapping
> completely?  I don't know why you'd want to take away that control.

If we limit ourselves to file-like objects, we can do something like
this:

--8<---------------cut here---------------start------------->8---
1 file changed, 20 insertions(+), 4 deletions(-)
gnu/services/sound.scm | 24 ++++++++++++++++++++----

modified   gnu/services/sound.scm
@@ -26,10 +26,12 @@ (define-module (gnu services sound)
   #:use-module (gnu services)
   #:use-module (gnu system pam)
   #:use-module (gnu system shadow)
+  #:use-module (guix diagnostics)
   #:use-module (guix gexp)
   #:use-module (guix packages)
   #:use-module (guix records)
   #:use-module (guix store)
+  #:use-module (guix ui)
   #:use-module (gnu packages audio)
   #:use-module (gnu packages linux)
   #:use-module (gnu packages pulseaudio)
@@ -149,10 +151,24 @@ (define pulseaudio-environment
        ("PULSE_CLIENTCONFIG" . "/etc/pulse/client.conf")))))
 
 (define (extra-script-files->file-union extra-script-files)
-  "Return a G-exp obtained by processing EXTRA-SCRIPT-FILES with FILE-UNION.
-Each file is named \"snippet-n.pa\", where N is their 1-offset index."
-  (let ((labels (map (lambda (n) (format #f "snippet-~a.pa" n))
-                     (iota (length extra-script-files) 1))))
+  "Return a G-exp obtained by processing EXTRA-SCRIPT-FILES with FILE-UNION."
+
+  (define (file-like->name file)
+    (let ((name (match file
+                  ((? local-file?)
+                   (local-file-name file))
+                  ((? plain-file?)
+                   (plain-file-name file))
+                  ((? computed-file?)
+                   (computed-file-name file))
+                  (_ (leave (G_ "~a is not a local-file, plain-file or \
+computed-file object~%") file)))))
+      (unless (string-suffix? name ".pa")
+        (leave (G_ "`~a' lacks the required '.pa' file name extension~%")
+               name))
+      name))
+
+  (let ((labels (map file-like->name extra-script-files)))
     (file-union "default.pa.d" (zip labels extra-script-files))))
 
 (define pulseaudio-etc
--8<---------------cut here---------------end--------------->8---

It works; and I agree it's nice to have control over the file name.

Maxim

[bug#53676] [PATCH 4/5] services: pulseaudio: Add an extra-script-files configuration field.

[Liliana Marie Prikler]

Hi Maxim,

Am Sonntag, dem 06.02.2022 um 02:25 -0500 schrieb Maxim Cournoyer:
> Hi Liliana,
> 
> [...]
> 
> > > 'strip-store-file-name' would be able to get the name from the
> > > store
> > > item (built derivation), but file-union takes a "two-element list
> > > where the first element is the file name to use in the new
> > > directory,
> > > and the second element is a gexp denoting the target file", e.g.,
> > > before the file-like object is built.  I don't see an easy way to
> > > make it work.
> > For the record, I do think we'd like to use file-like objects here,
> > not
> > raw gexps.  If that fails, why not expose the name to gexp mapping
> > completely?  I don't know why you'd want to take away that control.
> 
> If we limit ourselves to file-like objects, we can do something like
> this:
> 
> --8<---------------cut here---------------start------------->8---
> 1 file changed, 20 insertions(+), 4 deletions(-)
> gnu/services/sound.scm | 24 ++++++++++++++++++++----
> 
> modified   gnu/services/sound.scm
> @@ -26,10 +26,12 @@ (define-module (gnu services sound)
>    #:use-module (gnu services)
>    #:use-module (gnu system pam)
>    #:use-module (gnu system shadow)
> +  #:use-module (guix diagnostics)
>    #:use-module (guix gexp)
>    #:use-module (guix packages)
>    #:use-module (guix records)
>    #:use-module (guix store)
> +  #:use-module (guix ui)
>    #:use-module (gnu packages audio)
>    #:use-module (gnu packages linux)
>    #:use-module (gnu packages pulseaudio)
> @@ -149,10 +151,24 @@ (define pulseaudio-environment
>         ("PULSE_CLIENTCONFIG" . "/etc/pulse/client.conf")))))
>  
>  (define (extra-script-files->file-union extra-script-files)
> -  "Return a G-exp obtained by processing EXTRA-SCRIPT-FILES with FILE-
> UNION.
> -Each file is named \"snippet-n.pa\", where N is their 1-offset index."
> -  (let ((labels (map (lambda (n) (format #f "snippet-~a.pa" n))
> -                     (iota (length extra-script-files) 1))))
> +  "Return a G-exp obtained by processing EXTRA-SCRIPT-FILES with FILE-
> UNION."
> +
> +  (define (file-like->name file)
> +    (let ((name (match file
> +                  ((? local-file?)
> +                   (local-file-name file))
> +                  ((? plain-file?)
> +                   (plain-file-name file))
> +                  ((? computed-file?)
> +                   (computed-file-name file))
> +                  (_ (leave (G_ "~a is not a local-file, plain-file or
> \
> +computed-file object~%") file)))))
> +      (unless (string-suffix? name ".pa")
> +        (leave (G_ "`~a' lacks the required '.pa' file name
> extension~%")
> +               name))
> +      name))
> +
> +  (let ((labels (map file-like->name extra-script-files)))
>      (file-union "default.pa.d" (zip labels extra-script-files))))
>  
>  (define pulseaudio-etc
> --8<---------------cut here---------------end--------------->8---
> 
> It works; and I agree it's nice to have control over the file name.
Note that file-like->name serves multiple duties here.  In my opinion
it'd be better to

--8<---------------cut here---------------start------------->8---
(define (file-like-name file)
  ((match file
     ((? local-file?) local-file-name)
     ((? plain-file?) plain-file-name)
     ((? computed-file?) computed-file-name)
     [...]
     (_ (const #f))) ; alternatively raise an error
   file))
--8<---------------cut here---------------end--------------->8---
That at least decouples it from the burden of having to check whether
it is a valid pulseaudio script file name, which makes it reusable
elsewhere.  As a note regarding correctness, perhaps we should write an
implementation in (guix gexp) that works for everything that has a
gexp-compiler, but that's out of scope for now.

On the pulseaudio side, you'd compose that with an
--8<---------------cut here---------------start------------->8---
(define (assert-pulseaudio-script-file-name name)
  (unless name (raise ...))
  (unless (string-suffix? name ".pa") (leave ...))
  name)
--8<---------------cut here---------------end--------------->8---

Cheers

[bug#53676] [PATCH 0/5] *** PulseAudio service improvements ***

[Liliana Marie Prikler]

Hi Maxim,

Am Sonntag, dem 06.02.2022 um 01:30 -0500 schrieb Maxim Cournoyer:
> > > With the proposed 'extra-script-files', I'd argue that appending
> > > something to default.pa should be considered an anti-pattern; as
> > > the new field would be the more natural option to *extend*
> > > 'default.pa' (and having a field to override default.pa is still
> > > useful if you don't like any of the default behavior).
> > I don't think you're making a good case here.  Why do you want
> > appending to default.pa to be an anti-pattern?
> 
> Basically, to keep things as simple as they can be.  I'm expecting
> that extending the default.pa file must be a more common use case
> than hacking it up, justifying the 'extra-script-files' simple entry
> point catered for this use case.  Compare:
> 
> --8<---------------cut here---------------start------------->8---
> (script-file (computed-file "default.pa"
>                             #~(begin
>                                 (copy-file #$(file-apend pulseaudio
> "/etc/default.pa")
>                                            #$output)
>                                 (call-with-port
>                                   (open-file #$output "a")
>                                     (lambda (port)
>                                       (format port "~%\
> set-card-profile alsa_card.pci-0000_01_01.0 output:analog-surround-
> 40+input:analog-mono
> set-default-source alsa_input.pci-0000_01_01.0.analog-mono
> set-default-sink alsa_output.pci-0000_01_01.0.analog-surround-
> 40~%"))))))
> --8<---------------cut here---------------end--------------->8---
> 
> to:
> 
> --8<---------------cut here---------------start------------->8---
> (extra-script-files
>   (list (plain-file "configure-audigy-card"
>           (string-append "\
> set-card-profile alsa_card.pci-0000_01_01.0 output:analog-surround-
> 40+input:analog-mono
> set-default-source alsa_input.pci-0000_01_01.0.analog-mono
> set-default-sink alsa_output.pci-0000_01_01.0.analog-surround-40\n"))))
> --8<---------------cut here---------------end--------------->8---

to:

--8<---------------cut here---------------start------------->8---
(script-file 
  (mixed-text-file "default.pa"
                   ".include"
                   ;; (pulseaudio-configuration-script-file service)
                   (file-append pulseaudio "/etc/default.pa")
                   "
set-card-profile alsa_card.pci-0000_01_01.0
output:analog-surround-40+input:analog-mono
set-default-source alsa_input.pci-0000_01_01.0.analog-mono
set-default-sink
alsa_output.pci-0000_01_01.0.analog-surround-40\n"))
--8<---------------cut here---------------end--------------->8---

which yields

--8<---------------cut here---------------start------------->8---
.include /gnu/store/7xwgz4bavb1i8sfx1lm55hlrr3ngjkdx-pulseaudio-
15.0/etc/default.pa
set-card-profile alsa_card.pci-0000_01_01.0
output:analog-surround-40+input:analog-mono
set-default-source alsa_input.pci-0000_01_01.0.analog-mono
set-default-sink
alsa_output.pci-0000_01_01.0.analog-surround-40
--8<---------------cut here---------------end--------------->8---

> The later seems simpler, especially for someone starting with Guix and
> not very familiar with Guile and G-expressions.
That's because you're (intentionally or otherwise) not making a fair
comparison.  The way I wrote above is the way I intended pulseaudio-
service-type to be used and it's in terms of writing the pulseaudio
configuration not that much harder than what you are proposing.  It'd
be trivial to add a clause to ".include /etc/pulse/default.pa.d"
through the service configuration layer.  Also with pulseaudio < 15.0,
you could – after groking gexps a little – produce 

--8<---------------cut here---------------start------------->8---
.include /gnu/store/7xwgz4bavb1i8sfx1lm55hlrr3ngjkdx-pulseaudio-
15.0/etc/default.pa
.include /gnu/store/12345678901234567890123456789012-audigy-card.pa
.include /gnu/store/12345678901234567890123456789013-other-stuff.pa
[...]
--8<---------------cut here---------------end--------------->8---

inside that mixed-text-file.  With pulseaudio 15.0, you can also

(define my-pulseaudio-extra-config
  (directory-union ...))

and use it like

--8<---------------cut here---------------start------------->8---
(script-file 
  (mixed-text-file "default.pa"
                   ".include"
                   (file-append pulseaudio "/etc/default.pa")
                   ".include" my-pulseaudio-extra-config))
--8<---------------cut here---------------end--------------->8---
> 

> > I still don't agree that that's a good idea, however. 
> > Particularly, it would lead to including files from an "old distro"
> > that was infected with Guix when that probably wasn't asked for. 
> > If at all enabled, I'd prefer if pulseaudio-service-type magically
> > inserted that snippet for configurations that add files to
> > default.pa.d.
> 
> There are pros and cons; people might be find it handy that a
> Guix-installed pulseaudio also honors their user scripts living under
> /etc/pulse/default.pa.d.  It seems low risk to me; not worth the
> extra complexity in my opinion.
Note that you are introducing extra complexity to create that directory
from pulseaudio-service-type.  In particular, I don't see how your
solution is a notable improvement over mixed-text-file, which to me
seems better suited towards this purpose.

Cheers

[bug#53676] [PATCH 0/5] *** PulseAudio service improvements ***

[Maxim Cournoyer]

Hi Jack,

Jack Hill <jackhill@jackhill.us> writes:

> On Tue, 1 Feb 2022, Liliana Marie Prikler wrote:
>
>> Hi,
>>
>> Am Montag, dem 31.01.2022 um 23:19 -0500 schrieb Maxim Cournoyer:
>>> * gnu/services/sound.scm (pulseaudio-environment)
>>> [PULSE_CONFIG, PULSE_CLIENTCONFIG]: Use fix locations, and move logic
>>> to...
>>> (pulseaudio-etc): ... this service extension.  Guard against producing
>>> empty files.
>>
>> This patch reproduces (more or less) the initial layout we had for
>> pulseaudio-service-type.  However, that layout has been reported to not
>> work with some sandboxes.  I tried tracking down a specific bug, but
>> could only gather <https://issues.guix.gnu.org/42118#3>.
>>
>>> Due to a bug with webkit sandboxing, we no longer put daemon.conf
>>> into /etc/pulse (my bad), but rather set PULSE_CONFIG to directly
>>> point to it.
>>
>> In other words, we should check whether Epiphany still plays sound
>> properly with this patch applied.
>>
>> Cheers
>
> I reported the original bugs for this in Guix [0] and WebKitGTK [1],
> so it was easy for me to find the references; hope they help!
> Unfortunately, it doesn't look like the WebKitGTK bug has been fixed
> (probably waiting on a C++ hacker). Note that the symptom I saw wasn't
> just that sound didn't work, but that the sandboxed processes crashed,
> so no web content was rendered.
>
> [0] https://issues.guix.gnu.org/40837
> [1] https://bugs.webkit.org/show_bug.cgi?id=211131
>
> Unfortunately, I haven't had time to test this series.

Thanks for this!  I wasn't aware of the history; I tried it and it
failed the same.  The following fix I attempted in webkitgtk did not
seem to do anything:

--8<---------------cut here---------------start------------->8---
modified   Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
@@ -24,6 +24,7 @@
 #include <fcntl.h>
 #include <glib.h>
 #include <seccomp.h>
+#include <string.h>
 #include <sys/ioctl.h>
 #include <sys/mman.h>
 #include <unistd.h>
@@ -337,7 +338,16 @@ static void bindIfExists(Vector<CString>& args, const char* path, BindFlags bind
         bindType = "--ro-bind-try";
     else
         bindType = "--bind-try";
-    args.appendVector(Vector<CString>({ bindType, path, path }));
+
+    // Canonicalize the source path, otherwise a symbolic link could
+    // point to a location outside of the namespace.
+    char canonicalPath[PATH_MAX];
+    if (!realpath(path, canonicalPath)) {
+        if (strlen(path) + 1 > PATH_MAX)
+            return;                  // too long of a path
+        strcpy(path, canonicalPath); // no-op
+    }
+    args.appendVector(Vector<CString>({ bindType, canonicalPath, path }));
 }
 
 static void bindDBusSession(Vector<CString>& args, XDGDBusProxyLauncher& proxy)
--8<---------------cut here---------------end--------------->8---

Thanks,

Maxim

[bug#53676] [PATCH 0/5] *** PulseAudio service improvements ***

[Liliana Marie Prikler]

Hi,

Am Montag, dem 07.02.2022 um 17:29 -0500 schrieb Maxim Cournoyer:
> Thanks for this!  I wasn't aware of the history; I tried it and it
> failed the same.  The following fix I attempted in webkitgtk did not
> seem to do anything:
> 
> --8<---------------cut here---------------start------------->8---
> modified  
> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
> @@ -24,6 +24,7 @@
>  #include <fcntl.h>
>  #include <glib.h>
>  #include <seccomp.h>
> +#include <string.h>
>  #include <sys/ioctl.h>
>  #include <sys/mman.h>
>  #include <unistd.h>
> @@ -337,7 +338,16 @@ static void bindIfExists(Vector<CString>& args,
> const char* path, BindFlags bind
>          bindType = "--ro-bind-try";
>      else
>          bindType = "--bind-try";
> -    args.appendVector(Vector<CString>({ bindType, path, path }));
> +
> +    // Canonicalize the source path, otherwise a symbolic link could
> +    // point to a location outside of the namespace.
> +    char canonicalPath[PATH_MAX];
> +    if (!realpath(path, canonicalPath)) {
> +        if (strlen(path) + 1 > PATH_MAX)
> +            return;                  // too long of a path
> +        strcpy(path, canonicalPath); // no-op
> +    }
> +    args.appendVector(Vector<CString>({ bindType, canonicalPath,
> path }));
>  }
Apart from raw char arrays and string.h looking funny (and wrong) in
C++, what is strcpy supposed to do here?  Would it work if we mapped
canonicalPath to path (i.e. `ls path' in the container would be `ls
canonicalPath' under the hood)?

Cheers

[bug#53676] [PATCH 0/5] *** PulseAudio service improvements ***

[Maxime Devos]

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

Maxim Cournoyer schreef op ma 07-02-2022 om 17:29 [-0500]:
> +    char canonicalPath[PATH_MAX];

PATH_MAX does not exist on the Hurd, see
<https://www.gnu.org/software/hurd//community/gsoc/project_ideas/maxpath.html>.
Also, according to <https://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html>
more a kind of minimum and not really a maximum; apparently
most uses of PATH_MAX are wrong.

<https://www.gnu.org/software/hurd//hurd/porting/guidelines.html> recommends a
geometrically growing series.

Greetings,
Maxime.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#53676] [PATCH 0/5] *** PulseAudio service improvements ***

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Hi,
>
> Am Montag, dem 07.02.2022 um 17:29 -0500 schrieb Maxim Cournoyer:
>> Thanks for this!  I wasn't aware of the history; I tried it and it
>> failed the same.  The following fix I attempted in webkitgtk did not
>> seem to do anything:
>> 
>> --8<---------------cut here---------------start------------->8---
>> modified  
>> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
>> @@ -24,6 +24,7 @@
>>  #include <fcntl.h>
>>  #include <glib.h>
>>  #include <seccomp.h>
>> +#include <string.h>
>>  #include <sys/ioctl.h>
>>  #include <sys/mman.h>
>>  #include <unistd.h>
>> @@ -337,7 +338,16 @@ static void bindIfExists(Vector<CString>& args,
>> const char* path, BindFlags bind
>>          bindType = "--ro-bind-try";
>>      else
>>          bindType = "--bind-try";
>> -    args.appendVector(Vector<CString>({ bindType, path, path }));
>> +
>> +    // Canonicalize the source path, otherwise a symbolic link could
>> +    // point to a location outside of the namespace.
>> +    char canonicalPath[PATH_MAX];
>> +    if (!realpath(path, canonicalPath)) {
>> +        if (strlen(path) + 1 > PATH_MAX)
>> +            return;                  // too long of a path
>> +        strcpy(path, canonicalPath); // no-op
>> +    }
>> +    args.appendVector(Vector<CString>({ bindType, canonicalPath,
>> path }));
>>  }
> Apart from raw char arrays and string.h looking funny (and wrong) in
> C++, what is strcpy supposed to do here?  Would it work if we mapped
> canonicalPath to path (i.e. `ls path' in the container would be `ls
> canonicalPath' under the hood)?

I first went the C++ solution, which is std::filesystem::canonical, but
was suggested in #webkitgtk (on the GNOME IRC server) to use the POSIX
realpath, already in use in that file, upon finding out that their build
system is configured to disallow the use of exceptions
(-fno-exceptions).  I refined the experiment as:

--8<---------------cut here---------------start------------->8---
diff --git a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
index 0d5dd4f6986d..1512b73a985d 100644
--- a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
+++ b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
@@ -325,6 +325,18 @@ enum class BindFlags {
     Device,
 };
 
+static void bindSymlinksRealPath(Vector<CString>& args, const char* path,
+                                 const char* bindOption = "--ro-bind")
+{
+    char realPath[PATH_MAX];
+
+    if (realpath(path, realPath) && strcmp(path, realPath)) {
+        args.appendVector(Vector<CString>({
+            bindOption, realPath, realPath,
+        }));
+    }
+}
+
 static void bindIfExists(Vector<CString>& args, const char* path, BindFlags bindFlags = BindFlags::ReadOnly)
 {
     if (!path || path[0] == '\0')
@@ -337,6 +349,10 @@ static void bindIfExists(Vector<CString>& args, const char* path, BindFlags bind
         bindType = "--ro-bind-try";
     else
         bindType = "--bind-try";
+
+    // Canonicalize the source path, otherwise a symbolic link could
+    // point to a location outside of the namespace.
+    bindSymlinksRealPath(args, path, bindType);
     args.appendVector(Vector<CString>({ bindType, path, path }));
 }
 
@@ -615,17 +631,6 @@ static void bindV4l(Vector<CString>& args)
     }));
 }
 
-static void bindSymlinksRealPath(Vector<CString>& args, const char* path)
-{
-    char realPath[PATH_MAX];
-
-    if (realpath(path, realPath) && strcmp(path, realPath)) {
-        args.appendVector(Vector<CString>({
-            "--ro-bind", realPath, realPath,
-        }));
-    }
-}
-
 // Translate a libseccomp error code into an error message. libseccomp
 // mostly returns negative errno values such as -ENOMEM, but some
 // standard errno values are used for non-standard purposes where their
 --8<---------------cut here---------------end--------------->8---

Which produced the intended bwrap arguments, but unfortunately that'd
still fail.  The issue seems to be related to attempt to bind
/etc/pulse/client.conf over something already existing there; it can be
simply reproduced with:

--8<---------------cut here---------------start------------->8---
$ guix shell bubblewrap -- bwrap --ro-bind /gnu /gnu \
    --ro-bind /etc /etc \
    --ro-bind /etc/pulse/client.conf /etc/pulse/client.conf \
    /gnu/store/4y5m9lb8k3qkb1y9m02sw9w9a6hacd16-bash-minimal-5.1.8/bin/bash
bwrap: Can't create file at /etc/pulse/client.conf: No such file or directory
--8<---------------cut here---------------end--------------->8---

One thing to try would be to not bind mount client.conf; /etc/ is
already bind mounted as a whole.  If the resolved paths are all bind
mounted (which they are since we share the whole of /gnu), we should be
OK.

Alternatively we could try to bind only the resolved paths, and rewrite
the environment variables such as PULSE_CLIENTCONFIG at run time in
webkitgtk such that it points to the resolved destination.

To be continued...

Maxim

[bug#53676] [PATCH 0/5] *** PulseAudio service improvements ***

[Maxim Cournoyer]

Hi Maxime,

Maxime Devos <maximedevos@telenet.be> writes:

> Maxim Cournoyer schreef op ma 07-02-2022 om 17:29 [-0500]:
>> +    char canonicalPath[PATH_MAX];
>
> PATH_MAX does not exist on the Hurd, see
> <https://www.gnu.org/software/hurd//community/gsoc/project_ideas/maxpath.html>.
> Also, according to <https://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html>
> more a kind of minimum and not really a maximum; apparently
> most uses of PATH_MAX are wrong.
>
> <https://www.gnu.org/software/hurd//hurd/porting/guidelines.html> recommends a
> geometrically growing series.

Interesting!  Note that PATH_MAX is already in use in that
BubblewrapLauncher.cpp file.

Thanks for the links!

Maxim

[bug#53676] [PATCH 0/5] *** PulseAudio service improvements ***

[Maxim Cournoyer]

Hi again,

I forgot to mention; apparently the best/complete way to setup
PulseAudio in a BubbleWrap container is as flatpak does it; I was
pointed to this code:
https://github.com/flatpak/flatpak/blob/4470bf142523e8a9bd6791880a66676225dea555/common/flatpak-run.c#L725,
but I haven't had time to review it much yet.

Thanks,

Maxim

[bug#53676] [PATCH 0/5] *** PulseAudio service improvements ***

[Liliana Marie Prikler]

Hi,

Am Dienstag, dem 08.02.2022 um 09:25 -0500 schrieb Maxim Cournoyer:
> Hi Liliana,
> 
> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
> 
> > Hi,
> > 
> > Am Montag, dem 07.02.2022 um 17:29 -0500 schrieb Maxim Cournoyer:
> > > Thanks for this!  I wasn't aware of the history; I tried it and
> > > it
> > > failed the same.  The following fix I attempted in webkitgtk did
> > > not
> > > seem to do anything:
> > > 
> > > --8<---------------cut here---------------start------------->8---
> > > modified  
> > > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
> > > @@ -24,6 +24,7 @@
> > >  #include <fcntl.h>
> > >  #include <glib.h>
> > >  #include <seccomp.h>
> > > +#include <string.h>
> > >  #include <sys/ioctl.h>
> > >  #include <sys/mman.h>
> > >  #include <unistd.h>
> > > @@ -337,7 +338,16 @@ static void bindIfExists(Vector<CString>&
> > > args,
> > > const char* path, BindFlags bind
> > >          bindType = "--ro-bind-try";
> > >      else
> > >          bindType = "--bind-try";
> > > -    args.appendVector(Vector<CString>({ bindType, path, path
> > > }));
> > > +
> > > +    // Canonicalize the source path, otherwise a symbolic link
> > > could
> > > +    // point to a location outside of the namespace.
> > > +    char canonicalPath[PATH_MAX];
> > > +    if (!realpath(path, canonicalPath)) {
> > > +        if (strlen(path) + 1 > PATH_MAX)
> > > +            return;                  // too long of a path
> > > +        strcpy(path, canonicalPath); // no-op
> > > +    }
> > > +    args.appendVector(Vector<CString>({ bindType, canonicalPath,
> > > path }));
> > >  }
> > Apart from raw char arrays and string.h looking funny (and wrong)
> > in
> > C++, what is strcpy supposed to do here?  Would it work if we
> > mapped
> > canonicalPath to path (i.e. `ls path' in the container would be `ls
> > canonicalPath' under the hood)?
> 
> I first went the C++ solution, which is std::filesystem::canonical,
> but was suggested in #webkitgtk (on the GNOME IRC server) to use the
> POSIX realpath, already in use in that file, upon finding out that
> their build system is configured to disallow the use of exceptions
> (-fno-exceptions).  I refined the experiment as:
> 
> --8<---------------cut here---------------start------------->8---
> diff --git
> a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
> b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
> index 0d5dd4f6986d..1512b73a985d 100644
> --- a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
> +++ b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
> @@ -325,6 +325,18 @@ enum class BindFlags {
>      Device,
>  };
>  
> +static void bindSymlinksRealPath(Vector<CString>& args, const char*
> path,
> +                                 const char* bindOption = "--ro-
> bind")
> +{
> +    char realPath[PATH_MAX];
> +
> +    if (realpath(path, realPath) && strcmp(path, realPath)) {
> +        args.appendVector(Vector<CString>({
> +            bindOption, realPath, realPath,
> +        }));
> +    }
> +}
> +
>  static void bindIfExists(Vector<CString>& args, const char* path,
> BindFlags bindFlags = BindFlags::ReadOnly)
>  {
>      if (!path || path[0] == '\0')
> @@ -337,6 +349,10 @@ static void bindIfExists(Vector<CString>& args,
> const char* path, BindFlags bind
>          bindType = "--ro-bind-try";
>      else
>          bindType = "--bind-try";
> +
> +    // Canonicalize the source path, otherwise a symbolic link could
> +    // point to a location outside of the namespace.
> +    bindSymlinksRealPath(args, path, bindType);
>      args.appendVector(Vector<CString>({ bindType, path, path }));
>  }
>  
> @@ -615,17 +631,6 @@ static void bindV4l(Vector<CString>& args)
>      }));
>  }
>  
> -static void bindSymlinksRealPath(Vector<CString>& args, const char*
> path)
> -{
> -    char realPath[PATH_MAX];
> -
> -    if (realpath(path, realPath) && strcmp(path, realPath)) {
> -        args.appendVector(Vector<CString>({
> -            "--ro-bind", realPath, realPath,
> -        }));
> -    }
> -}
> -
>  // Translate a libseccomp error code into an error message.
> libseccomp
>  // mostly returns negative errno values such as -ENOMEM, but some
>  // standard errno values are used for non-standard purposes where
> their
>  --8<---------------cut here---------------end--------------->8---
Note that Webkit has a FileSystem namespace with a realPath function
that does std::filesystem::canonical already.

> Which produced the intended bwrap arguments, but unfortunately that'd
> still fail.  The issue seems to be related to attempt to bind
> /etc/pulse/client.conf over something already existing there; it can
> be simply reproduced with:
> 
> --8<---------------cut here---------------start------------->8---
> $ guix shell bubblewrap -- bwrap --ro-bind /gnu /gnu \
>     --ro-bind /etc /etc \
>     --ro-bind /etc/pulse/client.conf /etc/pulse/client.conf \
>     /gnu/store/4y5m9lb8k3qkb1y9m02sw9w9a6hacd16-bash-minimal-
> 5.1.8/bin/bash
> bwrap: Can't create file at /etc/pulse/client.conf: No such file or
> directory
> --8<---------------cut here---------------end--------------->8---
> 
> One thing to try would be to not bind mount client.conf; /etc/ is
> already bind mounted as a whole.  If the resolved paths are all bind
> mounted (which they are since we share the whole of /gnu), we should
> be OK.
Do we really need to bind all of /etc?  I think it'd make sense to try
and shrink that.

Not bind-mounting it is not a solution IIUC.  At least it appears that
is the standard quo in which the symlink is unresolved.  I think
bubblewrap simply ignores symlinks due to their inherent TOCTOU
characteristics and other "fun" things one could do with them.

> [...]
> 
> To be continued...

[bug#53676] [PATCH 5/5] services: pulseaudio: Deploy the configuration files to /etc/pulse.

[Maxim Cournoyer]

Hi Jack, Liliana,

Jack Hill <jackhill@jackhill.us> writes:

> On Tue, 1 Feb 2022, Liliana Marie Prikler wrote:
>
>> Hi,
>>
>> Am Montag, dem 31.01.2022 um 23:19 -0500 schrieb Maxim Cournoyer:
>>> * gnu/services/sound.scm (pulseaudio-environment)
>>> [PULSE_CONFIG, PULSE_CLIENTCONFIG]: Use fix locations, and move logic
>>> to...
>>> (pulseaudio-etc): ... this service extension.  Guard against producing
>>> empty files.
>>
>> This patch reproduces (more or less) the initial layout we had for
>> pulseaudio-service-type.  However, that layout has been reported to not
>> work with some sandboxes.  I tried tracking down a specific bug, but
>> could only gather <https://issues.guix.gnu.org/42118#3>.
>>
>>> Due to a bug with webkit sandboxing, we no longer put daemon.conf
>>> into /etc/pulse (my bad), but rather set PULSE_CONFIG to directly
>>> point to it.

Our webkitgtk has not been patched so that keeping PULSE_CLIENTCONFIG
pointing to /etc should work, unblocking this series.

I'll now address the remaining comments from Liliana and send an updated
series.

Thanks,

Maxim

[bug#53676] [PATCH 4/5] services: pulseaudio: Add an extra-script-files configuration field.

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

[...]

> Note that file-like->name serves multiple duties here.  In my opinion
> it'd be better to
>
> (define (file-like-name file)
>   ((match file
>      ((? local-file?) local-file-name)
>      ((? plain-file?) plain-file-name)
>      ((? computed-file?) computed-file-name)
>      [...]
>      (_ (const #f))) ; alternatively raise an error
>    file))
>
> That at least decouples it from the burden of having to check whether
> it is a valid pulseaudio script file name, which makes it reusable
> elsewhere.

I've modified it like so:

--8<---------------cut here---------------start------------->8---
modified   gnu/services/sound.scm
@@ -154,21 +154,24 @@ (define (extra-script-files->file-union extra-script-files)
   "Return a G-exp obtained by processing EXTRA-SCRIPT-FILES with FILE-UNION."

   (define (file-like->name file)
-    (let ((name (match file
-                  ((? local-file?)
-                   (local-file-name file))
-                  ((? plain-file?)
-                   (plain-file-name file))
-                  ((? computed-file?)
-                   (computed-file-name file))
-                  (_ (leave (G_ "~a is not a local-file, plain-file or \
-computed-file object~%") file)))))
-      (unless (string-suffix? ".pa" name)
-        (leave (G_ "`~a' lacks the required `.pa' file name extension~%")
-               name))
-      name))
-
-  (let ((labels (map file-like->name extra-script-files)))
+    (match file
+      ((? local-file?)
+       (local-file-name file))
+      ((? plain-file?)
+       (plain-file-name file))
+      ((? computed-file?)
+       (computed-file-name file))
+      (_ (leave (G_ "~a is not a local-file, plain-file or \
+computed-file object~%") file))))
+
+  (define (assert-pulseaudio-script-file-name name)
+    (unless (string-suffix? ".pa" name)
+      (leave (G_ "`~a' lacks the required `.pa' file name extension~%") name))
+    name)
+
+  (let ((labels (map (compose assert-pulseaudio-script-file-name)
+                              file-like->name
+                     extra-script-files)))
     (file-union "default.pa.d" (zip labels extra-script-files))))
--8<---------------cut here---------------end--------------->8---

Thanks for the suggestion.

I'll be sending a v2 series soon.

Maxim

[bug#53676] [PATCH 0/5] *** PulseAudio service improvements ***

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

[...]

> The way I wrote above is the way I intended pulseaudio- service-type
> to be used and it's in terms of writing the pulseaudio configuration
> not that much harder than what you are proposing.  It'd be trivial to
> add a clause to ".include /etc/pulse/default.pa.d" through the service
> configuration layer.  Also with pulseaudio < 15.0, you could – after
> groking gexps a little – produce
>
> .include /gnu/store/7xwgz4bavb1i8sfx1lm55hlrr3ngjkdx-pulseaudio-
> 15.0/etc/default.pa
> .include /gnu/store/12345678901234567890123456789012-audigy-card.pa
> .include /gnu/store/12345678901234567890123456789013-other-stuff.pa
> [...]
>
>
> inside that mixed-text-file.  With pulseaudio 15.0, you can also
>
> (define my-pulseaudio-extra-config
>   (directory-union ...))
>
> and use it like
>
> (script-file
>   (mixed-text-file "default.pa"
>                    ".include"
>                    (file-append pulseaudio "/etc/default.pa")
>                    ".include" my-pulseaudio-extra-config))

That is nice, but is still a bit more demanding from users:

1. They need to know how to compose multiple G-Exps expressions such as
mixed-text-file and file-append.
2. They need to know to use ".include" directives from PulseAudio.

My proposed change reduces the knowledge needed to just a single usage
of a G-Exp file-like object, such as plain-file or local-file; I think
that's a bit easier to grok for starters.  The resulting configuration
is also easy to inspect; it's all under /etc/pulse as the user would
expect.

Thanks,

Maxim

[bug#53676] [PATCH 0/5] *** PulseAudio service improvements ***

[Maxim Cournoyer]

Hello,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> Hi Maxime,
>
> Maxime Devos <maximedevos@telenet.be> writes:
>
>> Maxim Cournoyer schreef op ma 07-02-2022 om 17:29 [-0500]:
>>> +    char canonicalPath[PATH_MAX];
>>
>> PATH_MAX does not exist on the Hurd, see
>> <https://www.gnu.org/software/hurd//community/gsoc/project_ideas/maxpath.html>.
>> Also, according to <https://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html>
>> more a kind of minimum and not really a maximum; apparently
>> most uses of PATH_MAX are wrong.
>>
>> <https://www.gnu.org/software/hurd//hurd/porting/guidelines.html> recommends a
>> geometrically growing series.
>
> Interesting!  Note that PATH_MAX is already in use in that
> BubblewrapLauncher.cpp file.

For your information, the upstreamed patch [0] doesn't use PATH_MAX
anymore; it uses the FileSystem::realPath procedure of WebKit.  Thanks
to Liliana for mentioning its existence!

[0]  https://bugs.webkit.org/show_bug.cgi?id=211131

Maxim

[bug#53676] [PATCH v2 1/4] services/sound: Normalize pulseaudio-configuration accessor names.

[Maxim Cournoyer]

* gnu/services/sound.scm (<pulseaudio-configuration>): Adjust getter names to
match convention.
---
 gnu/services/sound.scm | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
index 03e62a1e36..9684e06d13 100644
--- a/gnu/services/sound.scm
+++ b/gnu/services/sound.scm
@@ -2,6 +2,7 @@
 ;;; Copyright © 2018, 2020 Oleg Pykhalov <go.wigust@gmail.com>
 ;;; Copyright © 2020 Liliana Marie Prikler <liliana.prikler@gmail.com>
 ;;; Copyright © 2020 Marius Bakke <mbakke@fastmail.com>
+;;; Copyright © 2022 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -115,16 +116,16 @@ (define alsa-service-type
 (define-record-type* <pulseaudio-configuration>
   pulseaudio-configuration make-pulseaudio-configuration
   pulseaudio-configuration?
-  (client-conf pulseaudio-client-conf
+  (client-conf pulseaudio-configuration-client-conf
                (default '()))
-  (daemon-conf pulseaudio-daemon-conf
+  (daemon-conf pulseaudio-configuration-daemon-conf
                ;; Flat volumes may cause unpleasant experiences to users
                ;; when applications inadvertently max out the system volume
                ;; (see e.g. <https://bugs.gnu.org/38172>).
                (default '((flat-volumes . no))))
-  (script-file pulseaudio-script-file
+  (script-file pulseaudio-configuration-script-file
                (default (file-append pulseaudio "/etc/pulse/default.pa")))
-  (system-script-file pulseaudio-system-script-file
+  (system-script-file pulseaudio-configuration-system-script-file
                       (default
                         (file-append pulseaudio "/etc/pulse/system.pa"))))
 
-- 
2.34.0

[bug#53676] [PATCH v2 2/4] gnu: pulseaudio: Graft to adjust configuration.

[Maxim Cournoyer]

* gnu/packages/pulseaudio.scm (pulseaudio/fixed): New variable.
(pulseaudio)[replacement]: Graft package with it.
---
 gnu/packages/pulseaudio.scm | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/gnu/packages/pulseaudio.scm b/gnu/packages/pulseaudio.scm
index fe028b5202..c1b3d33d4a 100644
--- a/gnu/packages/pulseaudio.scm
+++ b/gnu/packages/pulseaudio.scm
@@ -178,6 +178,7 @@ (define-public libsamplerate
 (define-public pulseaudio
   (package
     (name "pulseaudio")
+    (replacement pulseaudio/fixed)
     (version "15.0")
     (source (origin
              (method url-fetch)
@@ -269,6 +270,22 @@ (define-public pulseaudio
     ;; 'LICENSE' for details.
     (license l:gpl2+)))
 
+(define pulseaudio/fixed
+  (package
+    (inherit pulseaudio)
+    (arguments
+     (substitute-keyword-arguments (package-arguments pulseaudio)
+       ((#:phases phases)
+        `(modify-phases ,phases
+           (add-after 'unpack 'customize-default-script
+             (lambda _
+               (substitute* "src/daemon/default.pa.in"
+                 (("^\\.include.*default.pa.d.*" anchor)
+                  (string-append
+                   ;; Honor PulseAudio script extensions found under
+                   ;; /etc/pulse/default.pa.d.
+                   anchor ".include /etc/pulse/default.pa.d\n")))))))))))
+
 (define-public pavucontrol
   (package
     (name "pavucontrol")
-- 
2.34.0

[bug#53676] [PATCH v2 3/4] services: pulseaudio: Add an extra-script-files configuration field.

[Maxim Cournoyer]

* gnu/services/sound.scm (<pulseaudio-configuration>)
[extra-script-files]: Add field.
(extra-script-files->file-union): Add procedure.
(pulseaudio-etc): Use it.
* doc/guix.texi: Document it.
---
 doc/guix.texi          | 30 ++++++++++++++++++++++++++++++
 gnu/services/sound.scm | 38 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index f336c26e8a..9941be5033 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -21509,9 +21509,39 @@ List of settings to set in @file{daemon.conf}, formatted just like
 @item @code{script-file} (default: @code{(file-append pulseaudio "/etc/pulse/default.pa")})
 Script file to use as @file{default.pa}.
 
+@item @code{extra-script-files} (default: @code{'())})
+A list of file-like objects defining extra PulseAudio scripts to run at
+the initialization of the @command{pulseaudio} daemon, after the main
+@code{script-file}.  The scripts are deployed to the
+@file{/etc/pulse/default.pa.d} directory; they should have the
+@samp{.pa} file name extension.  For a reference of the available
+commands, refer to @command{man pulse-cli-syntax}.
+
 @item @code{system-script-file} (default: @code{(file-append pulseaudio "/etc/pulse/system.pa")})
 Script file to use as @file{system.pa}.
 @end table
+
+The example below sets the default PulseAudio card profile, the default
+sink and the default source to use for a old SoundBlaster Audigy sound
+card:
+@lisp
+(pulseaudio-configuration
+ (extra-script-files
+  (list (plain-file "audigy.pa"
+                    (string-append "\
+set-card-profile alsa_card.pci-0000_01_01.0 \
+  output:analog-surround-40+input:analog-mono
+set-default-source alsa_input.pci-0000_01_01.0.analog-mono
+set-default-sink alsa_output.pci-0000_01_01.0.analog-surround-40\n")))))
+@end lisp
+
+Note that @code{pulseaudio-service-type} is part of
+@code{%desktop-services}; if your operating system declaration was
+derived from one of the desktop templates, you'll want to adjust the
+above example to modify the existing @code{pulseaudio-service-type} via
+@code{modify-services} (@pxref{Service Reference,
+@code{modify-services}}), instead of defining a new one.
+
 @end deftp
 
 @deffn {Scheme Variable} ladspa-service-type
diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
index 9684e06d13..eecea1a733 100644
--- a/gnu/services/sound.scm
+++ b/gnu/services/sound.scm
@@ -26,14 +26,17 @@ (define-module (gnu services sound)
   #:use-module (gnu services)
   #:use-module (gnu system pam)
   #:use-module (gnu system shadow)
+  #:use-module (guix diagnostics)
   #:use-module (guix gexp)
   #:use-module (guix packages)
   #:use-module (guix records)
   #:use-module (guix store)
+  #:use-module (guix ui)
   #:use-module (gnu packages audio)
   #:use-module (gnu packages linux)
   #:use-module (gnu packages pulseaudio)
   #:use-module (ice-9 match)
+  #:use-module (srfi srfi-1)
   #:export (alsa-configuration
             alsa-service-type
 
@@ -125,6 +128,8 @@ (define-record-type* <pulseaudio-configuration>
                (default '((flat-volumes . no))))
   (script-file pulseaudio-configuration-script-file
                (default (file-append pulseaudio "/etc/pulse/default.pa")))
+  (extra-script-files pulseaudio-configuration-extra-script-files
+                      (default '()))
   (system-script-file pulseaudio-configuration-system-script-file
                       (default
                         (file-append pulseaudio "/etc/pulse/system.pa"))))
@@ -145,14 +150,43 @@ (define pulseaudio-environment
        ("PULSE_CLIENTCONFIG" . ,(apply mixed-text-file "client.conf"
                                        (map pulseaudio-conf-entry client-conf)))))))
 
+(define (extra-script-files->file-union extra-script-files)
+  "Return a G-exp obtained by processing EXTRA-SCRIPT-FILES with FILE-UNION."
+
+  (define (file-like->name file)
+    (match file
+      ((? local-file?)
+       (local-file-name file))
+      ((? plain-file?)
+       (plain-file-name file))
+      ((? computed-file?)
+       (computed-file-name file))
+      (_ (leave (G_ "~a is not a local-file, plain-file or \
+computed-file object~%") file))))
+
+  (define (assert-pulseaudio-script-file-name name)
+    (unless (string-suffix? ".pa" name)
+      (leave (G_ "`~a' lacks the required `.pa' file name extension~%") name))
+    name)
+
+  (let ((labels (map (compose assert-pulseaudio-script-file-name
+                              file-like->name)
+                     extra-script-files)))
+    (file-union "default.pa.d" (zip labels extra-script-files))))
+
 (define pulseaudio-etc
   (match-lambda
-    (($ <pulseaudio-configuration> _ _ default-script-file system-script-file)
+    (($ <pulseaudio-configuration> _ _ default-script-file extra-script-files
+                                   system-script-file)
      `(("pulse"
         ,(file-union
           "pulse"
           `(("default.pa" ,default-script-file)
-            ("system.pa" ,system-script-file))))))))
+            ("system.pa" ,system-script-file)
+            ,@(if (null? extra-script-files)
+                  '()
+                  `(("default.pa.d" ,(extra-script-files->file-union
+                                      extra-script-files)))))))))))
 
 (define pulseaudio-service-type
   (service-type
-- 
2.34.0

[bug#53676] [PATCH v2 4/4] services: pulseaudio: Deploy the configuration files to /etc/pulse.

[Maxim Cournoyer]

* gnu/services/sound.scm (pulseaudio-environment)
[PULSE_CONFIG, PULSE_CLIENTCONFIG]: Use fix locations, and move logic to...
(pulseaudio-etc): ... this service extension.  Guard against producing empty
files.
---
 gnu/services/sound.scm | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/gnu/services/sound.scm b/gnu/services/sound.scm
index eecea1a733..336f6c39a0 100644
--- a/gnu/services/sound.scm
+++ b/gnu/services/sound.scm
@@ -144,11 +144,11 @@ (define (pulseaudio-conf-entry arg)
 (define pulseaudio-environment
   (match-lambda
     (($ <pulseaudio-configuration> client-conf daemon-conf default-script-file)
-     `(("PULSE_CONFIG" . ,(apply mixed-text-file "daemon.conf"
-                                 "default-script-file = " default-script-file "\n"
-                                 (map pulseaudio-conf-entry daemon-conf)))
-       ("PULSE_CLIENTCONFIG" . ,(apply mixed-text-file "client.conf"
-                                       (map pulseaudio-conf-entry client-conf)))))))
+     ;; These config files kept at a fixed location, so that the following
+     ;; environment values are stable and do not require the user to reboot to
+     ;; effect their PulseAudio configuration changes.
+     '(("PULSE_CONFIG" . "/etc/pulse/daemon.conf")
+       ("PULSE_CLIENTCONFIG" . "/etc/pulse/client.conf")))))
 
 (define (extra-script-files->file-union extra-script-files)
   "Return a G-exp obtained by processing EXTRA-SCRIPT-FILES with FILE-UNION."
@@ -176,8 +176,8 @@ (define (assert-pulseaudio-script-file-name name)
 
 (define pulseaudio-etc
   (match-lambda
-    (($ <pulseaudio-configuration> _ _ default-script-file extra-script-files
-                                   system-script-file)
+    (($ <pulseaudio-configuration> client-conf daemon-conf default-script-file
+                                   extra-script-files system-script-file)
      `(("pulse"
         ,(file-union
           "pulse"
@@ -186,7 +186,18 @@ (define pulseaudio-etc
             ,@(if (null? extra-script-files)
                   '()
                   `(("default.pa.d" ,(extra-script-files->file-union
-                                      extra-script-files)))))))))))
+                                      extra-script-files))))
+            ,@(if (null? daemon-conf)
+                  '()
+                  `(("daemon.conf"
+                     ,(apply mixed-text-file "daemon.conf"
+                             "default-script-file = " default-script-file "\n"
+                             (map pulseaudio-conf-entry daemon-conf)))))
+            ,@(if (null? client-conf)
+                  '()
+                  `(("client.conf"
+                     ,(apply mixed-text-file "client.conf"
+                             (map pulseaudio-conf-entry client-conf))))))))))))
 
 (define pulseaudio-service-type
   (service-type
-- 
2.34.0

[bug#53676] [PATCH v2 3/4] services: pulseaudio: Add an extra-script-files configuration field.

[Maxime Devos]

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

Maxim Cournoyer schreef op do 24-02-2022 om 11:38 [-0500]:
> +  (define (file-like->name file)
> +    (match file
> +      ((? local-file?)
> +       (local-file-name file))
> +      ((? plain-file?)
> +       (plain-file-name file))
> +      ((? computed-file?)
> +       (computed-file-name file))
> +      (_ (leave (G_ "~a is not a local-file, plain-file or \
> +computed-file object~%") file))))

This would not work with things like '(file-append ...)'.
Perhaps 'extra-script-files->file-union' can be made more general
by creating a variant of 'file-union' for this use case?
Maybe something like (untested):

;; Based on 'file-union'
(define* (file-directory . files)
  ; files: (file-like1 file-like2 ...)
  (computed-file name
                 (with-imported-modules '((guix build utils))
                   (gexp
                    (begin
                      (use-modules (guix build utils))

                      (mkdir (ungexp output))
                      (chdir (ungexp output))
                      (ungexp-splicing
                       (map (lambda (source)
                              (gexp
                               (let ((target (basename source))
                                 ;; Stat the source to abort early if it does
                                 ;; not exist.
                                 (stat (ungexp source))
                                 (symlink (ungexp source) (ungexp target)))))
                            files)))))))

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#53676] [PATCH v2 2/4] gnu: pulseaudio: Graft to adjust configuration.

[Liliana Marie Prikler]

Am Donnerstag, dem 24.02.2022 um 11:38 -0500 schrieb Maxim Cournoyer:
> * gnu/packages/pulseaudio.scm (pulseaudio/fixed): New variable.
> (pulseaudio)[replacement]: Graft package with it.
> ---
>  gnu/packages/pulseaudio.scm | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/gnu/packages/pulseaudio.scm
> b/gnu/packages/pulseaudio.scm
> index fe028b5202..c1b3d33d4a 100644
> --- a/gnu/packages/pulseaudio.scm
> +++ b/gnu/packages/pulseaudio.scm
> @@ -178,6 +178,7 @@ (define-public libsamplerate
>  (define-public pulseaudio
>    (package
>      (name "pulseaudio")
> +    (replacement pulseaudio/fixed)
>      (version "15.0")
>      (source (origin
>               (method url-fetch)
> @@ -269,6 +270,22 @@ (define-public pulseaudio
>      ;; 'LICENSE' for details.
>      (license l:gpl2+)))
>  
> +(define pulseaudio/fixed
> +  (package
> +    (inherit pulseaudio)
> +    (arguments
> +     (substitute-keyword-arguments (package-arguments pulseaudio)
> +       ((#:phases phases)
> +        `(modify-phases ,phases
> +           (add-after 'unpack 'customize-default-script
> +             (lambda _
> +               (substitute* "src/daemon/default.pa.in"
> +                 (("^\\.include.*default.pa.d.*" anchor)
> +                  (string-append
> +                   ;; Honor PulseAudio script extensions found under
> +                   ;; /etc/pulse/default.pa.d.
> +                   anchor ".include
> /etc/pulse/default.pa.d\n")))))))))))
> +
I still think it'd be wiser to do this inside the code that generates
the configuration when we do fill /etc/pulse/default.pa.d given that
there's stuff to source.  At the very least, we'd avoid a graft for the
moment, but we'd also avoid some "lol, just source anything" scenarios.

Cheers

[bug#53676] [PATCH 0/5] *** PulseAudio service improvements ***

[Liliana Marie Prikler]

Hi,

Am Donnerstag, dem 24.02.2022 um 11:31 -0500 schrieb Maxim Cournoyer:
> That is nice, but is still a bit more demanding from users:
> 
> 1. They need to know how to compose multiple G-Exps expressions such
> as mixed-text-file and file-append.
> 2. They need to know to use ".include" directives from PulseAudio.
I don't think 2 counts here.  We assume the users know how to code
PulseAudio configuration script when we hand them the possibility to
edit it, so...

> My proposed change reduces the knowledge needed to just a single
> usage of a G-Exp file-like object, such as plain-file or local-file;
> I think that's a bit easier to grok for starters.  The resulting
> configuration is also easy to inspect; it's all under /etc/pulse as
> the user would expect.
That is a benefit, but I'm not sure how much of a benefit it is over
doing things "manually", particularly in terms of how config files end
up looking vs. the stuff we need to add to make that change.  At the
very least, we should ensure our internals are clean and maintainable.

Cheers

[bug#53676] [PATCH v2 2/4] gnu: pulseaudio: Graft to adjust configuration.

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Am Donnerstag, dem 24.02.2022 um 11:38 -0500 schrieb Maxim Cournoyer:
>> * gnu/packages/pulseaudio.scm (pulseaudio/fixed): New variable.
>> (pulseaudio)[replacement]: Graft package with it.

[...]

>> +(define pulseaudio/fixed
>> +  (package
>> +    (inherit pulseaudio)
>> +    (arguments
>> +     (substitute-keyword-arguments (package-arguments pulseaudio)
>> +       ((#:phases phases)
>> +        `(modify-phases ,phases
>> +           (add-after 'unpack 'customize-default-script
>> +             (lambda _
>> +               (substitute* "src/daemon/default.pa.in"
>> +                 (("^\\.include.*default.pa.d.*" anchor)
>> +                  (string-append
>> +                   ;; Honor PulseAudio script extensions found under
>> +                   ;; /etc/pulse/default.pa.d.
>> +                   anchor ".include
>> /etc/pulse/default.pa.d\n")))))))))))
>> +
> I still think it'd be wiser to do this inside the code that generates
> the configuration when we do fill /etc/pulse/default.pa.d given that
> there's stuff to source.  At the very least, we'd avoid a graft for the
> moment, but we'd also avoid some "lol, just source anything" scenarios.

Thank you for your continued feedback.  The reason I prefer this simple
substitution to a conditional one is two-fold:

1. It avoids two actors potentially touching the default 'script-file'
(the pulseaudio-service-type code as well as the user), which could be
unwieldy (do we plug the default.pa.d after their changes to ensure it
is there, or before, which means it'd potentially be erased?).  Having
it part of the shipped default.pa file makes this simpler to reason
with.

2. It allows foreign distribution users to keep their custom user script
working even when they use our pulseaudio package (it makes our
pulseaudio package behave as intended by upstream).

I wouldn't mind using a feature branch to get the < 2k dependent
packages rebuilt as suggested by Leo, if you think that's preferable.

Thanks,

Maxim

[bug#53676] [PATCH v2 3/4] services: pulseaudio: Add an extra-script-files configuration field.

[Maxim Cournoyer]

Hi Maxime,

Maxime Devos <maximedevos@telenet.be> writes:

> Maxim Cournoyer schreef op do 24-02-2022 om 11:38 [-0500]:
>> +  (define (file-like->name file)
>> +    (match file
>> +      ((? local-file?)
>> +       (local-file-name file))
>> +      ((? plain-file?)
>> +       (plain-file-name file))
>> +      ((? computed-file?)
>> +       (computed-file-name file))
>> +      (_ (leave (G_ "~a is not a local-file, plain-file or \
>> +computed-file object~%") file))))
>
> This would not work with things like '(file-append ...)'.
> Perhaps 'extra-script-files->file-union' can be made more general
> by creating a variant of 'file-union' for this use case?
> Maybe something like (untested):
>
> ;; Based on 'file-union'
> (define* (file-directory . files)
>   ; files: (file-like1 file-like2 ...)
>   (computed-file name
>                  (with-imported-modules '((guix build utils))
>                    (gexp
>                     (begin
>                       (use-modules (guix build utils))
>
>                       (mkdir (ungexp output))
>                       (chdir (ungexp output))
>                       (ungexp-splicing
>                        (map (lambda (source)
>                               (gexp
>                                (let ((target (basename source))
>                                  ;; Stat the source to abort early if it does
>                                  ;; not exist.
>                                  (stat (ungexp source))
>                                  (symlink (ungexp source) (ungexp target)))))
>                             files)))))))

Not a bad idea, but it steers a bit on the too-complicated side of
things for my taste; for one thing, I wouldn't know how to do the
validation of the file name anymore (it needs to end by ".pa").  It
could be done inside that procedure, but it'd become more tangled.

The simple file-like->name procedure above will error with an accurate
message telling the users about its limits (that it only accepts
local-file, plain-file or computed-file).

G-Exp wizards can still opt the mixed-text-file + any G-Exp
transformation they wish via the 'script-file' field.

Thanks,

Maxim

[bug#53676] [PATCH v2 2/4] gnu: pulseaudio: Graft to adjust configuration.

[Liliana Marie Prikler]

Hi Maxim,

Am Donnerstag, dem 24.02.2022 um 17:00 -0500 schrieb Maxim Cournoyer:
> Thank you for your continued feedback.  The reason I prefer this
> simple substitution to a conditional one is two-fold:
> 
> 1. It avoids two actors potentially touching the default 'script-
> file' (the pulseaudio-service-type code as well as the user), which
> could be unwieldy (do we plug the default.pa.d after their changes to
> ensure it is there, or before, which means it'd potentially be
> erased?).  Having it part of the shipped default.pa file makes this
> simpler to reason with.
Sure, but all we'd need here is proper documentation.  For the record,
I would check if a `source /etc/pulse/default.pa.d' is in the user-
supplied file (even if commented) and append it if not.

> 2. It allows foreign distribution users to keep their custom user
> script working even when they use our pulseaudio package (it makes
> our pulseaudio package behave as intended by upstream).
That ignores the case where users modify their distro's default.pa
*and* put stuff into default.pa.d.  This might be necessary in some
scenarios where the upstream default breaks user expectations.  I'd
really prefer if foreign distro users just set their environment
variables, as those work unconditionally as intended.

> I wouldn't mind using a feature branch to get the < 2k dependent
> packages rebuilt as suggested by Leo, if you think that's preferable.
That would work for the rebuilds, making this not a graft, but I'm
still concerned whether we really want these semantics or not.  With
the WebkitGTK bug fixed, we can put our generated default.pa into /etc
again, making it more debuggable.  My personal opinion is still on
explicitly declared rather than implicitly assumed.

Cheers

[bug#53676] [PATCH v2 2/4] gnu: pulseaudio: Graft to adjust configuration.

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Hi Maxim,
>
> Am Donnerstag, dem 24.02.2022 um 17:00 -0500 schrieb Maxim Cournoyer:
>> Thank you for your continued feedback.  The reason I prefer this
>> simple substitution to a conditional one is two-fold:
>> 
>> 1. It avoids two actors potentially touching the default 'script-
>> file' (the pulseaudio-service-type code as well as the user), which
>> could be unwieldy (do we plug the default.pa.d after their changes to
>> ensure it is there, or before, which means it'd potentially be
>> erased?).  Having it part of the shipped default.pa file makes this
>> simpler to reason with.
> Sure, but all we'd need here is proper documentation.  For the record,
> I would check if a `source /etc/pulse/default.pa.d' is in the user-
> supplied file (even if commented) and append it if not.

OK; I went a bit dumber/safer: when extra-script-files is non-null, the
.include is appended.

>> 2. It allows foreign distribution users to keep their custom user
>> script working even when they use our pulseaudio package (it makes
>> our pulseaudio package behave as intended by upstream).
> That ignores the case where users modify their distro's default.pa
> *and* put stuff into default.pa.d.  This might be necessary in some
> scenarios where the upstream default breaks user expectations.  I'd
> really prefer if foreign distro users just set their environment
> variables, as those work unconditionally as intended.

That sounds a bit hypothetical, but yes, it's a possibility.

>> I wouldn't mind using a feature branch to get the < 2k dependent
>> packages rebuilt as suggested by Leo, if you think that's preferable.
> That would work for the rebuilds, making this not a graft, but I'm
> still concerned whether we really want these semantics or not.  With
> the WebkitGTK bug fixed, we can put our generated default.pa into /etc
> again, making it more debuggable.  My personal opinion is still on
> explicitly declared rather than implicitly assumed.

OK, if we want to add the .include conditionally, I'd go with something
like:

--8<---------------cut here---------------start------------->8---
modified   doc/guix.texi
@@ -21507,7 +21507,10 @@ List of settings to set in @file{daemon.conf}, formatted just like
 @var{client-conf}.
 
 @item @code{script-file} (default: @code{(file-append pulseaudio "/etc/pulse/default.pa")})
-Script file to use as @file{default.pa}.
+Script file to use as @file{default.pa}.  In case the
+@code{extra-script-files} field below is used, an @code{.include}
+directive pointing to @file{/etc/pulse/default.pa.d} is appended to the
+provided script.
 
 @item @code{extra-script-files} (default: @code{'())})
 A list of file-like objects defining extra PulseAudio scripts to run at
modified   gnu/services/sound.scm
@@ -174,6 +174,21 @@ (define (assert-pulseaudio-script-file-name name)
                      extra-script-files)))
     (file-union "default.pa.d" (zip labels extra-script-files))))
 
+(define (append-include-directive script-file)
+  "Append an include directive to source scripts under /etc/pulse/default.pa.d."
+  (computed-file "default.pa"
+                 #~(begin
+                     (use-modules (ice-9 textual-ports))
+                     (define script-text
+                       (call-with-input-file #$script-file get-string-all))
+                     (call-with-output-file #$output
+                       (lambda (port)
+                         (format port (string-append script-text
+                                                     "
+# Added by Guix to include scripts specified in extra-script-files.
+.nofail
+.include /etc/pulse/default.pa.d~%")))))))
+
 (define pulseaudio-etc
   (match-lambda
     (($ <pulseaudio-configuration> client-conf daemon-conf default-script-file
@@ -181,7 +196,10 @@ (define pulseaudio-etc
      `(("pulse"
         ,(file-union
           "pulse"
-          `(("default.pa" ,default-script-file)
+          `(("default.pa"
+             ,(if (null? extra-script-files)
+                  default-script-file
+                  (append-include-directive default-script-file)))
             ("system.pa" ,system-script-file)
             ,@(if (null? extra-script-files)
                   '()
--8<---------------cut here---------------end--------------->8---

A mixed-file as you used previously (combining two .include) could have
been used, but I prefer to have the content directly visible under
/etc/pulse/default.pa (and the shebang line preserved).

This gets rid of the change on the pulseaudio package itself.

What do you think?

Thank you,

Maxim

[bug#53676] [PATCH v2 2/4] gnu: pulseaudio: Graft to adjust configuration.

[Liliana Marie Prikler]

Hi Maxim,

Am Samstag, dem 26.02.2022 um 01:21 -0500 schrieb Maxim Cournoyer:
> Hi Liliana,
> 
> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
> 
> > Hi Maxim,
> > 
> > Am Donnerstag, dem 24.02.2022 um 17:00 -0500 schrieb Maxim
> > Cournoyer:
> > 
> > > Thank you for your continued feedback.  The reason I prefer this
> > > simple substitution to a conditional one is two-fold:
> > > 
> > > 1. It avoids two actors potentially touching the default 'script-
> > > file' (the pulseaudio-service-type code as well as the user),
> > > which could be unwieldy (do we plug the default.pa.d after their
> > > changes to ensure it is there, or before, which means it'd
> > > potentially be erased?).  Having it part of the shipped
> > > default.pa file makes this simpler to reason with.
> > Sure, but all we'd need here is proper documentation.  For the
> > record, I would check if a `source /etc/pulse/default.pa.d' is in
> > the user-supplied file (even if commented) and append it if not.
> 
> OK; I went a bit dumber/safer: when extra-script-files is non-null,
> the .include is appended.
That works too and from what I can see you documented it, so people
will at least understand it if they read the manual :)

> > > I wouldn't mind using a feature branch to get the < 2k dependent
> > > packages rebuilt as suggested by Leo, if you think that's
> > > preferable.
> > That would work for the rebuilds, making this not a graft, but I'm
> > still concerned whether we really want these semantics or not. 
> > With the WebkitGTK bug fixed, we can put our generated default.pa
> > into /etc again, making it more debuggable.  My personal opinion is
> > still on explicitly declared rather than implicitly assumed.
> 
> OK, if we want to add the .include conditionally, I'd go with
> something like:
> 
> --8<---------------cut here---------------start------------->8---
> modified   doc/guix.texi
> @@ -21507,7 +21507,10 @@ List of settings to set in
> @file{daemon.conf}, formatted just like
>  @var{client-conf}.
>  
>  @item @code{script-file} (default: @code{(file-append pulseaudio
> "/etc/pulse/default.pa")})
> -Script file to use as @file{default.pa}.
> +Script file to use as @file{default.pa}.  In case the
> +@code{extra-script-files} field below is used, an @code{.include}
> +directive pointing to @file{/etc/pulse/default.pa.d} is appended to
> the
> +provided script.
>  
>  @item @code{extra-script-files} (default: @code{'())})
>  A list of file-like objects defining extra PulseAudio scripts to run
> at
> modified   gnu/services/sound.scm
> @@ -174,6 +174,21 @@ (define (assert-pulseaudio-script-file-name
> name)
>                       extra-script-files)))
>      (file-union "default.pa.d" (zip labels extra-script-files))))
>  
> +(define (append-include-directive script-file)
> +  "Append an include directive to source scripts under
> /etc/pulse/default.pa.d."
> +  (computed-file "default.pa"
> +                 #~(begin
> +                     (use-modules (ice-9 textual-ports))
> +                     (define script-text
> +                       (call-with-input-file #$script-file get-
> string-all))
> +                     (call-with-output-file #$output
> +                       (lambda (port)
> +                         (format port (string-append script-text
> +                                                     "
> +# Added by Guix to include scripts specified in extra-script-files.
> +.nofail
> +.include /etc/pulse/default.pa.d~%")))))))
> +
>  (define pulseaudio-etc
>    (match-lambda
>      (($ <pulseaudio-configuration> client-conf daemon-conf default-
> script-file
> @@ -181,7 +196,10 @@ (define pulseaudio-etc
>       `(("pulse"
>          ,(file-union
>            "pulse"
> -          `(("default.pa" ,default-script-file)
> +          `(("default.pa"
> +             ,(if (null? extra-script-files)
> +                  default-script-file
> +                  (append-include-directive default-script-file)))
>              ("system.pa" ,system-script-file)
>              ,@(if (null? extra-script-files)
>                    '()
> --8<---------------cut here---------------end--------------->8---
> 
> A mixed-file as you used previously (combining two .include) could
> have been used, but I prefer to have the content directly visible
> under /etc/pulse/default.pa (and the shebang line preserved).
I trust that this code does as you say it does, but looking at it with
my static analysis glasses I have no reason to believe it doesn't.

> This gets rid of the change on the pulseaudio package itself.
> 
> What do you think?
Looks good to me.  Is feedback from others still pending to make a v3
or am I the last straw here?

Cheers

bug#53676: [PATCH v2 2/4] gnu: pulseaudio: Graft to adjust configuration.

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

[...]

> Looks good to me.  Is feedback from others still pending to make a v3
> or am I the last straw here?

There was a suggestion by Maxime to attempt generalizing file-union with
implicit names from the file-like objects, but as I noted this would
make things a bit more complicated/tangled... perhaps it could accept
name sanitizer procedure as argument.

I'd prefer to keep such effort distinct, as I think it could live next
to file-union as file-union* for example.

I've now pushed this series, thank a lot for all the comments/feedback!

Maxim