[bug#70282,v4] gnu: gnome-shell: Wrap screencast service.

Adjust screencast such that GI_TYPELIB_PATH and GST_PLUGIN_SYSTEM_PATH are set
before starting.

Add all required gstreamer plugins to inputs.

To be able to use it a running pipewire service is needed.

* gnu/packages/gnome.scm (gnome-shell):
[inputs]: Add gst-plugins-good and pipewire.
[#:phases]<'wrap-programs>: Wrap org.gnome.Shell.Screencast.

Change-Id: I2c31bf1bd92e281b86c57b06988c6a3793a58d40
---
Here is v4 which appends the gstreamer plugins to GST_PLUGIN_SYSTEM_PATH, handling the case when getenv returns null. I think the difference between appendending or prepending in this case is not too important as the user GST_PLUGIN_PATH should take precedent. The more important thing is that the plugins are in any plugin path at all.

 gnu/packages/gnome.scm | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)


base-commit: 7b7f299bb493e485c5534b8c554e51d4f3a8c026

Hi Dariqq,

Dariqq <dariqq@posteo.net> writes:

> Adjust screencast such that GI_TYPELIB_PATH and GST_PLUGIN_SYSTEM_PATH are set
> before starting.
>
> Add all required gstreamer plugins to inputs.
>
> To be able to use it a running pipewire service is needed.
>
> * gnu/packages/gnome.scm (gnome-shell):
> [inputs]: Add gst-plugins-good and pipewire.
> [#:phases]<'wrap-programs>: Wrap org.gnome.Shell.Screencast.

Thanks for your efforts improving our GNOME desktop experience!

> Change-Id: I2c31bf1bd92e281b86c57b06988c6a3793a58d40
> ---
> Here is v4 which appends the gstreamer plugins to
> GST_PLUGIN_SYSTEM_PATH, handling the case when getenv returns null. I
> think the difference between appendending or prepending in this case
> is not too important as the user GST_PLUGIN_PATH should take
> precedent. The more important thing is that the plugins are in any
> plugin path at all.
>
>  gnu/packages/gnome.scm | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/gnu/packages/gnome.scm b/gnu/packages/gnome.scm
> index 92e35e3c5a..4bbff2a89b 100644
> --- a/gnu/packages/gnome.scm
> +++ b/gnu/packages/gnome.scm
> @@ -9408,6 +9408,7 @@ (define-public gnome-shell
>              (add-after 'install 'wrap-programs
>                (lambda* (#:key inputs #:allow-other-keys)
>                  (let ((gi-typelib-path  (getenv "GI_TYPELIB_PATH"))
> +                      (gst-plugin-path  (getenv "GST_PLUGIN_SYSTEM_PATH"))
>                        (python-path
>                         (string-join
>                          (filter (lambda (item)
> @@ -9427,6 +9428,19 @@ (define-public gnome-shell
>                                      "path => imports.gi.GIRepository.Repository."
>                                      "prepend_search_path(path));\n"
>                                      all)))
> +                  ;; Screencast requires a pipewire service running
> +                  ;; (i.e. as provided by home-pipewire-service-type)
> +                  (substitute* (string-append #$output "/share/gnome-shell/"
> +                                              "org.gnome.Shell.Screencast")
> +                    (("imports\\.package\\.start" all)
> +                     (string-append "'" gi-typelib-path "'.split(':').forEach("
> +                                    "path => imports.gi.GIRepository.Repository."
> +                                    "prepend_search_path(path));\n"
> +                                    "imports.gi.GLib.setenv('GST_PLUGIN_SYSTEM_PATH',"
> +                                    "[imports.gi.GLib.getenv('GST_PLUGIN_SYSTEM_PATH'),"
> +                                    "'" gst-plugin-path "'].filter(v => v).join(':'),"
> +                                    "true);\n"
> +                                    all)))

Perhaps a simple patch would convey the change better and be easier to
maintain in the future / be readily available for other distributions to
use.

>                    (for-each
>                     (lambda (prog)
>                       (wrap-program (string-append #$output "/bin/" prog)
> @@ -9492,6 +9506,7 @@ (define-public gnome-shell
>             gnome-settings-daemon
>             graphene
>             gst-plugins-base
> +           gst-plugins-good
>             ibus
>             libcanberra
>             libcroco
> @@ -9502,6 +9517,7 @@ (define-public gnome-shell
>             mesa-headers
>             mutter
>             network-manager-applet
> +           pipewire
>             polkit
>             pulseaudio
>             python-pygobject

Otherwise, LGTM!

Hi Maxim,

On 08.05.24 21:51, Maxim Cournoyer wrote:
> Hi Dariqq,
> 
> Dariqq <dariqq@posteo.net> writes:
> 
>> Adjust screencast such that GI_TYPELIB_PATH and GST_PLUGIN_SYSTEM_PATH are set
>> before starting.
>>
>> Add all required gstreamer plugins to inputs.
>>
>> To be able to use it a running pipewire service is needed.
>>
>> * gnu/packages/gnome.scm (gnome-shell):
>> [inputs]: Add gst-plugins-good and pipewire.
>> [#:phases]<'wrap-programs>: Wrap org.gnome.Shell.Screencast.
> 
> Thanks for your efforts improving our GNOME desktop experience!
> 

The slight annoyance with this is that if pipewire is not running the 
option to enable the screen recorder is there but it will stop 
immediately. But at least searching online for this problem has some 
mentions of pipewire and i added a comment in the pacakge aswell. I 
think it is definitly a better solution than not knowing that the option 
is even there as the button does not get displayed at all if the service 
fails to start.

>> Change-Id: I2c31bf1bd92e281b86c57b06988c6a3793a58d40
>> ---
>> Here is v4 which appends the gstreamer plugins to
>> GST_PLUGIN_SYSTEM_PATH, handling the case when getenv returns null. I
>> think the difference between appendending or prepending in this case
>> is not too important as the user GST_PLUGIN_PATH should take
>> precedent. The more important thing is that the plugins are in any
>> plugin path at all.
>>
>>   gnu/packages/gnome.scm | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/gnu/packages/gnome.scm b/gnu/packages/gnome.scm
>> index 92e35e3c5a..4bbff2a89b 100644
>> --- a/gnu/packages/gnome.scm
>> +++ b/gnu/packages/gnome.scm
>> @@ -9408,6 +9408,7 @@ (define-public gnome-shell
>>               (add-after 'install 'wrap-programs
>>                 (lambda* (#:key inputs #:allow-other-keys)
>>                   (let ((gi-typelib-path  (getenv "GI_TYPELIB_PATH"))
>> +                      (gst-plugin-path  (getenv "GST_PLUGIN_SYSTEM_PATH"))
>>                         (python-path
>>                          (string-join
>>                           (filter (lambda (item)
>> @@ -9427,6 +9428,19 @@ (define-public gnome-shell
>>                                       "path => imports.gi.GIRepository.Repository."
>>                                       "prepend_search_path(path));\n"
>>                                       all)))
>> +                  ;; Screencast requires a pipewire service running
>> +                  ;; (i.e. as provided by home-pipewire-service-type)
>> +                  (substitute* (string-append #$output "/share/gnome-shell/"
>> +                                              "org.gnome.Shell.Screencast")
>> +                    (("imports\\.package\\.start" all)
>> +                     (string-append "'" gi-typelib-path "'.split(':').forEach("
>> +                                    "path => imports.gi.GIRepository.Repository."
>> +                                    "prepend_search_path(path));\n"
>> +                                    "imports.gi.GLib.setenv('GST_PLUGIN_SYSTEM_PATH',"
>> +                                    "[imports.gi.GLib.getenv('GST_PLUGIN_SYSTEM_PATH'),"
>> +                                    "'" gst-plugin-path "'].filter(v => v).join(':'),"
>> +                                    "true);\n"
>> +                                    all)))
> 
> Perhaps a simple patch would convey the change better and be easier to
> maintain in the future / be readily available for other distributions to
> use.


The simple patch that would do this is basically the patch from nixos in 
v1 of this which adds a shebang line for gjs to the service invocation 
files (rather than the dbus service invoking $gjs $service). The problem 
then is that wrap-program changes the filename to * .real which makes 
gjs unhappy.

The people from nix circumvent this by using some js at the beginning to 
reset the entrypoint to the correct value.

One nice way around avoiding the problem would be using wrap-script 
instead though that does not support gjs as interpreter (yet?) and 
adding that forces a rebuild of all packages due to (guix build utils) 
changing.

Maybe another comment, similiar to the one Liliana suggested earlier in 
this thread, could be added at the beginning to inform about changing to 
wrap script + patch instead once that is a viable option?

> 
>>                     (for-each
>>                      (lambda (prog)
>>                        (wrap-program (string-append #$output "/bin/" prog)
>> @@ -9492,6 +9506,7 @@ (define-public gnome-shell
>>              gnome-settings-daemon
>>              graphene
>>              gst-plugins-base
>> +           gst-plugins-good
>>              ibus
>>              libcanberra
>>              libcroco
>> @@ -9502,6 +9517,7 @@ (define-public gnome-shell
>>              mesa-headers
>>              mutter
>>              network-manager-applet
>> +           pipewire
>>              polkit
>>              pulseaudio
>>              python-pygobject
> 
> Otherwise, LGTM!
> 
Have a nice day

Hi Dariqq,

Am Mittwoch, dem 08.05.2024 um 21:18 +0000 schrieb Dariqq:
> [...]
> 
> On 08.05.24 21:51, Maxim Cournoyer wrote:
> 
> > [...]
> > Perhaps a simple patch would convey the change better and be easier
> > to
> > maintain in the future / be readily available for other
> > distributions to
> > use.
> 
> The simple patch that would do this is basically the patch from nixos
> in  v1 of this which adds a shebang line for gjs to the service
> invocation files (rather than the dbus service invoking $gjs
> $service). The problem then is that wrap-program changes the filename
> to * .real which makes gjs unhappy.
> 
> [...]
> Maybe another comment, similiar to the one Liliana suggested earlier
> in this thread, could be added at the beginning to inform about
> changing to wrap script + patch instead once that is a viable option?
The pattern we typically use is to add an autotools-style "variable",
e.g. @GNOME_SHELL_GST_PLUGIN_SYSTEM_PATH@ through a patch, then use
substitute* to fill it in.  I don't think it's a requirement, but since
Maxim suggested, it'd definitely be nice to have.

Cheers

Hi Liliana,

On 09.05.24 00:11, Liliana Marie Prikler wrote:
> Hi Dariqq,
> 
> Am Mittwoch, dem 08.05.2024 um 21:18 +0000 schrieb Dariqq:
>> [...]
>>
>> On 08.05.24 21:51, Maxim Cournoyer wrote:
>>
>>> [...]
>>> Perhaps a simple patch would convey the change better and be easier
>>> to
>>> maintain in the future / be readily available for other
>>> distributions to
>>> use.
>>
>> The simple patch that would do this is basically the patch from nixos
>> in  v1 of this which adds a shebang line for gjs to the service
>> invocation files (rather than the dbus service invoking $gjs
>> $service). The problem then is that wrap-program changes the filename
>> to * .real which makes gjs unhappy.
>>
>> [...]
>> Maybe another comment, similiar to the one Liliana suggested earlier
>> in this thread, could be added at the beginning to inform about
>> changing to wrap script + patch instead once that is a viable option?

> The pattern we typically use is to add an autotools-style "variable",
> e.g. @GNOME_SHELL_GST_PLUGIN_SYSTEM_PATH@ through a patch, then use
> substitute* to fill it in.  I don't think it's a requirement, but since
> Maxim suggested, it'd definitely be nice to have.

How would this work in this case for gnomeshell? Put the js that gets 
concatenated here into the actual file with a patch adding placeholder 
variables for GST Plugin path and gi typelib path and replace later?

As these js files get generated from a common template for each service 
this would require substituting all of them and not only the screencast 
service.


> 
> Cheers
>

Hi Dariqq,

Dariqq <dariqq@posteo.net> writes:

[...]

>>> diff --git a/gnu/packages/gnome.scm b/gnu/packages/gnome.scm
>>> index 92e35e3c5a..4bbff2a89b 100644
>>> --- a/gnu/packages/gnome.scm
>>> +++ b/gnu/packages/gnome.scm
>>> @@ -9408,6 +9408,7 @@ (define-public gnome-shell
>>>               (add-after 'install 'wrap-programs
>>>                 (lambda* (#:key inputs #:allow-other-keys)
>>>                   (let ((gi-typelib-path  (getenv "GI_TYPELIB_PATH"))
>>> +                      (gst-plugin-path  (getenv "GST_PLUGIN_SYSTEM_PATH"))
>>>                         (python-path
>>>                          (string-join
>>>                           (filter (lambda (item)
>>> @@ -9427,6 +9428,19 @@ (define-public gnome-shell
>>>                                       "path => imports.gi.GIRepository.Repository."
>>>                                       "prepend_search_path(path));\n"
>>>                                       all)))
>>> +                  ;; Screencast requires a pipewire service running
>>> +                  ;; (i.e. as provided by home-pipewire-service-type)
>>> +                  (substitute* (string-append #$output "/share/gnome-shell/"
>>> +                                              "org.gnome.Shell.Screencast")
>>> +                    (("imports\\.package\\.start" all)
>>> +                     (string-append "'" gi-typelib-path "'.split(':').forEach("
>>> +                                    "path => imports.gi.GIRepository.Repository."
>>> +                                    "prepend_search_path(path));\n"
>>> +                                    "imports.gi.GLib.setenv('GST_PLUGIN_SYSTEM_PATH',"
>>> +                                    "[imports.gi.GLib.getenv('GST_PLUGIN_SYSTEM_PATH'),"
>>> +                                    "'" gst-plugin-path "'].filter(v => v).join(':'),"
>>> +                                    "true);\n"
>>> +                                    all)))
>> Perhaps a simple patch would convey the change better and be easier
>> to
>> maintain in the future / be readily available for other distributions to
>> use.
>
>
> The simple patch that would do this is basically the patch from nixos
> in v1 of this which adds a shebang line for gjs to the service
> invocation files (rather than the dbus service invoking $gjs
> $service). The problem then is that wrap-program changes the filename
> to * .real which makes gjs unhappy.
>
> The people from nix circumvent this by using some js at the beginning
> to reset the entrypoint to the correct value.
>
> One nice way around avoiding the problem would be using wrap-script
> instead though that does not support gjs as interpreter (yet?) and
> adding that forces a rebuild of all packages due to (guix build utils)
> changing.

It'd be nice to prep such support on core-updates.  You can test it on
master by having a (guix build utils-next) module that you explicitly
use.

> Maybe another comment, similiar to the one Liliana suggested earlier
> in this thread, could be added at the beginning to inform about
> changing to wrap script + patch instead once that is a viable option?

That could be a good reminder to have, yes.

Hi Liliana and Maxim,

On 09.05.24 00:11, Liliana Marie Prikler wrote:
> Hi Dariqq,
> 
> Am Mittwoch, dem 08.05.2024 um 21:18 +0000 schrieb Dariqq:
>> [...]
>>
>> On 08.05.24 21:51, Maxim Cournoyer wrote:
>>
>>> [...]
>>> Perhaps a simple patch would convey the change better and be easier
>>> to
>>> maintain in the future / be readily available for other
>>> distributions to
>>> use.
>>
>> The simple patch that would do this is basically the patch from nixos
>> in  v1 of this which adds a shebang line for gjs to the service
>> invocation files (rather than the dbus service invoking $gjs
>> $service). The problem then is that wrap-program changes the filename
>> to * .real which makes gjs unhappy.
>>
>> [...]
>> Maybe another comment, similiar to the one Liliana suggested earlier
>> in this thread, could be added at the beginning to inform about
>> changing to wrap script + patch instead once that is a viable option?
> The pattern we typically use is to add an autotools-style "variable",
> e.g. @GNOME_SHELL_GST_PLUGIN_SYSTEM_PATH@ through a patch, then use
> substitute* to fill it in.  I don't think it's a requirement, but since
> Maxim suggested, it'd definitely be nice to have.
> 

Tried this today and as the js service files are created from a common 
template using mesons 'configure_file' method this sets all 
autotools-style variables unknown to meson to the empty string. 
Afterwardes the susbtitute* at the wrapping phase is unable to replace 
anything ofc.

So I think I would need to either change the naming-scheme of the 
placeholders or substitute them into the template file  before the files 
get configured by meson.

Do you have a preference for any option (or maybe another idea)?

> Cheers
> 
Have a nice day

Am Freitag, dem 10.05.2024 um 14:59 +0000 schrieb Dariqq:
> Hi Liliana and Maxim,
> 
> On 09.05.24 00:11, Liliana Marie Prikler wrote:
> > Hi Dariqq,
> > 
> > Am Mittwoch, dem 08.05.2024 um 21:18 +0000 schrieb Dariqq:
> > > [...]
> > > 
> > > On 08.05.24 21:51, Maxim Cournoyer wrote:
> > > 
> > > > [...]
> > > > Perhaps a simple patch would convey the change better and be
> > > > easier
> > > > to
> > > > maintain in the future / be readily available for other
> > > > distributions to
> > > > use.
> > > 
> > > The simple patch that would do this is basically the patch from
> > > nixos
> > > in  v1 of this which adds a shebang line for gjs to the service
> > > invocation files (rather than the dbus service invoking $gjs
> > > $service). The problem then is that wrap-program changes the
> > > filename
> > > to * .real which makes gjs unhappy.
> > > 
> > > [...]
> > > Maybe another comment, similiar to the one Liliana suggested
> > > earlier
> > > in this thread, could be added at the beginning to inform about
> > > changing to wrap script + patch instead once that is a viable
> > > option?
> > The pattern we typically use is to add an autotools-style
> > "variable",
> > e.g. @GNOME_SHELL_GST_PLUGIN_SYSTEM_PATH@ through a patch, then use
> > substitute* to fill it in.  I don't think it's a requirement, but
> > since
> > Maxim suggested, it'd definitely be nice to have.
> > 
> 
> Tried this today and as the js service files are created from a
> common template using mesons 'configure_file' method this sets all 
> autotools-style variables unknown to meson to the empty string. 
> Afterwardes the susbtitute* at the wrapping phase is unable to
> replace anything ofc.
> 
> So I think I would need to either change the naming-scheme of the 
> placeholders or substitute them into the template file  before the
> files get configured by meson.
Or you add an option to meson_options.txt to fill it in, so that you
can provide the right value via #:configure-flags

Cheers

On 10.05.24 18:04, Liliana Marie Prikler wrote:
> Am Freitag, dem 10.05.2024 um 14:59 +0000 schrieb Dariqq:
>> Hi Liliana and Maxim,
>>
>> On 09.05.24 00:11, Liliana Marie Prikler wrote:
>>> Hi Dariqq,
>>>
>>> Am Mittwoch, dem 08.05.2024 um 21:18 +0000 schrieb Dariqq:
>>>> [...]
>>>>
>>>> On 08.05.24 21:51, Maxim Cournoyer wrote:
>>>>
>>>>> [...]
>>>>> Perhaps a simple patch would convey the change better and be
>>>>> easier
>>>>> to
>>>>> maintain in the future / be readily available for other
>>>>> distributions to
>>>>> use.
>>>>
>>>> The simple patch that would do this is basically the patch from
>>>> nixos
>>>> in  v1 of this which adds a shebang line for gjs to the service
>>>> invocation files (rather than the dbus service invoking $gjs
>>>> $service). The problem then is that wrap-program changes the
>>>> filename
>>>> to * .real which makes gjs unhappy.
>>>>
>>>> [...]
>>>> Maybe another comment, similiar to the one Liliana suggested
>>>> earlier
>>>> in this thread, could be added at the beginning to inform about
>>>> changing to wrap script + patch instead once that is a viable
>>>> option?
>>> The pattern we typically use is to add an autotools-style
>>> "variable",
>>> e.g. @GNOME_SHELL_GST_PLUGIN_SYSTEM_PATH@ through a patch, then use
>>> substitute* to fill it in.  I don't think it's a requirement, but
>>> since
>>> Maxim suggested, it'd definitely be nice to have.
>>>
>>
>> Tried this today and as the js service files are created from a
>> common template using mesons 'configure_file' method this sets all
>> autotools-style variables unknown to meson to the empty string.
>> Afterwardes the susbtitute* at the wrapping phase is unable to
>> replace anything ofc.
>>
>> So I think I would need to either change the naming-scheme of the
>> placeholders or substitute them into the template file  before the
>> files get configured by meson.
> Or you add an option to meson_options.txt to fill it in, so that you
> can provide the right value via #:configure-flags

I'd want to avoid putting the environement variables together myself and 
rather use getenv to retrieve the value at build time though. Don't 
think that would work with #:configure-flags

> 
> Cheers

Have a nice day

Am Donnerstag, dem 09.05.2024 um 11:30 -0400 schrieb Maxim Cournoyer:
> Hi Dariqq,
> 
> Dariqq <dariqq@posteo.net> writes:
> 
> [...]
> 
> > > > diff --git a/gnu/packages/gnome.scm b/gnu/packages/gnome.scm
> > > > index 92e35e3c5a..4bbff2a89b 100644
> > > > --- a/gnu/packages/gnome.scm
> > > > +++ b/gnu/packages/gnome.scm
> > > > @@ -9408,6 +9408,7 @@ (define-public gnome-shell
> > > >               (add-after 'install 'wrap-programs
> > > >                 (lambda* (#:key inputs #:allow-other-keys)
> > > >                   (let ((gi-typelib-path  (getenv
> > > > "GI_TYPELIB_PATH"))
> > > > +                      (gst-plugin-path  (getenv
> > > > "GST_PLUGIN_SYSTEM_PATH"))
> > > >                         (python-path
> > > >                          (string-join
> > > >                           (filter (lambda (item)
> > > > @@ -9427,6 +9428,19 @@ (define-public gnome-shell
> > > >                                       "path =>
> > > > imports.gi.GIRepository.Repository."
> > > >                                      
> > > > "prepend_search_path(path));\n"
> > > >                                       all)))
> > > > +                  ;; Screencast requires a pipewire service
> > > > running
> > > > +                  ;; (i.e. as provided by home-pipewire-
> > > > service-type)
> > > > +                  (substitute* (string-append #$output
> > > > "/share/gnome-shell/"
> > > > +                                             
> > > > "org.gnome.Shell.Screencast")
> > > > +                    (("imports\\.package\\.start" all)
> > > > +                     (string-append "'" gi-typelib-path
> > > > "'.split(':').forEach("
> > > > +                                    "path =>
> > > > imports.gi.GIRepository.Repository."
> > > > +                                   
> > > > "prepend_search_path(path));\n"
> > > > +                                   
> > > > "imports.gi.GLib.setenv('GST_PLUGIN_SYSTEM_PATH',"
> > > > +                                   
> > > > "[imports.gi.GLib.getenv('GST_PLUGIN_SYSTEM_PATH'),"
> > > > +                                    "'" gst-plugin-path
> > > > "'].filter(v => v).join(':'),"
> > > > +                                    "true);\n"
> > > > +                                    all)))
> > > Perhaps a simple patch would convey the change better and be
> > > easier to maintain in the future / be readily available for other
> > > distributions to use.
> > 
> > 
> > The simple patch that would do this is basically the patch from
> > nixos in v1 of this which adds a shebang line for gjs to the
> > service invocation files (rather than the dbus service invoking
> > $gjs $service). The problem then is that wrap-program changes the
> > filename to * .real which makes gjs unhappy.
> > 
> > The people from nix circumvent this by using some js at the
> > beginning to reset the entrypoint to the correct value.
> > 
> > One nice way around avoiding the problem would be using wrap-script
> > instead though that does not support gjs as interpreter (yet?) and
> > adding that forces a rebuild of all packages due to (guix build
> > utils) changing.
> 
> It'd be nice to prep such support on core-updates.  You can test it
> on master by having a (guix build utils-next) module that you
> explicitly use.
> 
> > Maybe another comment, similiar to the one Liliana suggested
> > earlier in this thread, could be added at the beginning to inform
> > about changing to wrap script + patch instead once that is a viable
> > option?
> 
> That could be a good reminder to have, yes.
I've pushed the current version as-is, without reminder, so that we can
at least mark the issue done.  If someone has a good text to add,
please don't hesitate to do so in post.  Also don't hesitate to submit
new stuff to gnome-team or core-updates after the pending merges are
through.

Cheers and thanks for all the hard work :)