Bug 523650 - Review Request: qmpdclient - A Qt4 based MPD client
Summary: Review Request: qmpdclient - A Qt4 based MPD client
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-09-16 10:48 UTC by Julian G
Modified: 2010-02-18 22:37 UTC (History)
7 users (show)

Fixed In Version: qmpdclient-1.1.2-4.fc12
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-02-18 22:37:03 UTC
Type: ---
rdieter: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)
QMPDClient Spec File (1.90 KB, application/octet-stream)
2009-09-16 10:50 UTC, Julian G
no flags Details
Patch for Qt4 Project File (984 bytes, patch)
2009-09-16 10:51 UTC, Julian G
no flags Details | Diff
Desktop File (207 bytes, application/octet-stream)
2009-09-16 10:52 UTC, Julian G
no flags Details

Description Julian G 2009-09-16 10:48:38 UTC
QMPDClient is a Qt4 based MPD client which supports:
* Covers
* Lyrics
* Tag guessing
* Internet radio
* Storing & using playlists
* Last.fm track submission
* Tray notification
* Skinnable interface
* OSD

Version 1.1.1 Tarball: http://git.bitcheese.net/?a=archive&p=qmpdclient-ne&h=3c4b61f2aa02daee2b696a7d735680bb89d51b47&t=targz

It's my first spec file so please excuse some flaws. I will do my best to improve it.

Comment 1 Julian G 2009-09-16 10:50:45 UTC
Created attachment 361231 [details]
QMPDClient Spec File

Comment 2 Julian G 2009-09-16 10:51:49 UTC
Created attachment 361232 [details]
Patch for Qt4 Project File

Comment 3 Julian G 2009-09-16 10:52:36 UTC
Created attachment 361233 [details]
Desktop File

Comment 4 Susi Lehtola 2009-09-17 10:55:32 UTC
Added FE-NEEDSPONSOR.

Please post hyperlinks to the spec file and the SRPM generated from it instead of posting attachments to the review bug.

Comment 5 Colin Coe 2009-09-17 11:58:27 UTC
Hi

Could you please post the spec file and SRPM to a web or ftp server where they can be downloaded.  I'll have a look at the package once this is done.  

Thanks

CC

Comment 7 Colin Coe 2009-09-18 11:39:40 UTC
Initial inspection

1) The changelog entry should be %version-%release i.e. 1.1.1-1 (without the %dist tag)

2) The files section should use macros i.e. /usr/share = %_datadir 

3) There is an empty %doc tag

4) The group Application/Music is non-standard

5) Source0 should reference the full downloadable URL for the tarball

6) rpmlint reports mixed use of spaces and tabs on line of the spec file, please try and keep these consistent.  Just use one or the other.

More comments to follow.

CC

Comment 8 Colin Coe 2009-09-19 01:56:05 UTC
This fails building in koji.

+ desktop-file-install --vendor fedora --dir /builddir/build/BUILDROOT/qmpdclient-1.1.1-1.fc11.i386//usr/share/applications --add-category X-Fedora /builddir/build/SOURCES/QMPDClient.desktop
/var/tmp/rpm-tmp.9iHeZb: line 39: desktop-file-install: command not found
error: Bad exit status from /var/tmp/rpm-tmp.9iHeZb (%install)

The file 'desktop-file-install' wasn't found.  Can you please check the filename/path and providing package?

Thanks

CC

Comment 9 Rex Dieter 2009-09-24 16:37:43 UTC
for desktop-file-install, just add
BuildRequires: desktop-file-utils

Comment 10 Julian G 2009-09-24 18:54:05 UTC
I'm sorry i couldn't respond sooner. Thanks for you hints, Colin.

@Dieter: it's strange, this line is already in the spec file and it did well on OpenSuse Buildservice. Do I have to set a path?

@Colin:
1.) is fixed
2.) Do I have to define these macros?
3.) Should I remove the %doc tag if there isn't any documentation.
4.) Set to Application/Multimedia, is that correct?
5.) There is just this git url as mentioned before, is it better now?
6.) Should be fixed now.

Comment 11 Colin Coe 2009-09-24 22:13:38 UTC
Please advise where updated spec and src.rpm files can be found.  The spec changelog and release should also be updated.

The macros are defined in /usr/lib/rpm/macros file.

Thanks

CC

Comment 12 Julian G 2009-09-27 23:04:22 UTC
I updated the "old" files. Is it ok for you if I update release number and changelog next time? I didn't do it because I thought it's not a "release". If it's necessary to do it this time, please let me know.

Comment 13 Rex Dieter 2009-09-27 23:07:26 UTC
Yes, please do increment Release and add changelog entries, makes it easier to track changes and progress.

Comment 14 Colin Coe 2009-09-27 23:10:21 UTC
Julian, as an example, http://members.iinet.net.au/~coec/RackTables.spec is an example of a package I recently had reviewed.  Note the changelog entries track the issues raised by the review.

CC

Comment 15 Julian G 2009-09-30 22:49:57 UTC
Sorry, busy week. I've updated the files now.

http://files.4-web.net/qmpdclient/qmpdclient-1.1.1-2.fc10.src.rpm
http://files.4-web.net/qmpdclient/qmpdclient.spec

Thanks for your support :)

Comment 16 Thomas Janssen 2009-10-03 20:19:13 UTC
A few quick comments:

License is GPLv2+
(Look at the source code "either version 2 of the License, or (at your option) any later version.")

There's no need to include BR: gcc-c++
That's part of the minimal build environment:
http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRequires

MUST: Each package must consistently use macros.
Source0: http://files.4-web.net/%{name}/%{name}-%{release}.tar.gz
Source1: %{name}.desktop 
why do you use capital letters there?
Patch: %{name}.pro.patch

What's with the patch anyways? You have it there but dont use it.
You could comment it. But that's up to you.

Be consistent: %{_datadir} %_datadir

%doc is empty, fill it with AUTHOR COPYING INSTALL README

MUST: rpmlint must be run on every package. The output should be posted in the review.
Please post it here.

Please post as well the "Task Info link" of a koji scratch build here:
http://fedoraproject.org/wiki/PackageMaintainers/Join

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 17 Julian G 2009-10-08 13:42:12 UTC
Thanks for your reply, Thomas.

The patch gets applied in the %prep section.

Should I replace $RPM_BUILD_ROOT with %{buildroot}?

Are there any not documented options for desktop-file-install?

How do i tell the doc tag to copy the files AUTHOR COPYING INSTALL README Changelog?

Comment 18 Thomas Janssen 2009-10-08 14:16:02 UTC
I use %{buildroot} over $RPM_BUILD_ROOT, but that's up to you. As long as you dont mix it.

* %doc question
You use:
%doc AUTHOR COPYING README Changelog
Thats it.

* desktop-file-install question
http://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage
Don't apply a vendor tag.

Well, there's a man page for desktop-file-install online. But the host is veeeeeery slow.
http://olympus.het.brown.edu/cgi-bin/man/man2html?desktop-file-install+8


Why you're useing capital letters for your .desktop file?

* patch question
Normal you use: Patch0: foo.patch
and           : %patch0    OR    %patch -P 0

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 19 Julian G 2009-10-08 17:36:07 UTC
I lowered the capital letters already.

http://files.4-web.net/qmpdclient/qmpdclient-1.1.1-3.fc10.src.rpm
http://files.4-web.net/qmpdclient/qmpdclient.spec

rpmlint qmpdclient.spec qmpdclient-1.1.1-3.fc10.src.rpm
1 packages and 1 specfiles checked; 0 errors, 0 warnings.

Comment 20 Thomas Janssen 2009-10-09 09:56:28 UTC
The spec file linked in Comment #19 is still the old one.
The SRPM is not found on that server.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 21 Julian G 2009-10-09 11:56:23 UTC
Sorry, I've uploaded it into the wrong directory. Links are fixed now. I'll double check it in future ;)

Comment 22 Thomas Janssen 2009-10-11 19:33:47 UTC
[thomas@tusdell ~]$ rpmlint srpm-review-test/qmpdclient.spec
srpm-review-test/qmpdclient.spec: W: mixed-use-of-spaces-and-tabs (spaces: line 48, tab: line 1)
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

[thomas@tusdell ~]$ rpmlint rpmbuild/RPMS/x86_64/qmpdclient-*
qmpdclient-debuginfo.x86_64: E: empty-debuginfo-package
2 packages and 0 specfiles checked; 1 errors, 0 warnings.

[thomas@tusdell ~]$ rpmlint -I empty-debuginfo-package
empty-debuginfo-package:
This debuginfo package contains no files.  This is often a sign of binaries
being unexpectedly stripped too early during the build, rpmbuild not being
able to strip the binaries, the package actually being a noarch one but
erratically packaged as arch dependent, or something else.  Verify what the
case is, and if there's no way to produce useful debuginfo out of it, disable
creation of the debuginfo package.

That's what i get after i recompiled your package and run rpmlint on it.
Installed package is rpmlint clean.
I have to say that my only idea is, to ask upstream if it's known or how to get the debuginfo out..
Maybe one of the other reviewers is able to help out?

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

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 23 Julian G 2009-10-15 10:56:09 UTC
http://files.4-web.net/qmpdclient/qmpdclient-1.1.1-4.fc10.src.rpm
http://files.4-web.net/qmpdclient/qmpdclient.spec

rpmlint RPMS/i386/qmpdclient-*4* SRPMS/qmpdclient-1.1.1-4.fc10.src.rpm SPECS/qmpdclient.spec
qmpdclient-debuginfo.i386: W: hidden-file-or-dir /usr/src/debug/qmpdclient-1.1.1/.res
qmpdclient-debuginfo.i386: W: hidden-file-or-dir /usr/src/debug/qmpdclient-1.1.1/.res
qmpdclient-debuginfo.i386: W: hidden-file-or-dir /usr/src/debug/qmpdclient-1.1.1/.moc
qmpdclient-debuginfo.i386: W: hidden-file-or-dir /usr/src/debug/qmpdclient-1.1.1/.moc
qmpdclient-debuginfo.i386: W: hidden-file-or-dir /usr/src/debug/qmpdclient-1.1.1/.ui
qmpdclient-debuginfo.i386: W: hidden-file-or-dir /usr/src/debug/qmpdclient-1.1.1/.ui
3 packages and 1 specfiles checked; 0 errors, 6 warnings.

Any Ideas? Is this debuginfo package usefull or should i disable it?

Comment 24 Thomas Janssen 2009-10-21 12:06:58 UTC
[thomas@tusdell x86_64]$ rpmlint -I hidden-file-or-dir
hidden-file-or-dir:
The file or directory is hidden. You should see if this is normal, and delete
it from the package if not.

The debuginfo package looks useful to me. You might ask upstream why they use 3 hidden folder (.res, .moc, .ui) and one not hidden (src). That doesn't make sense to me, but who knows.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 25 Julian G 2009-10-22 22:55:19 UTC
I talked to the maintainer of the project. The hidden directories are created by qmake and filled with generated content by make.

Comment 26 xandry 2009-10-27 13:20:16 UTC
I'm build this package on a F11. Nice client. Very much it would be desirable to see in repository. Good job.

Comment 27 xandry 2009-11-13 20:00:58 UTC
Julian, new version is available on project homepage http://bitcheese.net/wiki/QMPDClient ;)
Why not to build it?

Comment 28 Julian G 2009-11-17 13:39:08 UTC
http://files.4-web.net/qmpdclient/qmpdclient-1.1.2-1.fc10.src.rpm
http://files.4-web.net/qmpdclient/qmpdclient.spec


rpmlint SPECS/qmpdclient.spec SRPMS/qmpdclient-1.1.2-1.fc10.src.rpm RPMS/i386/qmpdclient-1.1.2-1.fc10.i386.rpm RPMS/i386/qmpdclient-debuginfo-1.1.2-1.fc10.i386.rpm
qmpdclient-debuginfo.i386: W: hidden-file-or-dir /usr/src/debug/qmpdclient/.ui
qmpdclient-debuginfo.i386: W: hidden-file-or-dir /usr/src/debug/qmpdclient/.ui
qmpdclient-debuginfo.i386: W: hidden-file-or-dir /usr/src/debug/qmpdclient/.moc
qmpdclient-debuginfo.i386: W: hidden-file-or-dir /usr/src/debug/qmpdclient/.moc
qmpdclient-debuginfo.i386: W: hidden-file-or-dir /usr/src/debug/qmpdclient/.res
qmpdclient-debuginfo.i386: W: hidden-file-or-dir /usr/src/debug/qmpdclient/.res
3 packages and 1 specfiles checked; 0 errors, 6 warnings.

Comment 29 Julian G 2010-01-06 21:04:29 UTC
What else is needed to get this into the repos?

Comment 30 xandry 2010-01-07 01:04:00 UTC
Because, that this is your first package, you need a sponsor.

Comment 31 Julian G 2010-01-20 11:41:59 UTC
http://files.4-web.net/qmpdclient/qmpdclient-1.1.2-2.fc12.src.rpm
http://files.4-web.net/qmpdclient/qmpdclient.spec

rpmlint SPECS/qmpdclient.spec SRPMS/qmpdclient-1.1.2-2.fc12.src.rpm RPMS/x86_64/qmpdclient*-1.1.2-2.fc12.x86_64.rpm
qmpdclient.x86_64: W: unstripped-binary-or-object /usr/bin/qmpdclient
2 packages and 1 specfiles checked; 0 errors, 1 warnings.

Comment 32 Rex Dieter 2010-01-20 21:06:47 UTC
SHOULD:  drop "Qt4 based" from package summary/description.  (hardly) anyone cares what toolkit is used, as long as the application works.

MUST: drop Requires: qt  
not needed

MUST: fix desktop-file-install usage
(see also http://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files)
1.  don't use '--add-category X-Fedora'
2.  no need to run both desktop-file-install *and* desktop-file-validate (the former is sufficient).

MUST: update icon-related scriptlets, see,
http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

MUST: please install qmpdclient64.png icon too (most DE's don't handle svg icons well or at all)

MUST: please adjust the patch to drop
QMAKE_LFLAGS_RELEASE adjustments completely.  These are not needed or desired either (this part should be upstreamable, they shouldn't be mucking with these flags).

Comment 33 Julian G 2010-01-21 01:23:30 UTC
There are lots of MPD Clients (http://mpd.wikia.com/wiki/Clients) so in my opinion it's a useful information if I'm searching for a client which fits into my desktop environment.

Is it possible to use qmpdclient64.png as fallback referenced in the desktop file?

Many thanks for your help :)

http://files.4-web.net/qmpdclient/qmpdclient-1.1.2-3.fc12.src.rpm
http://files.4-web.net/qmpdclient/qmpdclient.spec

rpmlint SPECS/qmpdclient.spec SRPMS/qmpdclient-1.1.2-3.fc12.src.rpm RPMS/x86_64/qmpdclient*-1.1.2-3.fc12.x86_64.rpm
qmpdclient.x86_64: W: unstripped-binary-or-object /usr/bin/qmpdclient
2 packages and 1 specfiles checked; 0 errors, 1 warnings.

Comment 34 Rex Dieter 2010-01-22 18:28:07 UTC
Looks good, thanks.

APPROVED.

(what's your FAS username?  given that, I can sponsor you).

Comment 35 Julian G 2010-01-22 23:11:11 UTC
Thx :) I've just created an FAS account (jgold).

Comment 36 Rex Dieter 2010-01-23 01:31:47 UTC
OK, sponsored.

continue on in the steps outlined in
http://fedoraproject.org/wiki/PackageMaintainers/Join
(ie, submit a cvsadmin request, and start to setup fedora-packager tools).

Feel free to contact me if you ever have any questions or need help.

Comment 37 Julian G 2010-01-26 11:08:28 UTC
New Package CVS Request
=======================
Package Name: qmpdclient
Short Description: Qt4 based MPD client
Owners: jgold
Branches: F-12
InitialCC: jgold

Comment 38 Jason Tibbitts 2010-01-27 04:54:27 UTC
CVS done (by process-cvs-requests.py).

Comment 39 Fedora Update System 2010-01-29 20:39:40 UTC
qmpdclient-1.1.2-3.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/qmpdclient-1.1.2-3.fc12

Comment 40 Julian G 2010-01-29 21:05:18 UTC
Thanks everyone. :)

Comment 41 nucleo 2010-01-30 03:09:38 UTC
I think that 'make install' is not working well because not install translations.
All files can be installed manually.

Here spec which not uses 'make install'
http://nucleo.fedorapeople.org/qmpdclient.spec

%find_lang not works, so I have used %lang.
May be somehow %find_lang can work in this case but I don't know how.

Comment 42 nucleo 2010-01-30 03:57:43 UTC
The other solution is to use cmake. Then patching is not needed.
http://nucleo.fedorapeople.org/qmpdclient-cmake.spec

desktop file installs but it uses icon that installs in wrong place.

Comment 43 Fedora Update System 2010-02-01 01:06:09 UTC
qmpdclient-1.1.2-3.fc12 has been pushed to the Fedora 12 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 qmpdclient'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F12/FEDORA-2010-1246

Comment 44 Julian G 2010-02-03 19:04:27 UTC
Thank you nucleo! The cmake way seems more accurate.

http://koji.fedoraproject.org/koji/packageinfo?packageID=9835

Comment 45 Fedora Update System 2010-02-05 01:33:46 UTC
qmpdclient-1.1.2-4.fc12 has been pushed to the Fedora 12 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 qmpdclient'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F12/FEDORA-2010-1246

Comment 46 Fedora Update System 2010-02-18 22:36:57 UTC
qmpdclient-1.1.2-4.fc12 has been pushed to the Fedora 12 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.