[bug#40677] [PATCH v4] gnu: Add ffmpeg-jami.

[Jan <tona_kosmicznego_smiecia@interia.pl>]

On Thu, 14 May 2020 09:05:03 +0200
Mathieu Othacehe <othacehe@gnu.org> wrote:

> Hello Jan,
> 
> 
> It's better to avoid using "set!".
> 
What should I use instead?

> > +         (system=? (lambda (s)
> > +                     (string-prefix? %current-system s))))  
> 
> This should be (%current-system), plus I think arguments should be
> transposed.
Okay

> > +    (if (string-contains %current-system "linux")
> > +        (begin (append-flags %ffmpeg-linux-configure-flags)
> > +               (cond ((or (system=? "i686")
> > +                          (system=? "x86_64"))
> > +                      (append-flags
> > %ffmpeg-linux-x86-configure-flags))
> > +                     ((system=? "x86_64")
> > +                      (append-flags '("--arch=x86_64")))  
> 
> If the first branch of the cond succeeds, we will never add this flag.
That's what happens when you code late in night :)
I should sit down with a cup of coffee and read Guile's manual seriously
this time. Sorry for making your code review harder.

> Plus, it seems than ffmpeg is able to detect the running system. So I
> would suggest to do this:
> 
> --8<---------------cut here---------------start------------->8---
> ;; This procedure composes the configure flags list for ffmpeg-jami.
> (define (ffmpeg-compose-configure-flags)
>   (define (system=? s)
>     (string-prefix? s (%current-system)))
> 
>   `(,@%ffmpeg-default-configure-flags
>     ;; Add Linux specific flags.
>     ,@(if (string-contains %current-system "linux")
>           %ffmpeg-linux-configure-flags
>           '())
>     ,@(if (or (system=? "i686") (system=? "x86_64"))
>           %ffmpeg-linux-x86-configure-flags
>           '())))
> --8<---------------cut here---------------end--------------->8---
> 
> What do you think?

I think  %ffmpeg-linux-x86-configure-flags should be added only if
linux is present, not just when on i686 or x86_64. I called it
"%ffmpeg-linux-x86...", because it was inside of the "ifdef HAVE_LINUX"
condition. But there was also a comment saying "Desktop Linux", which
as always means very little. I'm not sure if it really requires the
Linux kernel there or what.

What about the output of the procedure? Is it okay for the list to be
not proper? It will look something like this:
((flag1 flag2 ... flagN) (flag1 flag2 ... flagN))
Is it okay because everything is treated as a pair in a recursive
manner?

Other than this, it will be good.


> Thanks,
> 
> Mathieu