GNU bug report logs - #53676
[PATCH 0/5] *** PulseAudio service improvements ***

Previous Next

Full log

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

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Cc: 53676 <at> debbugs.gnu.org
Subject: Re: [PATCH v2 2/4] gnu: pulseaudio: Graft to adjust configuration.
Date: Sat, 26 Feb 2022 01:21:44 -0500

Hi Liliana,

Liliana Marie Prikler <liliana.prikler <at> 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

Previous Next

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