Bug 667226 - Review Request: tepsonic - A simple, fast and lightweight Qt audio player
Summary: Review Request: tepsonic - A simple, fast and lightweight Qt audio player
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
low
medium
Target Milestone: ---
Assignee: Volker Fröhlich
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 666633
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-01-04 20:49 UTC by Dan Vratil
Modified: 2011-01-26 21:00 UTC (History)
3 users (show)

Fixed In Version: tepsonic-0.98.1-5.fc14
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-01-26 21:00:59 UTC
Type: ---
volker27: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Dan Vratil 2011-01-04 20:49:31 UTC
Spec URL: http://www.tepsonic.org/files/fedora/tepsonic.spec
SRPM URL: http://www.tepsonic.org/files/fedora/tepsonic-0.98.1-1.fc14.src.rpm

Description: a simple, fast and lightweight multiplaform Qt audio player with collections, plugins and all the basic functions that every good player should have.

Comment 1 Volker Fröhlich 2011-01-06 17:26:27 UTC
Some initial comments:

The license seems to be GPLv3+:

 * 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 3
 * of the License, or (at your option) any later version.

Please don't put all the Build Requires in one line, since it's not very clear.

There is no package called lastfmlib-devel in Fedora. The name should be
liblastfm-devel and the package doesn't build for me.

The source also contains a copy of qtx. Please make sure it isn't used. Maybe
delete it.

I noticed, the player also works with MySQL. Does that need any preparation?

Please don't mix $RPMBUILDROOT and %{buildroot} -- simply opt for one.

I guess, a full stop can replace %{_builddir}/%{name}-%{version} from the cmake
call. /usr should be %{_prefix}, but first check, if they are all necessary.

Please use %{_datadir}, to substitute for /usr/share -- also in the files
section. The category list in the desktop file should terminate in a semicolon
(desktop-file-validate).

Probably put the %doc first in the files section (after %defattr).

Comment 2 Dan Vratil 2011-01-06 17:43:50 UTC
The lastfmlib package (https://bugzilla.redhat.com/show_bug.cgi?id=666633) was just approved and I'm working on it. liblastfm is an another package.

The Qxt source is needed to provide cross-platform global shortcuts. There is not whole Qxt source included, only the few files needed to provide the required functionality.

Could you please provide an explanation about the replacement of %{_buildir}/%{name}-%{version} in cmake cmd? I'm not sure I understand it.

Thank you

Comment 3 Volker Fröhlich 2011-01-06 19:05:11 UTC
Ah, I didn't know about that package.

Fedora has it's own libqxt. Please use that instead.

cmake \
        -DCMAKE_INSTALL_PREFIX=%{_prefix} \
        -DCMAKE_BUILD_TYPE=Release \
        -DCMAKE_INSTALL_LIBRARY_DIR=%{_libdir}/%{name} \
        .

Notice the fullstop that stands for the current directory instead of stating its full path. Also notice %{_prefix}.

Comment 4 Dan Vratil 2011-01-06 19:22:29 UTC
Thanks for the cmake. 
About the Qxt - the libqxt (or just the one class to be exact) is statically linked into the executable binary, so no duplicate libqxt.so will be produced.

I can fix the CMakeLists.txt to try look after libqxt.so first and then eventually use the included sources, but it will appear first in 0.98.2.

Comment 5 Dan Vratil 2011-01-08 21:45:28 UTC
The lastfmlib is now available in the updates-testing repository.

I updated the .spec file fixing all the issues you mentioned. For MySQL I added qt-mysql to dependencies. I just left the qxt sources because of reasons mention in the comment above.

Spec URL: http://www.tepsonic.org/files/fedora/tepsonic.spec
SRPM URL: http://www.tepsonic.org/files/fedora/tepsonic-0.98.1-2.fc14.src.rpm

Comment 6 Volker Fröhlich 2011-01-14 02:57:10 UTC
You forgot to make a changelog entry. The changelog should reflect the changes made to the spec file. You can avoid this and other glitches by running rpmlint on your spec file or src.rpm. That would have also saved you from this one:

%{_datadir}/share/tepsonic --> %{_datadir}/%{name}

Taking a look at desktop files with desktop-file-validate, the categories list doesn't end in a semicolon. sed -i 's/\(^Categories.*\)/\1;/g' %{name}.desktop

%{_prefix}/share/ --> %{_datadir}

Please use rm -f or rm -rf instead of the rm macro.

Please patch CMakeLists.txt for qxt. If Fedora shipped a new version of qxt, it could be break. 

Please align "qt-mysql". It now uses a tabulator.

The license is still stated as GPLv3, although the files say GPLv3+.

Please also take a look at http://fedoraproject.org/wiki/Packaging/Guidelines#Handling_Locale_Files

Comment 7 Dan Vratil 2011-01-14 18:18:18 UTC
I fixed the paths variables, desktop file using your sed, removed the %{rm} macro, fixed tabulator-issue, license and did some patching around Qxt and locale files (I also pushed the changes to upstream, so in next release it will work without the patches).

I'm just not sure, if gettext should be a dependency when I'm using Qt's locale format, but I added it.

Spec URL: http://www.tepsonic.org/files/fedora/tepsonic.spec
SRPM URL: http://www.tepsonic.org/files/fedora/tepsonic-0.98.1-3.fc14.src.rpm

Comment 8 Volker Fröhlich 2011-01-15 23:37:04 UTC
No, Gettext is only necessary if the project uses .po/.mo. The Phonon Requires is also not necessary, as RPM finds that on it's own.

I erroneously checked the original desktop file the first time. You can delete the sed call, because --add-category solves the syntax error. But "application" is regarded as invalid, so:

desktop-file-install --dir=%{buildroot}/%{_datadir}/applications --remove-category="Application" --add-category="AudioVideo" %{name}.desktop

You're using custom permissions for installing the executable. After compilation its 755, after installing 555. I removed the custom settings with this rough sed:

sed -i '/PERMISSIONS/d' player/CMakeLists.txt

Afterwards, the file section looks like that:

%files -f %{name}.lang -f lastfmscrobbler.lang
%defattr(-,root,root,-)
%doc LICENSE README
%{_bindir}/*
%{_libdir}/%{name}/*.so.*
%{_datadir}/applications/*
%{_datadir}/icons/*


Some cosmetic improvements:

You can also simplify to %cmake -DCMAKE_INSTALL_LIBRARY_DIR=%{_libdir}/%{name} .

You can use something like "%patch0 -p1 -b .qxt", so you can have a look at the patched sources, if necessary, and get a kind of comment as well, as a side effect.

If you don't need the .so link in general, you may consider the use of NAMELINK_SKIP in CMakeLists.txt. For packaging, deleting it is also fine. If you stay with rm: Your comment there says, it would delete all NON-developer libs, which is wrong, I think.


That should be it. I'll do the formal review after you submitted your changes.

Comment 9 Dan Vratil 2011-01-16 00:04:42 UTC
I did all proposed changes.

Ad removing non-developer libs, the Packaging guide states, that unversioned .so should be in -devel. Since TepSonic does not have any devel package and the libraries are just plugins which nothing will be linked against, the developer libs seem quite useless to me. They are just included, because some distributions require it (at least I was told so by packagers).

New sources:

Spec URL: http://www.tepsonic.org/files/fedora/tepsonic.spec
SRPM URL: http://www.tepsonic.org/files/fedora/tepsonic-0.98.1-4.fc14.src.rpm

Comment 10 Volker Fröhlich 2011-01-16 01:40:22 UTC
You dropped the BuildRequires instead of the Requires for Phonon.

Please re-add:

BuildRequires: phonon-devel

and delete

Requires: phonon

Please mend that, because otherwise the package won't build. 

Here is the review.

- Package meets naming guidelines
- Spec file matches base package name.
- Spec has consistent macro usage.
- Meets Packaging Guidelines.
- License acceptable for Fedora
- License field in spec matches
- License file included in package
- Spec in American English
- Spec is legible.
- Sources match upstream md5sum: aba7189133005260fcf85046a5f6e80c
- BuildRequires correct
- Spec handles locales/find_lang
- Package has %defattr and permissions on files is good.
- Package has a correct %clean section.
- Package is code
- Packages %doc files don't affect runtime.
- Package is a GUI app and has a .desktop file

- Package has no duplicate files in %files.
- Package doesn't own any directories other packages own.
- Package owns all the directories it creates.

SHOULD Items:

- Functions as described
- Has dist tag
- Packages latest version


The comment should say "Remove all devel libraries ..." not "Remove all non-devel libraries ...".

As you don't use a buildroot tag, you can drop the clean section and the rm -rf %{buildroot} in %install. But it doesn't do any harm.

Tests MISSING until you revised:

- Requires and provides
- rpmlint
- Package compiles and builds on at least one arch:
- Should build on all supported archs -- no buildroot, so it's only Intel.

Comment 11 Dan Vratil 2011-01-16 09:57:31 UTC
Fixed Phonon dependency, comment and removed the rm command from install and clean sections.

New sources:
Spec URL: http://www.tepsonic.org/files/fedora/tepsonic.spec
SRPM URL: http://www.tepsonic.org/files/fedora/tepsonic-0.98.1-5.fc14.src.rpm

Comment 12 Volker Fröhlich 2011-01-16 17:08:05 UTC
- rpmlint:

tepsonic.x86_64: W: no-manual-page-for-binary tepsonic
3 packages and 0 specfiles checked; 0 errors, 1 warnings.

- Should build on all supported archs -- no buildroot, so it's only Intel.

http://koji.fedoraproject.org/koji/taskinfo?taskID=2724370

- Package builds in Mock

For the future: Please keep the changelog as accurate as you can.

- Requires and provides

Provides: 

libtepsonic_lastfmscrobbler.so.1()(64bit)  
tepsonic = 0.98.1-5.fc13
tepsonic(x86-64) = 0.98.1-5.fc13

tepsonic-debuginfo = 0.98.1-5.fc13
tepsonic-debuginfo(x86-64) = 0.98.1-5.fc13

Requires (without libc and rpmlib):

libICE.so.6()(64bit)  
libQtCore.so.4()(64bit)  
libQtGui.so.4()(64bit)  
libQtNetwork.so.4()(64bit)  
libQtSql.so.4()(64bit)  
libQxtCore.so.0()(64bit)  
libQxtGui.so.0()(64bit)  
libSM.so.6()(64bit)  
libX11.so.6()(64bit)  
libXcursor.so.1()(64bit)  
libXext.so.6()(64bit)  
libXfixes.so.3()(64bit)  
libXi.so.6()(64bit)  
libXinerama.so.1()(64bit)  
libXrandr.so.2()(64bit)  
libXrender.so.1()(64bit)  
libcrypto.so.10()(64bit)  
libdl.so.2()(64bit)  
libfontconfig.so.1()(64bit)  
libfreetype.so.6()(64bit)  
libgcc_s.so.1()(64bit)  
libgcc_s.so.1(GCC_3.0)(64bit)  
libglib-2.0.so.0()(64bit)  
libgobject-2.0.so.0()(64bit)  
libgthread-2.0.so.0()(64bit)  
liblastfmlib.so.1()(64bit)  
libm.so.6()(64bit)  
libphonon.so.4()(64bit)  
libpng12.so.0()(64bit)  
libpthread.so.0()(64bit)  
libpthread.so.0(GLIBC_2.2.5)(64bit)  
librt.so.1()(64bit)  
libssl.so.10()(64bit)  
libstdc++.so.6()(64bit)  
libstdc++.so.6(CXXABI_1.3)(64bit)  
libstdc++.so.6(GLIBCXX_3.4)(64bit)  
libtag.so.1()(64bit)  
libtepsonic_lastfmscrobbler.so.1()(64bit)  
libz.so.1()(64bit)  
qt-mysql  
qt-sqlite  
rtld(GNU_HASH)

--------
APPROVED
--------

You can request SCM.

Comment 13 Dan Vratil 2011-01-16 17:16:22 UTC
ew Package SCM Request
=======================
Package Name: tepsonic
Short Description: A simple, fast and lightweight multiplaform Qt audio player with collections, plugins and all the basic functions that every good player should have.
Owners: progdan
Branches: f14
InitialCC:

Comment 14 Dan Vratil 2011-01-16 17:16:43 UTC
Package SCM Request
=======================
Package Name: tepsonic
Short Description: A simple, fast and lightweight multiplaform Qt audio player
with collections, plugins and all the basic functions that every good player
should have.
Owners: progdan
Branches: f14
InitialCC:

Comment 15 Jens Petersen 2011-01-18 07:24:22 UTC
Invalid Request format: "New Package SCM Request" is correct.

Also please shorten the description to a few words (probably the package Summary field).

Comment 16 Dan Vratil 2011-01-18 09:18:26 UTC
New Package SCM Request
=======================
Package Name: tepsonic
Short Description: A simple, fast and lightweight Qt audio player.
Owners: progdan
Branches: f14
InitialCC:

Comment 17 Jason Tibbitts 2011-01-18 13:58:28 UTC
Git done (by process-git-requests).

Comment 18 Fedora Update System 2011-01-18 14:46:20 UTC
tepsonic-0.98.1-5.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/tepsonic-0.98.1-5.fc14

Comment 19 Fedora Update System 2011-01-18 21:36:30 UTC
tepsonic-0.98.1-5.fc14 has been pushed to the Fedora 14 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update tepsonic'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/tepsonic-0.98.1-5.fc14

Comment 20 Fedora Update System 2011-01-26 21:00:51 UTC
tepsonic-0.98.1-5.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.


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