[Bug 586433] Review Request: Xnoise, a media player for gtk+ written in vala

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=586433

--- Comment #5 from Lubomir Rintel <lkundrak at v3.sk> 2010-04-28 08:09:54 EDT ---
This is definitely not ready for review yet. I understand this is your first
package, please take a look at existing packages or completed reviews and
ensure you're familiar with the packaging guidelines [1].

[1] http://fedoraproject.org/wiki/Packaging/Guidelines

Thank you.

1.) Summary:        A media player for gtk+

It's GTK+, not gtk+ according to http://www.gtk.org/

2.) Getting the source file is not exactly correct

> #Get xnoise from the repository
> #   hg clone https://xnoise.googlecode.com/hg/ xnoise
> #   cd xnoise
> #   hg up -C default

You need to specify _exact_ revision which you packed and exactly how did you
create the tarball.

Also, you can use version macro in the source file name:
xnoise-%{version}.tar.gz

3.) Long lines harm legibility:

> BuildRequires:  vala, vala-devel, intltool, libtool, autogen,
> automake >= 1.11, > gnome-common, gtk2-devel, sqlite-devel,
> taglib-devel, unique-devel, gstreamer-devel,
> gstreamer-plugins-base-devel, gettext, desktop-file-utils

You can write these on separate lines:

BuildRequires:  vala
BuildRequires:  vala-devel
..

Also, note that vala is probably redundant; vala-devel will drag it in.

4.) Description is rather illegible:

> %description
> Xnoise is a media player for gtk+
> Xnoise is written in vala
> Xnoise can play every kind of audio/video data that gstreamer can handle

Please be more descriptive here and write complete sentences. What you wrote in
your blog entry [1] could fit here.

[1]
http://ratnadeepdebnath.wordpress.com/2010/04/28/rpm-packaging-xnoise-for-fedora/

Imagine how will this look in packagekit.

5.) No useless comments please:

> #%package devel
> #Summary:    Development files for %{name}
...

Either add a devel package, or delete them.

Also, please delete the extra line breaks or use them consistently.

6.) No devel files in main package

> %{_libdir}/pkgconfig/xnoise-1.0.pc

Either place this in a -devel subpackage, or %exclude it.

7.) Missing revision number in changelog

> * Tue Apr 27 2010 rtnpro <rtnpro at gmail.com> 0.1.2

This is what rpmlint complains about. Add -1 there.

8.) Use macros consistently

%{buildroot} vs. $RPM_BUILD_ROOT

http://fedoraproject.org/wiki/Packaging/Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS

9.) Extra dependencies

> Requires:   gstreamer, gstreamer-plugins-base, gtk2, gettext

I'm quite sure most of these (libraries) will be depended on automatically.
Have you checked? Is gettext really required at runtime?

10.) Arch specific?

> BuildArch:      i686

Why is this only for i686?

11.) Are you sure about the license?

src/plugins/lyricsfly/xnoise-lyricsfly-registration.vala:
> *  This program is free software; you can redistribute it and/or modify
> *  it under the terms of the GNU General Public License as published by
> *  the Free Software Foundation; either version 2 of the License, or
> *  (at your option) any later version.

That looks like GPLv2+, not GPLv2

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.