[bug#56803,3/6] gnu: Add python-mpv.

On 2022-07-27 22:05, Maxime Devos wrote:
> On 27-07-2022 21:00, Tomasz Jeneralczyk wrote:
> IIUC, that's for using Guile modules defined in packages -- but mpv is
> not a Guile package, so I expect that the surrounding with-extensions
> can be dropped.
You're right. I dropped it with no negative consequences.

> This sounds like something upstream should be informed about, otherwise 
> they wouldn't know there is something to fix.
I had a friend on arch linux run those tests and everything worked just 
fine, so it might have something to do with guix
itself or an incomplete package definition. In any case, I'll make sure 
to write a proper bug report in the upstream repo later.


> Look for substitute-keyword-arguments, which isn't stateful and hence
> there is less risk of accidentally modifying the arguments of the
> parent package.  Also, any reason for not adding this to the original
> package? (Possibly there is one).
Thanks, this macro is not documented in manual, but it looks much nicer 
now.
The reason I made a new package is simply because someone on irc 
recommended me to do as such. Though your question made me realize
that just adding one flag worked because all the necessary packages to 
build python buildings were in opencv package already...
maybe originally it was intended for the python bindings to be included 
in opencv? And so I added the flag to opencv and removed
my original package - it works all the same.


> Run "./pre-inst-env guix lint hydrus-network", it will have a remark
> about this.  Also, technically this is racy -- it's possible for
> python to start before Xvfb is ready though so far this doesn't seem
> to have caused trouble for other packages yet AFAIK -- I recommend
> "xvfb-run" "--" "python" "test.py" instead.
I though I missed something so I ran lint again, but it only said 
there's a new version of hydrus available.
Nevertheless I changed the invocation to what you recommended. I also 
updated hydrus to version 493.


> (I just scrolled quickly through the patches, a more full review will
> have to come later.)
Thank you for all your comments so far.
I made changes but I wont send them as patches until I fix all the 
problems or get a confirmation that there's nothing more to do.

However I'm attaching a "preview" diff of the changes I'll want to 
iclude in v2 of my patches.
I suppose I should only send v2 of the ones I changed, right?.

Hi,

Tomasz Jeneralczyk <tj@schwi.pl> skribis:

> +        #~(begin
> +            ;; Without an absolute path it is not able find and
> +            ;; load the libmpv library.
> +            (substitute* "mpv.py"
> +              (("(sofile = )(.*)" _ pre post)
> +               (string-append pre "\"" #$mpv "/lib/\" + " post)))

In addition, this substitution should be done in a phase rather than in
a snippet, because (1) the result of ‘guix build -S’ should be
platform-independent and thus not include the file name of ‘mpv’ for a
particular system, and (2) the reference to variable ‘mpv’ at the top
level can cause problems due to circular dependencies among modules.

For clarity, could you resent the whole series with those changes,
squashing the relevant amendments?

Thanks in advance,
Ludo’.

On 28-07-2022 16:58, Tomasz Jeneralczyk wrote:
>
>> Run "./pre-inst-env guix lint hydrus-network", it will have a remark
>> about this.  Also, technically this is racy -- it's possible for
>> python to start before Xvfb is ready though so far this doesn't seem
>> to have caused trouble for other packages yet AFAIK -- I recommend
>> "xvfb-run" "--" "python" "test.py" instead.
> I though I missed something so I ran lint again, but it only said 
> there's a new version of hydrus available.
> Nevertheless I changed the invocation to what you recommended. I also 
> updated hydrus to version 493. 

Looks like the linter ignored hydrus-network because it doesn't 
understand the 'let' construct ... work for later (not this patch 
series) ...

Greetings,
Maxime.

On 2022-08-09 15:09, Ludovic Courtès wrote:
> Perhaps these comments, or some of them, should go as comments in the
> source?  That will prove helpful next time you or someone else tries to
> work on the package.
They cluttered the code with meta-information that I didn't consider to 
be directly related to the package's source. Thought anyone searching 
would see the commit message and that would be enough. I'll add them 
back in if that's what you prefer.

> In general, the solution here, rather than copy files like the ‘ffmpeg’
> executable, would be to patch Hydrus so that it contains the absolute
> file name of ‘ffmpeg’ as returned by (search-input-file inputs 
> "/bin/ffmpeg").
> 
> How does that sound?
At the time I thought that directly patching the binary paths could 
break some logic, but I looked into it and it looks like I only had to 
patch one `if` statement. This could change in future releases, but it's 
not likely. It appears to work as well and maybe even better - Now 
miniupnpc isn't timing out for me, which might or might not be because 
of this change.

> In addition, this substitution should be done in a phase rather than in
> a snippet, because (1) the result of ‘guix build -S’ should be
> platform-independent and thus not include the file name of ‘mpv’ for a
> particular system, and (2) the reference to variable ‘mpv’ at the top
> level can cause problems due to circular dependencies among modules.
I didn't know that, but it makes sense. I moved the code into its own 
phase.
And if I understand the 2nd point correctly I should use something like 
`(assoc-ref inputs "mpv")` instead of `#$mpv`, right? I  cannot use 
`(search-input-file ...)` because the name of the mpv's lib is 
determined during runtime by the python code and I didn't want to 
hardcode the mpv so file's version.

After I finish making the changes I'll send the whole patchset again, it 
will also include updated packages to their newest release and be 
rebased onto current master.

Thank you for taking the time to review my work.

Hey,

On Sat Aug 13, 2022 at 3:34 PM BST, Tomasz Jeneralczyk wrote:
> > In addition, this substitution should be done in a phase rather than in
> > a snippet, because (1) the result of ‘guix build -S’ should be
> > platform-independent and thus not include the file name of ‘mpv’ for a
> > particular system, and (2) the reference to variable ‘mpv’ at the top
> > level can cause problems due to circular dependencies among modules.
> And if I understand the 2nd point correctly I should use something like 
> `(assoc-ref inputs "mpv")` instead of `#$mpv`, right? I  cannot use 
> `(search-input-file ...)` because the name of the mpv's lib is 
> determined during runtime by the python code and I didn't want to 
> hardcode the mpv so file's version.

No, the assoc-ref style is basically deprecated last I heard. The problem
is with snippets: apparently something with the definition of the `snippet`
field can cause module dependency issues, whereas `arguments` does not
suffer from the same. IIRC, the issue is that the former is not specifie
as a `(delayed)` field. So, #$pkg is okay in arguments, but sometimes not
in snippets.

    -- (

On 2022-08-13 21:26, ( wrote:
> No, the assoc-ref style is basically deprecated last I heard. The 
> problem
> is with snippets: apparently something with the definition of the 
> `snippet`
> field can cause module dependency issues, whereas `arguments` does not
> suffer from the same. IIRC, the issue is that the former is not 
> specifie
> as a `(delayed)` field. So, #$pkg is okay in arguments, but sometimes 
> not
> in snippets.

Does this mean I should replace `(assoc-ref inputs "python")` with 
`#$python` in the install phase of hydrus?
I checked and it didn't cause any problems.

I also noticed that python-mpv's mpv library cannot find libmpv.so 
during runtime (only in the check phase) so I hardcoded the path in 
source to `(search-input-file inputs "/lib/libmpv.so")`.
Was told on irc that most libraries are hardcoded so I take it's not a 
problem.

On Sun Aug 14, 2022 at 11:10 AM BST, Tomasz Jeneralczyk wrote:
> Does this mean I should replace `(assoc-ref inputs "python")` with 
> `#$python` in the install phase of hydrus?
> I checked and it didn't cause any problems.

You could, but I think #$(this-package-input "python") would be better style.

    -- (