guix-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[bug#30679] [PATCH] Update VLC to 3.0.1 (Fixed submission - second try)


From: Ludovic Courtès
Subject: [bug#30679] [PATCH] Update VLC to 3.0.1 (Fixed submission - second try)
Date: Sat, 03 Mar 2018 22:31:00 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux)

Hello pkill9,

Thanks for the patch!

In the future, can you please send a patch containing a commit log that
follows the project’s conventions, and produced by ‘git format-patch’?

See
<https://www.gnu.org/software/guix/manual/html_node/Submitting-Patches.html>.
Don’t worry if you’re unsure about the details, we can help.

<address@hidden> skribis:

>      (native-inputs
>       `(("git" ,git) ; needed for a test
> +       ("gcc" ,gcc)

This is normally not needed.  Can you remove it?

> +       ;Optional inputs
> +       ;("lua" ,lua) ;Configure phase doesn't find this
> +       ("libarchive" ,libarchive)
> +       ("livemedia-utils" ,livemedia-utils)
> +       ("libbluray" ,libbluray)
> +       ("samba" ,samba)
> +       ;("nfs-utils" ,(@ (gnu packages nfs) nfs-utils)) ;configure phase 
> doesn't find this
> +       ;("freerdp" ,(@ (gnu packages rdesktop) freerdp)) ;causes compilation 
> failure
> +       ("libshout" ,libshout)
> +       ("libebml" ,libebml)
> +       ("libmatroska" ,libmatroska)
> +       ("libmodplug" ,libmodplug)
> +       ("mpg123" ,mpg123)
> +       ;("gstreamer"
> +       ; ,(@ (gnu packages gstreamer) gstreamer)) ;configure phase doesn't 
> find this.

Nitpick: please use two semicolons for comments like this (one semicolon
is for margin comments.)

> +       ("libva" ,libva)
> +       ("twolame" ,twolame)
> +       ("libdca" ,libdca)
> +       ("libmpeg2" ,libmpeg2)
> +       ("speexdsp" ,speexdsp)
> +       ("fluidsynth" ,fluidsynth)
> +       ("libass" ,libass)
> +       ("fribidi" ,fribidi)
> +       ("librsvg" ,librsvg)
> +       ("libcaca" ,libcaca)
> +       ("jack" ,jack-1)
> +       ("soxr" ,soxr)
> +       ("chromaprint" ,chromaprint)
> +       ("protobuf" ,protobuf)
> +       ("eudev" ,eudev)
> +       ("libmtp" ,libmtp)
> +       ("libupnp" ,libupnp)
> +       ("taglib" ,taglib)
> +       ("libsecret" ,libsecret)
> +       ("libnotify" ,libnotify)))

That’s a lot of new dependencies!  :-)

Could you check the impact this has on the size of vlc plus its
dependencies, as shown by “guix size vlc”?

In many cases we add all the optional dependencies of the package, for
convenience.  However there are cases where doing it may be impractical,
because that would make the whole thing just too big.  If that is the
case, we should keep a reduced set of dependencies of the ‘vlc’ package
and, if needed, add specific package variants with additional
dependencies.

> +         (add-after 'build 'fix-misnamed-appdata
> +           ;; vlc.appdata.xml.in gets created instead of vlc.appdata.xml, so
> +        ;; we copy it to the correct name - This solution is hacky, if the
> +        ;; root cause is found then please supply a patch.
> +           (lambda _ (copy-file "share/vlc.appdata.xml.in" 
> "share/vlc.appdata.xml")))

That looks fishy.  Could you check if there’s a Makefile(.am) that
mentions “vlc.appdata.xml” so we can see how it’s supposed to be built?

Thanks,
Ludo’.





reply via email to

[Prev in Thread] Current Thread [Next in Thread]