Re: [PATCH] gnu: Add rhythmbox.

[David Hashe]

Thanks for the review.

On Thu, Jun 18, 2015 at 8:45 AM, Ricardo Wurmus <address@hidden> wrote:

David Hashe <address@hidden> writes:

> * gnu/packages/gnome.scm (rhythmbox): New variable.

[...]

> +(define-public rhythmbox
> + (package
> +   (name "rhythmbox")
> +   (version "3.2.1")
> +   (source (origin
> +            (method url-fetch)
> +            (uri (string-append "mirror://gnome/sources/rhythmbox/3.2/"

Can you use (version-major+minor version) instead of “3.2” here?

I also replaced each instance of "rhythmbox" with name.

 

> +                                "rhythmbox-" version ".tar.xz"))
> +            (sha256
> +             (base32
> +              "0f3radhlji7rxl760yl2vm49fvfslympxrpm8497acbmbd7wlhxz"))))
> +   (build-system glib-or-gtk-build-system)
> +  (native-inputs
> +    `(("intltool" ,intltool)
> +      ("glib" ,glib "bin")
> +      ("gobject-introspection" ,gobject-introspection)
> +      ("pkg-config" ,pkg-config)))

The indentation of (native-inputs ...) is wrong.
 

> +   (inputs
> +    `(("json-glib" ,json-glib)

[...]

> +      ("brasero" ,brasero)))

Is Brasero an optional input?  It’s a CD burning application, which
seems unrelated to a music player.  Will Totem work even if Brasero is
not available at build time?  Or does it integrate more deeply with
Brasero?

It's an optional input. Rhythmbox uses it (specifically libbrasero-media) to allow burning playlists straight to CD.

Your comment also made me realize that I had the home-page wrong; I've fixed that.

 

> +   (description "Rhythmbox is a music playing application for GNOME. It supports
> +playlists, song ratings, and any codecs installed through
> gstreamer.")

Please use two spaces at the end of a sentence.

~~ Ricardo

Updated patch attached.

David

0001-gnu-Add-rhythmbox.patch
Description: Text Data