| Message ID | 20220224163828.11330-2-maxim.cournoyer@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#53676,v2,1/4] services/sound: Normalize pulseaudio-configuration accessor names. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
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
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
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
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
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
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
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")