diff mbox series

[bug#59372] gnu: peek: Add missing input.

Message ID 20221119012744.30470-1-jgart@dismail.de
State New
Headers show
Series [bug#59372] gnu: peek: Add missing input. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git-branch success View Git branch
cbaines/applying patch success
cbaines/issue success View issue

Commit Message

jgart Nov. 19, 2022, 1:27 a.m. UTC
* gnu/packages/video.scm (peek): Add missing input.
[inputs]: Add ffmpeg so that Peek will not fail to run on a foreign
distro.

hi, peek fails to run on a foreign distro without ffmpeg as an input.

this patch fixes that.

all best,

jgart
---
 gnu/packages/video.scm | 2 ++
 1 file changed, 2 insertions(+)

Comments

Tobias Geerinckx-Rice Nov. 19, 2022, 2:50 a.m. UTC | #1
Hi jgart,

jgart via Guix-patches via 写道:
> hi, peek fails to run on a foreign distro without ffmpeg as an 
> input.
>
> this patch fixes that.

Thanks!

As far as I can tell, this change doesn't affect the peek 
package's output at all.  Am I mistaken?  What should the 
observable difference be, beyond ‘working’?

How are you invoking both peeks, and testing the difference?  Are 
they both running in the exact same environment, back-to-back?

Kind regards,

T G-R
jgart Nov. 19, 2022, 4:07 a.m. UTC | #2
On Sat, 19 Nov 2022 03:50:05 +0100 Tobias Geerinckx-Rice <me@tobias.gr> wrote:
> As far as I can tell, this change doesn't affect the peek 
> package's output at all.  Am I mistaken?  What should the 
> observable difference be, beyond ‘working’?

It was complaining on Ubuntu when I started it that either ffmpeg needs
to be installed or a gnome-shell needs to be running. I didn't log the
exact error message but that was more or less it.

> How are you invoking both peeks, and testing the difference?

When I add ffmpeg it worked without the warning and actually started
recording my screen as expected. After pressing the record button of
course ;()

I tested once without ffmpeg and once with ffmpeg. 

The former failed to record.

> Are they both running in the exact same environment, back-to-back?

Yup I tested both in Ubuntu.

Let me know if you'd like any more info or if I wasn't precise enough
in my description.

all best,

jgart
\( Nov. 19, 2022, 9:13 a.m. UTC | #3
Heya,

On Sat Nov 19, 2022 at 1:27 AM GMT, jgart via Guix-patches via wrote:
> * gnu/packages/video.scm (peek): Add missing input.
> [inputs]: Add ffmpeg so that Peek will not fail to run on a foreign
> distro.

> diff --git a/gnu/packages/video.scm b/gnu/packages/video.scm
> index c394c65442..4f38d4432c 100644
> --- a/gnu/packages/video.scm
> +++ b/gnu/packages/video.scm
> @@ -5369,6 +5369,8 @@ (define-public peek
>      (arguments '(#:glib-or-gtk? #t))
>      (inputs
>       `(("gtk+" ,gtk+)
> +       ;; Peek fails to run on a foreign distro without ffmpeg.
> +       ("ffmpeg" ,ffmpeg)
>         ("python" ,python-wrapper)))
>      (native-inputs
>       `(("desktop-file-utils" ,desktop-file-utils) ; for update-desktop-database

I don't think this will be enough; does the program call the ``ffmpeg''
executable? If so, you'll need to patch the call to ``ffmpeg'' to point to
/gnu/store/.../bin/ffmpeg directly.

Could you also update the inputs/arguments style to gexp form?

    -- (
Tobias Geerinckx-Rice Nov. 19, 2022, 9:38 a.m. UTC | #4
jgart 写道:
> It was complaining on Ubuntu when I started it that either 
> ffmpeg needs
> to be installed or a gnome-shell needs to be running. I didn't 
> log the
> exact error message but that was more or less it.

And how does this patch provide ffmpeg?  It adds it to the build 
environment, but what happens then?

We know it doesn't add a reference to the final ‘peek’ output (you 
can verify this yourself, assuming peek builds reproducibly).  It 
doesn't change any code in the peek binary.

So what's the mechanism by which it would work?  It will need to 
be documented.

>> How are you invoking both peeks, and testing the difference?
>
> When I add ffmpeg it worked without the warning and actually 
> started
> recording my screen as expected. After pressing the record 
> button of
> course ;()

But how exactly?  Please provide exact steps and commands.

( 写道:
> Could you also update the inputs/arguments style to gexp form?

…but in a separate patch, please.)

Kind regards,

T G-R
jgart Nov. 21, 2022, 2:27 p.m. UTC | #5
On Sat, 19 Nov 2022 10:38:26 +0100 Tobias Geerinckx-Rice <me@tobias.gr> wrote:
> So what's the mechanism by which it would work?  It will need to 
> be documented.

Hi Tobias,

peek, needs either ffmpeg or gnome shell to be present.

See here for the exact reference:

https://github.com/phw/peek/blob/2daba7cf114e01cff870afd20b96b212a0252054/src/application.vala#L67

How should I handle that in Guix?

Should I patch ffmpeg? What if a user wants to use gnome-shell instead of ffmpeg?

Is there a switch I can provide in Guile code?

all best,

jgart
Tobias Geerinckx-Rice Nov. 21, 2022, 3:14 p.m. UTC | #6
Hi jgart,

The patch you sent does nothing and didn't fix your foreign 
distro.  The questions you won't answer were supposed to find out 
what might have, but let's move on.

So, please test ('s suggestion.  It's the right fix!  Propagation 
isn't.

You can offer two variants of peek (peek-ffmpeg and 
peek-gnome-shell, say) if you like.  I don't think that's 
necessary, though, unless ffmpeg support is incomplete?

Thanks,

T G-R
Simon Tournier Nov. 21, 2022, 5:13 p.m. UTC | #7
Hi,

Thanks for resuming #51202. :-)

On Sat, 19 Nov 2022 at 03:50, Tobias Geerinckx-Rice via Guix-patches via <guix-patches@gnu.org> wrote:

> As far as I can tell, this change doesn't affect the peek 
> package's output at all.  Am I mistaken?  What should the 
> observable difference be, beyond ‘working’?

Peek needs to be propagated.  An issue is open,

    http://issues.guix.gnu.org/issue/51202
    
IIRC, peek needs to be patched to avoid the propagation and personally,
I am using something along,

    guix shell peek \
            gtk+ gsettings-desktop-schemas ffmpeg \
            -- peek


Cheers,
simon
jgart Nov. 21, 2022, 7:38 p.m. UTC | #8
On Mon, 21 Nov 2022 18:13:26 +0100 zimoun <zimon.toutoune@gmail.com> wrote:
> Hi,
> 
> Thanks for resuming #51202. :-)
> 
> On Sat, 19 Nov 2022 at 03:50, Tobias Geerinckx-Rice via Guix-patches via <guix-patches@gnu.org> wrote:
> 
> > As far as I can tell, this change doesn't affect the peek 
> > package's output at all.  Am I mistaken?  What should the 
> > observable difference be, beyond ‘working’?
> 
> Peek needs to be propagated.  An issue is open,

I read through the open issue. Should I send a patch implementing lilyp's suggestion?

That's what I was also thinking of doing but I also had the question
regarding choosing between ffmpeg or gnome-shell. Should we prefer
ffmpeg? Would be cool if we could support choosing between ffmpeg or
gnome-shell.

Is there currently a way to do that?
Tobias Geerinckx-Rice Nov. 21, 2022, 8:18 p.m. UTC | #9
jgart 写道:
> peek, needs either ffmpeg or gnome shell to be present.
> How should I handle that in Guix?

Ideally: let peek pop up the window asking the user to install 
ffmpeg.

Then the user installs ffmpeg only if it's actually needed, and 
isn't ‘forced’ to pay the cost if it's not.

Does it not show that window on a foreign distribution?

Kind regards,

T G-R
jgart Nov. 21, 2022, 8:58 p.m. UTC | #10
On Mon, 21 Nov 2022 21:18:32 +0100 Tobias Geerinckx-Rice <me@tobias.gr> wrote:
> Does it not show that window on a foreign distribution?

Yup, it does show that window on a foreign distro.

So, we should just have the user install ffmpeg along with peek in their profile?

Should this ticket be closed then? https://issues.guix.gnu.org/issue/51202#3
Simon Tournier Nov. 22, 2022, 2:39 p.m. UTC | #11
Hi,

On Mon, 21 Nov 2022 at 13:38, jgart via Guix-patches via <guix-patches@gnu.org> wrote:

> That's what I was also thinking of doing but I also had the question
> regarding choosing between ffmpeg or gnome-shell. Should we prefer
> ffmpeg? Would be cool if we could support choosing between ffmpeg or
> gnome-shell.

Well, from my point of view, let start to use ffmeg without the
propagation.  Then, incremental improvements. :-)

Cheers,
simon
jgart Nov. 22, 2022, 6:29 p.m. UTC | #12
On Tue, 22 Nov 2022 15:39:31 +0100 zimoun <zimon.toutoune@gmail.com> wrote:
> Well, from my point of view, let start to use ffmeg without the
> propagation.  Then, incremental improvements. :-)

Ok, should I send a new patch patching the ffmpeg executable from the
inputs of peek?
宋文武 Jan. 29, 2023, 6:02 a.m. UTC | #13
jgart <jgart@dismail.de> writes:

> On Mon, 21 Nov 2022 21:18:32 +0100 Tobias Geerinckx-Rice <me@tobias.gr> wrote:
>> Does it not show that window on a foreign distribution?
>
> Yup, it does show that window on a foreign distro.
>
> So, we should just have the user install ffmpeg along with peek in their profile?
>
> Should this ticket be closed then? https://issues.guix.gnu.org/issue/51202#3

Yes, I think it's expected to install ffmpeg yourself for peek when not
in gnome shell.  Close now.
diff mbox series

Patch

diff --git a/gnu/packages/video.scm b/gnu/packages/video.scm
index c394c65442..4f38d4432c 100644
--- a/gnu/packages/video.scm
+++ b/gnu/packages/video.scm
@@ -5369,6 +5369,8 @@  (define-public peek
     (arguments '(#:glib-or-gtk? #t))
     (inputs
      `(("gtk+" ,gtk+)
+       ;; Peek fails to run on a foreign distro without ffmpeg.
+       ("ffmpeg" ,ffmpeg)
        ("python" ,python-wrapper)))
     (native-inputs
      `(("desktop-file-utils" ,desktop-file-utils) ; for update-desktop-database