Re: [PATCH] gnu: Add gst-plugins-ugly

[Mark H Weaver <mhw@netris.org>]

David Hashe <david.hashe@dhashe.com> writes:

> From c3ec7cf01a6c1bf9013a2819c2c5ec7181724947 Mon Sep 17 00:00:00 2001
> From: David Hashe <david.hashe@dhashe.com>
> Date: Tue, 7 Jul 2015 23:40:01 -0500
> Subject: [PATCH] gnu: Add rhythmbox.
>
> * gnu/packages/gnome.scm (rhythmbox): New variable.
> ---
>  gnu/packages/gnome.scm | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
>
> diff --git a/gnu/packages/gnome.scm b/gnu/packages/gnome.scm
> index 0ec5ea2..6d238af 100644
> --- a/gnu/packages/gnome.scm
> +++ b/gnu/packages/gnome.scm
> @@ -56,6 +56,7 @@
>    #:use-module (gnu packages libcanberra)
>    #:use-module (gnu packages linux)
>    #:use-module (gnu packages libusb)
> +  #:use-module (gnu packages lirc)
>    #:use-module (gnu packages image)
>    #:use-module (gnu packages perl)
>    #:use-module (gnu packages pkg-config)

I've since added (gnu packages lirc) here as part of the Totem patch, so
this hunk should be removed.

> @@ -2718,3 +2719,76 @@ the patterned block to the area bordered by green markers.  To do so, you will
>  need to slide other blocks out of the way.  Complete each puzzle in as few moves
>  as possible!")
>      (license license:gpl2+)))
> +
> +(define-public rhythmbox
> + (package
> +   (name "rhythmbox")
> +   (version "3.2.1")
> +   (source (origin
> +            (method url-fetch)
> +            (uri (string-append "mirror://gnome/sources/" name "/"
> +                                (version-major+minor version) "/"
> +                                name "-" version ".tar.xz"))
> +            (sha256
> +             (base32
> +              "0f3radhlji7rxl760yl2vm49fvfslympxrpm8497acbmbd7wlhxz"))))
> +   (build-system glib-or-gtk-build-system)
> +   (arguments
> +    `(#:configure-flags
> +      (list "--enable-lirc"
> +            "--enable-python"
> +            "--enable-vala"
> +            "--with-brasero"
> +            "--with-gudev"
> +            "--with-libsecret")))
> +   (propagated-inputs
> +    `(("dconf" ,dconf)
> +      ("gobject-introspection" ,gobject-introspection)
> +      ("gst-libav" ,gst-libav)
> +      ("gst-plugins-base" ,gst-plugins-base)
> +      ("gst-plugins-good" ,gst-plugins-good)
> +      ("gst-plugins-ugly" ,gst-plugins-ugly)
> +      ("totem-pl-parser" ,totem-pl-parser)))

I agree that 'dconf' should be a propagated-input.

'gobject-introspection' should be moved to 'native-inputs'.

'gst-libav' and 'gst-plugins-ugly' should be removed entirely.  Users
can add them to their profile if they wish.

As for 'gst-plugins-base' and 'gst-plugins-good': I think it would be
better to make them normal inputs, and add a wrapper for rhythmbox that
adds a prefix to GST_PLUGIN_SYSTEM_PATH, similar to what we do in the
Totem package.

What about 'totem-pl-parser'?  Does that need to be a propagated-input?
If so, why?

> +   (native-inputs
> +    `(("intltool" ,intltool)
> +      ("glib" ,glib "bin")
> +      ("desktop-file-utils" ,desktop-file-utils)
> +      ("pkg-config" ,pkg-config)))
> +   (inputs
> +    `(("json-glib" ,json-glib)
> +      ("tdb" ,tdb)
> +      ("gnome-desktop" ,gnome-desktop)
> +      ("python" ,python)
> +      ("python-pygobject" ,python2-pygobject)
> +      ("vala" ,vala)
> +      ("gmime" ,gmime)
> +      ("nettle" ,nettle)
> +      ("itstool" ,itstool)
> +      ("adwaita-icon-theme" ,adwaita-icon-theme)
> +      ("gstreamer" ,gstreamer)
> +      ("gudev" ,eudev)

Does 'eudev' provide 'gudev'?  This seems mismatched, but perhaps I'm
mistaken.

> +      ;("libmtp" ,libmtp) FIXME Not detected

Please use two semicolons here.  In general, use one semicolon for
margin comments (on the right), and two semicolons for comments that are
in the same column as the surrounding code.  Emacs decides how to
auto-indent Lisp/Scheme comments based on the number of semicolons.

> +      ("libsecret" ,libsecret)
> +      ("libsoup" ,libsoup)
> +      ("libnotify" ,libnotify)
> +      ("libpeas" ,libpeas)
> +      ("lirc" ,lirc)
> +      ; TODO Unused without mx

Two semicolons, and it's not clear which input the comment above refers
to.  Please make it more clear.

> +      ;("clutter" ,clutter)
> +      ;("clutter-gtk" ,clutter-gtk)
> +      ;("clutter-gst" ,clutter-gst)

Two semicolons.

> +      ("gsettings-desktop-schemas" ,gsettings-desktop-schemas)
> +      ("atk" ,atk)
> +      ("pango" ,pango)
> +      ("gtk+" ,gtk+)
> +      ;; TODO:
> +      ;;  * grilo

We have grilo now.  You should probably add both 'grilo' and
'grilo-plugins' as inputs and then set GRL_PLUGIN_PATH in the wrapper,
like we do in the Totem package.

> +      ;;  * libgpod
> +      ;;  * mx
> +      ;;  * webkit
> +      ("brasero" ,brasero)))
> +   (home-page "https://wiki.gnome.org/Apps/Rhythmbox")
> +   (synopsis "Music player for GNOME")
> +   (description "Rhythmbox is a music playing application for GNOME.  It
> +supports playlists, song ratings, and any codecs installed through gstreamer.")
> +   (license license:gpl2+)))

Can you send an updated patch?

     Thanks,
       Mark