Spec URL: http://sn.bluehost.cz/tmp/no-mp3/qmmp.spec SRPM URL: http://sn.bluehost.cz/tmp/no-mp3/qmmp-0.1.3.1-2.fc7.src.rpm Description: Hello. I just packed qmmp for use in Fedora. Qmmp stands for "Qt-based MultiMedia Player" which is quite selfexplaining, although it supports audio only. The player resembles old WinAmp/XMMS style. This package disables support for ffmpeg and mad to meet the no-mp3 policy. The SRPM contains Czech translation which is not yet available upstream (this is the reason why the .tar.bz2 differs from the one downloadable at the project page). This is my first package, so I need a sponsor.
(In reply to comment #0) > Spec URL: http://sn.bluehost.cz/tmp/no-mp3/qmmp.spec > SRPM URL: http://sn.bluehost.cz/tmp/no-mp3/qmmp-0.1.3.1-2.fc7.src.rpm SRPM URL: http://sn.bluehost.cz/tmp/no-mp3/qmmp-0.1.4-1.fc7.src.rpm - new version available (.spec updated too)
Quick initial comments: 1. Drop Requires: flac >= 1.1.3 jack-audio-connection-kit >= 0.102.5 libmpcdec >= 1.2.2 alsa-lib taglib qt4 >= 4.2 rpm should auto-req these without listing them explicitly. 2. wrt desktop-file-install usage, drop --add-category X-Fedora --add-category X-Red-Hat-Extra see also: http://fedoraproject.org/wiki/Packaging/Guidelines#head-d559ee7363418a5840ce63090c608c991cd39ce6
(In reply to comment #2) > Quick initial comments: > 1. Drop > Requires: flac >= 1.1.3 jack-audio-connection-kit >= 0.102.5 libmpcdec >= 1.2.2 > alsa-lib taglib qt4 >= 4.2 > rpm should auto-req these without listing them explicitly. I've already dropped dependencies on libogg etc., as suggested, but those left were not mentioned by rpmlint and I did not "just try" ... > 2. wrt desktop-file-install usage, drop > --add-category X-Fedora --add-category X-Red-Hat-Extra > see also: > http://fedoraproject.org/wiki/Packaging/Guidelines#head-d559ee7363418a5840ce63090c608c991cd39ce6 "X-Fedora" comes from investigating the other packages ... "X-Red-Hat-Extra" comes from the documentation: http://docs.fedoraproject.org/developers-guide/ch-menus.html#s1-categories if this is no longer valid then it should be updated Spec URL: http://sn.bluehost.cz/tmp/no-mp3/qmmp.spec (overwritten with new version) SRPM URL: http://sn.bluehost.cz/tmp/no-mp3/qmmp-0.1.4-2.fc7.src.rpm
* missing "BuildRequires: cmake" * not building with $RPM_OPT_FLAGS / %{optflags} (run make VERBOSE=1 to see) * hint: http://fedoraproject.org/wiki/Packaging/cmake * licence seems to be GPLv2+ source files contain the "or any later version" phrase) * build failed here due to SMP make flags: ... [ 24%] Building CXX object lib/CMakeFiles/qmmp.dir/moc_iir.o /usr/bin/ld: cannot find -lqmmp collect2: ld returned 1 exit status make[2]: *** [lib/qmmp/Input/vorbis/libvorbis.so] Error 1 make[1]: *** [lib/qmmp/Input/vorbis/CMakeFiles/vorbis.dir/all] Error 2 make[1]: *** Waiting for unfinished jobs.... * dropping the %{_smp_mflags} make the build-job succeed * GPLv2 text is not included! (only compiled in) * shared libraries are not chmod +x * rpmlint qmmp-0.1.4-2.fc8.i386.rpm qmmp.i386: E: invalid-soname /usr/lib/libqmmp.so libqmmp.so => acceptable during development, but it requires strictly versioned "Requires" in plugin packages * package provides SONAMEs it must not provide except for "libqmmp.so": $ rpm -qp --provides qmmp-0.1.4-2.fc8.i386.rpm | grep lib libalsa.so libflac.so libjack.so libmpc.so libqmmp.so libvorbis.so The others come from the plugin libraries. It is highly recommended to filter them out as not to disturb what legitimate library packages provide/require. http://fedoraproject.org/wiki/PackagingDrafts/FilteringAutomaticDependencies
(In reply to comment #4) > * missing "BuildRequires: cmake" oops, originally I used qmake and I forgot to add cmake later ... - fixed > * not building with $RPM_OPT_FLAGS / %{optflags} > (run make VERBOSE=1 to see) - it should be fixed by using %cmake macro (?) > * licence seems to be GPLv2+ > source files contain the "or any later version" phrase) you are right; I was confused by some web and did not look into the sources - fixed > * build failed here due to SMP make flags: I use -j3 (dualcore Xeon) and it works_for_me(tm) - do you have any idea why the error happens? I am not a programmer, it would take me ages to figure it out (especially when I cannot reproduce) :-( > * GPLv2 text is not included! (only compiled in) - fixed (added COPYING to %doc) > * shared libraries are not chmod +x - fixed > * rpmlint qmmp-0.1.4-2.fc8.i386.rpm > qmmp.i386: E: invalid-soname /usr/lib/libqmmp.so libqmmp.so > => acceptable during development, but it requires strictly > versioned "Requires" in plugin packages I guess this is upstream problem? - should I rename the library somehow and make a symlink at the end of %install? > * package provides SONAMEs it must not provide except > for "libqmmp.so": - fixed new SRPM: http://sn.bluehost.cz/tmp/no-mp3/qmmp-0.1.4-3.fc7.src.rpm
> I use -j3 (dualcore Xeon) and it works_for_me(tm) - > do you have any idea why the error happens? I am not > a programmer, it would take me ages to figure it > out (especially when I cannot reproduce) :-( You would simply drop the %{_smp_mflags} from the spec, add a comment in the same line and be done. However, if you believe it builds fine *always*, keep the SMP make flags and expect the unexpected. Keep it until it breaks. ;) > - should I rename the library somehow and make a symlink > at the end of %install? That won't help at all, since it doesn't change the SONAME of the library, and plugins would still depend on that SONAME. It's solely a matter of API/ABI stability of libqmmp.so -- stuff that is linked against libqmmp.so requires a specific interface version of the lib, not *any* libqmmp.so. Sooner or later there may be additional plugin packages which would be built for a specific version of libqmmp, and if that is not reflected in a properly versioned library soname dependency, it is up to the packagers to hardcode versions as "Requires" in their packages manually and avoid that an upgrade of qmmp breaks plugin pkgs by changing libqmmp in incompatible ways. Don't worry yet, just be aware of the implications of a version-less soname in run-time linker's search path.
(In reply to comment #6) > You would simply drop the %{_smp_mflags} from the spec, add > a comment in the same line and be done. However, if you believe > it builds fine *always*, keep the SMP make flags and expect > the unexpected. Keep it until it breaks. ;) I know that dropping parallel compilation would help, but I would like to see a real solution, not just workaround ... if this beast builds on all of my systems (which include two Gentoo boxes using distcc and cross-compiling, old AMD Duron vs Intel Core2 Duo) and it fails on your box then we should find what is "wrong" on your system I do not know what is the official Fedora policy in this case, but I would like to know whether it builds in Brew (or what is currently used?) - if not, then smp has to be removed, if it builds, then I would let it go ... being aware of the bug and eager to resolve it with the help of the affected users > Don't worry yet, just be aware of the implications of a version-less > soname in run-time linker's search path. this is no problem for me, but if 3rd party plugin comes and someone else packages it ...
FYI, simply rebuilding from .spec works for Fedora 8 new SRPM: http://sn.bluehost.cz/tmp/no-mp3/qmmp-0.1.4-3.fc8.src.rpm
Hi Karel, I see that you are looking for someone to sponsor you, and as it happens I'm allowed to sponsor people. Here is what I would like to do: I see that you've submitted 3 packages for review, I will review all 3 of them and then after one or two iterations the can hopefully be approved, assuming that process go well I'll sponsor you when all 3 are approved.
Here are the Must Fix and Should fix items resulting from a full review, notice that I've also recycled some of the comments above, thanks to all involved for those! Must Fix: --------- * buildroot does not match the buildroot mandated by the guidelines * remove "* MPEG1 layer 1/2/3 support;" from %description * BuildRequires line longer then 80 chars, please split this up in multiple lines each starting with BuildRequires: * desktop-file-install line longer then 80 chars, please split this up in multiple lines. * use %defattr(-,root,root,-) * put %doc directly under %defattr * drop %{?_smp_mflags}, if it fails on some systems it must be dropped, the fact that it happens to work on others is not relevant, we don't want a lottery, we want reproducable builds * add: "Requires(post): /sbin/ldconfig" and "Requires(postun): /sbin/ldconfig" * All these lines: %dir %{_libdir}/qmmp %dir %{_libdir}/qmmp/Input %dir %{_libdir}/qmmp/Output %{_libdir}/qmmp/Input/*.so %{_libdir}/qmmp/Output/*.so Can be written as just: %{_libdir}/qmmp Notice that you will still need: %{_libdir}/libqmmp.so Listed seperately * Drop the second %defattr line Possible improvements: ---------------------- * Use single quotes instead of double quotes around your sed scripts so that you do not have to use \ infront of " inside the scripts.
Hmm, I've just find out I've been mixing you up with someone else who also needs a sponsor and who has 2 packages submitted for review, hence my: "I see that you've submitted 3 packages for review" remark, my bad. Please submit some more packages and / or do some pre reviews (some normal reviews were you clearly indicate you cannot do official reviews as you are not sponsored yet), thanks.
(In reply to comment #10) > * buildroot does not match the buildroot mandated by the guidelines there is: BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root reading the guidelines http://fedoraproject.org/wiki/Packaging/Guidelines#head-b4fdd45fa76cbf54c885ef0836361319ab962473 The recommended values for the BuildRoot tag are (in descending order of preference) : %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX) %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) %{_tmppath}/%{name}-%{version}-%{release}-root so it matches exactly the third line from the guidelines, what is the problem? > * remove "* MPEG1 layer 1/2/3 support;" from %description done > * BuildRequires line longer then 80 chars, please split this up in multiple > lines each starting with BuildRequires: > * desktop-file-install line longer then 80 chars, please split this up in > multiple lines. done btw, where comes this requirement from? - reading the guidelines, I see that only the Description cannot have lines longer than 80 chars (http://fedoraproject.org/wiki/Packaging/Guidelines#head-ef67b32cfe3903b0aaab1b3c920940769007da6a) > * use %defattr(-,root,root,-) done ... but this produces the following rpmlint error: qmmp.x86_64: E: non-readable /usr/bin/qmmp 0601 I am confused where this comes from, since the compiled qmmp binary has 755 and I see no change on install of that file unfortunately, the executable not being readable means that the included resources are unusable, effectively disabling translations > * put %doc directly under %defattr done btw, once again, where this requirement comes from? > * drop %{?_smp_mflags}, if it fails on some systems it must be dropped, the fact > that it happens to work on others is not relevant, we don't want a lottery, we > want reproducable builds ok ... but I thought that it would be better to fix one b0rk3n machine than crippling the build everywhere > * add: "Requires(post): /sbin/ldconfig" and "Requires(postun): /sbin/ldconfig" done btw, is that documented somewhere? - I took inspiration from another packages providing libraries and did not see that, so I expect there is a _lot_ of packages broken this way in the repos > * All these lines: > %dir %{_libdir}/qmmp > %dir %{_libdir}/qmmp/Input > %dir %{_libdir}/qmmp/Output > %{_libdir}/qmmp/Input/*.so > %{_libdir}/qmmp/Output/*.so > Can be written as just: > %{_libdir}/qmmp > Notice that you will still need: > %{_libdir}/libqmmp.so > Listed seperately done > * Drop the second %defattr line done > Possible improvements: > ---------------------- > * Use single quotes instead of double quotes around your sed scripts so that > you do not have to use \ infront of " inside the scripts. done, thanks new files: Spec URL: http://sn.bluehost.cz/tmp/no-mp3/qmmp.spec SRPM URL: http://sn.bluehost.cz/tmp/no-mp3/qmmp-0.1.4-4.fc8.src.rpm however, this is unusable because of the abovementioned problem with permissions :-(
(In reply to comment #12) > (In reply to comment #10) > > * buildroot does not match the buildroot mandated by the guidelines > > there is: > > BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root > > reading the guidelines > http://fedoraproject.org/wiki/Packaging/Guidelines#head-b4fdd45fa76cbf54c885ef0836361319ab962473 > > The recommended values for the BuildRoot tag are (in descending order of > preference) : > %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX) > %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) > %{_tmppath}/%{name}-%{version}-%{release}-root > > so it matches exactly the third line from the guidelines, what is the problem? > Ah, the third line in the guidelines is new (AFAIK) in the past only the first 2 were allowed, my bad. > > * BuildRequires line longer then 80 chars, please split this up in multiple > > lines each starting with BuildRequires: > > * desktop-file-install line longer then 80 chars, please split this up in > > multiple lines. > > done > > btw, where comes this requirement from? - reading the guidelines, I see that > only the Description cannot have lines longer than 80 chars > Readability in a terminal based editor, AFAIK everyone wraps all lines in the spec files at 80 chars. > > * use %defattr(-,root,root,-) > > done > > ... but this produces the following rpmlint error: > qmmp.x86_64: E: non-readable /usr/bin/qmmp 0601 > > I am confused where this comes from, since the compiled qmmp binary has 755 > and I see no change on install of that file > > unfortunately, the executable not being readable means that the included > resources are unusable, effectively disabling translations > Nasty, I'll try to reproduce this and see if I can come up with a fix. > > * put %doc directly under %defattr > > done > > btw, once again, where this requirement comes from? > Readability / consistency, this is how everyone (almost everyone) else does it. > > * drop %{?_smp_mflags}, if it fails on some systems it must be dropped, the > fact > > that it happens to work on others is not relevant, we don't want a > lottery, we > > want reproducable builds > > ok ... but I thought that it would be better to fix one b0rk3n machine than > crippling the build everywhere > Its not a broken machine %{?_smp_mflags} with badly written makefiles will break builds depending on timing, its a race condition problem, which depends on timing, which will change depeding on workload and speed of the machine. > > * add: "Requires(post): /sbin/ldconfig" > and "Requires(postun): /sbin/ldconfig" > > done > > btw, is that documented somewhere? - I took inspiration from another packages > providing libraries and did not see that, so I expect there is a _lot_ of > packages broken this way in the repos > Normally for libraries you use: %post -p /sbin/ldconfig And then /sbin/ldconfig gets used as script interpreter (interpreting an empty script), which will automatically add the Requires. > new files: > Spec URL: http://sn.bluehost.cz/tmp/no-mp3/qmmp.spec > SRPM URL: http://sn.bluehost.cz/tmp/no-mp3/qmmp-0.1.4-4.fc8.src.rpm > > however, this is unusable because of the abovementioned problem with > permissions :-( Good work sofar I'll see if I can recreate the permission problem and figure out whats going on.
> Its not a broken machine %{?_smp_mflags} with badly written makefiles will > break builds depending on timing, its a race condition problem, which > depends on timing, which will change depeding on workload and speed > of the machine. it seems to me very strange ... see comment #7 how does it come that there is problem only on one machine, and it is reproducible, if I get it right? badly written makefiles? - they are generated by cmake, I've never seen such problem, and I use parallel building heavily ... > I'll see if I can recreate the permission problem and figure out > whats going on. ok, looking forward to hear from you
Created attachment 265261 [details] PATCH fixing the problems with install permissions (In reply to comment #14) > > Its not a broken machine %{?_smp_mflags} with badly written makefiles will > > break builds depending on timing, its a race condition problem, which > > depends on timing, which will change depeding on workload and speed > > of the machine. > > it seems to me very strange ... see comment #7 > > how does it come that there is problem only on one machine, and it is > reproducible, if I get it right? > Well usually it is reproducable, on a certain machine when idle, and not on another while idle, adding an additional workload can further randomize things, then again you may be right and maybe something really is wrong with the machine in subject, but either way better safe then sorry, its not like using smpflags will save hours of buildtime. > badly written makefiles? - they are generated by cmake, I've never seen such > problem, and I use parallel building heavily ... > Well, even with cmake the people writing the CMakeLists can do stupid things, see below. > > I'll see if I can recreate the permission problem and figure out > > whats going on. > > ok, looking forward to hear from you UGH, the CMakeLists file in the src dir explictly sets install permissions and pretty bogus ones, patch fixing this attached.
Well, that pretty much wraps qmmp (add the patch and its approved), is there anything else you are planning / working on packaging, I would like to walk through atleast one other review with you before sponsoring you.
(In reply to comment #15) > Created an attachment (id=265261) [edit] > PATCH fixing the problems with install permissions thanks, included new files: Spec URL: http://sn.bluehost.cz/tmp/no-mp3/qmmp.spec SRPM URL: http://sn.bluehost.cz/tmp/no-mp3/qmmp-0.1.4-5.fc8.src.rpm > ... its not like using smpflags will save hours of buildtime. ok, I just though we should find the error ... > UGH, the CMakeLists file in the src dir explictly sets install permissions and > pretty bogus ones, patch fixing this attached. I see ... I must have been blind :-( (In reply to comment #16) > Well, that pretty much wraps qmmp (add the patch and its approved), is there > anything else you are planning / working on packaging, I would like to walk > through atleast one other review with you before sponsoring you. nothing right now ... I looked at KSquirrel, since I recently made Czech translation for it, but it seems that the 3rd party Fedora package is maintained pretty well
(In reply to comment #17) > (In reply to comment #16) > > Well, that pretty much wraps qmmp (add the patch and its approved), is there > > anything else you are planning / working on packaging, I would like to walk > > through atleast one other review with you before sponsoring you. > > nothing right now ... I looked at KSquirrel, since I recently made Czech > translation for it, but it seems that the 3rd party Fedora package is > maintained pretty well Well 3th party sounds like its not in Fedora proper, its much preferred to have things in Fedora proper. Then it will be build for ppc/ppc64 as well for example, but also then it will be avaible for install by users from add/remove programs without users having to know about all the 3th party repos. The fact that its currently well maintained in the thirdparty repo only makes your job easier. So moving this to Fedora proper would be good, unless its not admissable due to license / patents / etc ofcourse.
(In reply to comment #18) ... > So moving this to Fedora proper would be good, unless its not admissable > due to license / patents / etc ofcourse. I've exchanged a few emails with the repository maintainer, and it looks like he is going to contribute to Fedora directly ... so I have to find me another nice piece of software
Well, there is always: http://fedoraproject.org/wiki/PackageMaintainers/WishList#head-c49f426d7555f46259579267d3b520b975549487 http://fedoraproject.org/wiki/SIGs/Games#head-90aaf9bf465a8c09c75f2933edceb123d67066ba And a college of mine would like to add filezilla to the list.
wishlists are interesting, but I would prefer selecting something that I am personally interested in, not to make the boring duty of the packaging ... however, I see one thing I have dealt with in the past - UFO:AI (am I drunk? I see it twice ;-)) - so maybe I'll try in the near future (after I finish my duties for Tagua) - thanks
new version available ... Spec URL: http://www.hajnet.cz/soubory/qmmp/qmmp.spec SRPM URL: http://www.hajnet.cz/soubory/qmmp/qmmp-0.1.5-1.fc8.src.rpm packages for x86_64: http://www.hajnet.cz/soubory/qmmp/qmmp-0.1.5-1.fc8.x86_64.rpm http://www.hajnet.cz/soubory/qmmp/qmmp-debuginfo-0.1.5-1.fc8.x86_64.rpm
p.s. for mp3 support, take a look at http://bugzilla.livna.org/show_bug.cgi?id=1631 ... since that one does not get too much attention from Livna people, can somebody from here (Hans?) take a look at it so I can polish any mistakes, please?
Karel, as already said in another review of yours, I'm ready to sponsor you, so go ahead ask for cvsextras membership in the account system and I'll approve it.
New Package CVS Request ======================= Package Name: qmmp Short Description: Qt-based multimedia player Owners: kvolny Branches: F-7 F-8 InitialCC: jwrdegoede Cvsextras Commits: yes
Hans: Can you approve this package before cvs is completed?
Done, I kept setting the approved flag bet because of sponsorship, but thats taken care of now.
Thanks. cvs done.
qmmp-0.1.5-1.fc8 has been pushed to the Fedora 8 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 qmmp'
qmmp-0.1.5-1.fc7 has been pushed to the Fedora 7 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 qmmp'
Package Change Request ====================== Package Name: qmmp New Branches: EL-5 Owners: kvolny
cvs done.
Package Change Request ====================== Package Name: qmmp New Branches: epel7 Owners: kvolny InitialCC: nucleo see bug #1242361
Git done (by process-git-requests).