Bug 586433 - Review Request: Xnoise, a media player for gtk+ written in vala
Review Request: Xnoise, a media player for gtk+ written in vala
Status: CLOSED DUPLICATE of bug 607777
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Lubomir Rintel
Fedora Extras Quality Assurance
http://rtnpro.dgplug.org/Packages/xno...
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-27 10:55 EDT by Ratnadeep Debnath
Modified: 2013-10-19 10:42 EDT (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-06-25 05:26:52 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lkundrak: fedora‑review?


Attachments (Terms of Use)
xnoise source rpm (1.81 MB, application/x-rpm)
2010-04-27 10:55 EDT, Ratnadeep Debnath
no flags Details

  None (edit)
Description Ratnadeep Debnath 2010-04-27 10:55:26 EDT
Created attachment 409495 [details]
xnoise source rpm

Description of problem:
new package, 1 rpmlint error

Version-Release number of selected component (if applicable):


How reproducible:


Steps to Reproduce:
1.Download the xnoise-0.1.2-1.fc12.src.rpm
2.rpm -ivh xnoise-0.1.2-1.fc12.src.rpm
3.cd ~/rpmbuild/SPECS; rpmbuild -ba xnoise.spec
4. rpmlint xnoise.spec ../RPMS/*/xnoise-0.1.2-1.fc12.rpm ../SRPMS/xnoise-0.1.2-1.fc12.src.rpm
  
Actual results:

xnoise.i686: W: incoherent-version-in-changelog 0.1.2 ['0.1.2-1.fc12', '0.1.2-1']
xnoise.i686: W: devel-file-in-non-devel-package /usr/lib/pkgconfig/xnoise-1.0.pc
xnoise.i686: E: incorrect-locale-subdir /usr/share/locale/default/LC_MESSAGES/xnoise.mo
2 packages and 1 specfiles checked; 1 errors, 2 warnings.


Expected results:

Should not give any error

Additional info:
Comment 1 Ratnadeep Debnath 2010-04-27 11:41:34 EDT
SPEC URL: http://rtnpro.dgplug.org/Packages/xnoise/xnoise.spec
SRPM URL: http://rtnpro.dgplug.org/Packages/xnoise/xnoise-0.1.2-1.fc12.src.rpm
homepage: http://code.google.com/p/xnoise/
Xnoise : A media player for gtk+
Comment 2 Ratnadeep Debnath 2010-04-27 13:51:04 EDT
koji scratch build link for xnoise :
https://koji.fedoraproject.org/koji/taskinfo?taskID=2141673
Comment 3 Ratnadeep Debnath 2010-04-27 19:45:13 EDT
**koji scratch build link in comment 2 obsoleted**

koji scratch build link for xnoise:
https://koji.fedoraproject.org/koji/taskinfo?taskID=2141814
Comment 4 Lubomir Rintel 2010-04-28 07:46:21 EDT
Taking this for review.
Comment 5 Lubomir Rintel 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@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
Comment 6 Ratnadeep Debnath 2010-04-30 22:38:07 EDT
Thanks for the review.

I made the suggested changes in the xnoise.spec file and submitted a koji scratch build at 
https://koji.fedoraproject.org/koji/taskinfo?taskID=2152364

> 9.) Extra dependencies

It seems that the package is working correctly without the "Requires" line. So commented it out for the time being.

I had a talk with the xnoise upstream and I came to know that xnoise will be officially released after some testing and fixes. They also plan to split the xnoise and the plugins, so that it will be possible for the users to choose what dependencies to pull in. This will result in two or more packages for xnoise (xnoise and xnoise-plugins-default for example).

This will then requires some changes in the xnoise.spec file.

Updated the SRPM and spec files at :
SRPM URL: http://rtnpro.dgplug.org/Packages/xnoise/xnoise-0.1.2-1.fc12.src.rpm
SPEC URL: http://rtnpro.dgplug.org/Packages/xnoise/xnoise.spec
Comment 7 Christoph Wickert 2010-05-01 03:58:18 EDT
In the %description, please insert line breaks at 80 characters, otherwise they wont fit on a terminal. Please don't start every sentence with Xnoise and please don't mention technical things like vala that are not important to the users.

Suggestion:

Xnoise is a media player written in GTK+ that can play every kind of audio/video data that gstreamer can handle. It uses a tracklist centric design and a hierarchical tree structure media browser along with plugin interface.

Xnoise is always running in a single instance, this means that additional files will always be added to the tracklist instead of a new instance.


Some more comments:

You are using both %{buildroot} and ${RPM_BUILD_ROOT}. There is no benefit from one or the other, but please only use one macro style for consistency.

The package doesn't own %{_datadir}/xnoise/ but only the files inside, thus an empty folder will remain after uninstall.

The package contains libtool archives (*.la files) which is strictly forbidden in Fedora. You will have to remove them in %install with something like
rm -f ${RPM_BUILD_ROOT}%{_libdir}/xnoise/*.la

When you install icons to /usr/share/icons, you need to update the gtk icon-cache, see http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache

/usr/share/locale/default/LC_MESSAGES/xnoise.mo seems bogus

Please add a blank line between every changelog entry
Comment 8 Ratnadeep Debnath 2010-05-19 14:21:25 EDT
Updated the spec and srpm files according to comment #7
SPEC URL: http://rtnpro.fedorapeople.org/Packages/SPECS/xnoise.spec
SRPM URL: http://rtnpro.fedorapeople.org/Packages/SRPMS/xnoise-0.1.2-1.fc12.src.rpm
Comment 9 Chen Lei 2010-05-24 11:07:27 EDT
Some more suggestions:

The tarball bundles two seperate projects - xnoise and xnoise-plugins-core.
It may not be appropriate.

It's seems 0.1.2 is still a prerelease, you should modify you release field to match naming guideline.
See http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-Release_packages

I highly suggest you to preprocess vala code to pure C in spec and not ship preprocessed code in you source tarball.
Comment 10 Ratnadeep Debnath 2010-06-25 05:26:52 EDT
I will not be available for some days, till 1st week of July 2010.
 Michel Alexandre Salim (http://salimma.fedorapeople.org/) wanted to 
package this. So he may take up xnoise for packaging.
Comment 11 Michel Alexandre Salim 2010-06-25 06:13:22 EDT
Thanks. I'm making this request a duplicate of the new one, so interested parties get transferred over.

@Chen Lei -- xnoise-plugins-core is a separate review that depends on the new xnoise review

*** This bug has been marked as a duplicate of bug 607777 ***

Note You need to log in before you can comment on or make changes to this bug.