Bug 477542
Summary: | Review Request: mpdscribble - A mpd client which submits information about tracks being played to Last.fm | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Jaroslaw Gorny <jaroslaw.gorny> |
Component: | Package Review | Assignee: | Peter Lemenkov <lemenkov> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | low | ||
Version: | rawhide | CC: | atorkhov, avm-xandry, fedora-package-review, lemenkov, notting |
Target Milestone: | --- | Keywords: | Reopened |
Target Release: | --- | Flags: | lemenkov:
fedora-review+
kevin: fedora-cvs+ |
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2009-11-10 21:10:10 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: |
Description
Jaroslaw Gorny
2008-12-21 19:40:20 UTC
I'll review it. REVIEW: - You packaged relatively old version - 0.13, although they released v 0.16. - rpmlint is not silent: [petro@host-12-116 SPECS]$ rpmlint ~/Desktop/mpdscribble-0.13-1.fc10.* mpdscribble.i386: W: incoherent-version-in-changelog 0.13.1 ['0.13-1.fc10', '0.13-1'] 2 packages and 0 specfiles checked; 0 errors, 1 warnings. [petro@host-12-116 SPECS] Please use %{version}-%{release} w/o .fc10 suffix in %changelog. For example, if %{version} is 0.13 and release is 1%{?dist} you should use 0.13-1 as version tag in %changelog. + The package is named according to the Package Naming Guidelines . + The spec file name matches the base package %{name}, in the format %{name}.spec unless your package has an exemption. +/- The package meets the Packaging Guidelines. After brief chacking, I found only one *possible* issue - this package ships (and was built against) libmpdclient library ( http://mpd.wikia.com/wiki/ClientLib:libmpdclient ). The Guidelines has statement that we must avoid using such libraries if they already exists in system. This library, libmpdclient still not included in main Fedora repository (therefore it doesn't a problem), however it might be included in the future (and will be a problem). You should watch this issue. + The package is licensed with a Fedora approved license and meets the Licensing Guidelines. + The License field in the package spec file matches the actual license. + The file, containing the text of the license(s), is included in %doc. + The spec file is written in American English. + The spec file for the package is legible. + The sources used to build the package matches the upstream source, as provided in the spec URL. [petro@host-12-116 SOURCES]$ md5sum mpdscribble-0.13.tar.gz* 884717131356ad2f62918d458258b443 mpdscribble-0.13.tar.gz 884717131356ad2f62918d458258b443 mpdscribble-0.13.tar.gz.from_srpm [petro@host-12-116 SOURCES]$ + The package MUST successfully compile and build into binary rpms on at least one primary architecture. http://koji.fedoraproject.org/koji/taskinfo?taskID=1098823 + All build dependencies are listed in BuildRequires. + No need to handle locales. + No shared library files. + The package is not designed to be relocatable. + The package owns all directories that it creates. + The package does not contain any duplicate files in the %files listing. + Permissions on files are set properly. + The package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT. + The package consistently uses macros. + The package contains code, or permissable content. + No large documentation files. + Everything, the package includes as %doc, does not affect the runtime of the application. + No header files + No static libraries + No pkgconfig(.pc) files + No library files with a suffix + No devel packages + No .la libtool archives + Not a GUI application + The package does not own files or directories already owned by other packages. + At the beginning of %install, the package runs rm -rf $RPM_BUILD_ROOT. + All filenames in the package are valid UTF-8. So here are my suggestions: * update srpm to latest 0.16 (or explain why you won't do it). * fix %changelog versioning entry * consider packaging libmpdclient also (not a blocker) * I've updated to latest version (0.16) - 0.13 was latest when I packaged it. * %changelog fixed So rpmlint is quiet - at least here. Results are here: SRPM URL: http://strony.aster.pl/muranow/fedora/mpdscribble-0.16-1.fc10.src.rpm SPEC URL: http://strony.aster.pl/muranow/fedora/mpdscribble.spec OK, I'll consider to package libmpdclient too ;) OK, some additional steps (because of updated srpm): + The sources used to build the package matches the upstream source, as provided in the spec URL. [petro@host-12-116 SOURCES]$ md5sum mpdscribble-0.16.tar.bz2* deeeb403ebd50b7abacd1eeabc96b320 mpdscribble-0.16.tar.bz2 deeeb403ebd50b7abacd1eeabc96b320 mpdscribble-0.16.tar.bz2.from_srpm [petro@host-12-116 SOURCES]$ + The package successfully compiles and builds into binary rpms on all primary architectures. http://koji.fedoraproject.org/koji/taskinfo?taskID=1103246 This package is ============== ============== == APPROVED == ============== ============== Your next steps are: * Wait for someone, who sponsors you (I cannot sponsor you). * Request cvs branches (see https://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure ) * Proceed from step 8 at this page - https://fedoraproject.org/wiki/PackageMaintainers/NewPackageProcess Ping! Any news? Seems like nobody wants to sponsor me (this package). Without that I think making this 2 next steps doesn't make any sense. Am I right? I was cleaning up the sponsorship queue and found this package. Jaroslaw, are you still interested in submitting this package? Peter technically has the ability to sponsor you; I don't know if he gained that within the last six months, but at this point it makes no difference. Peter, Jaroslaw is still in the sponsorship queue (FAS ID jaroslav); did you want to get this moving? What else would be required to force some progress here? Hello all. The new version of mpdscribble: SPEC URL: http://narod.ru/disk/14320486000/mpdscribble.spec.html SRPM URL: http://narod.ru/disk/14320467000/mpdscribble-0.18.1-1.fc11.src.rpm.html OK, since xandry decided to continue Jaroslaw's work, I'm closing this now. Jaroslaw, thanks for your efforts in packaging this. *** This bug has been marked as a duplicate of bug 529917 *** I feel somehow dissappointed. I wanted to maintain this package. But if Fedora doesn't need new people - very well. I thought, that you're gone, Jaroslaw. I'm sorry for inconvinience, and, actually, I'm glad to see you back. Returning to topic - now, I can sponsor you. Removing FE_NEEDSPONSOR - I just sponsored Jaroslaw. Jaroslaw, please, update package up to the latst version. Also, please, consider adding init-script, since mpdscribble can act as a daemon. I created one, but didn't test it yet (however it looks sane): http://peter.fedorapeople.org/stuff/mpdscribble.init Peter, I need your help, I've packaged new version (0.18.1) but there's one problem. They (upstream) set permissions for /etc/mpdscribble.conf to be "0600". We have usually "0644" and this is what rpmlint expects. So I have: <code> [makerpm@moonstone i686]$ rpmlint mpdscribble-0.18.1-1.fc12.i686.rpm mpdscribble.i686: E: non-readable /etc/mpdscribble.conf 0600 1 packages and 0 specfiles checked; 1 errors, 0 warnings. </code> I have two possibilities - make a patch, or ignore rpmlint. Which one to chose? There is no need to expose to this file of the right. There by default all becomes as it is necessary. (In reply to comment #15) > There is no need to expose to this file of the right. There by default all > becomes as it is necessary. @xandry: I changed this using %attr(). Now rpmlint does not complain. I've installed this package and file permissions are correct. @Peter: Init script is attached, looks sane, appropriate %post and %preun sections are added. New version of package is available: RPM: http://strony.aster.pl/muranow/fedora/mpdscribble-0.18.1-1.fc12.i686.rpm http://strony.aster.pl/muranow/fedora/mpdscribble-debuginfo-0.18.1-1.fc12.i686.rpm SRPM: http://strony.aster.pl/muranow/fedora/mpdscribble-0.18.1-1.fc12.src.rpm SPEC: http://strony.aster.pl/muranow/fedora/mpdscribble.spec Peter, can I set 'fedora-cvs' flag to '?' now? (In reply to comment #16) > @xandry: I changed this using %attr(). Now rpmlint does not complain. I've > installed this package and file permissions are correct. First, I'm afraid, that you should package it as is, w/o changing permissions. There is one possible user case - running mpdscribble as a server on an iron with multiple user accounts, and server's admin would like to hide last.fs account settings from others. Second, I don't see a reason in using killall in %preun section. I think that killing all instances of the app, started by other users, is somewhat improper. Also, I advice you a little simplification. Instead of #init scripts install -d $RPM_BUILD_ROOT/%{_sysconfdir}/rc.d/init.d install -m 0755 %{SOURCE1} $RPM_BUILD_ROOT/%{_sysconfdir}/rc.d/init.d/%{name} you may write install -D -m 0755 %{SOURCE1} $RPM_BUILD_ROOT/%{_sysconfdir}/rc.d/init.d/%{name} or even install -D -m 0755 %{SOURCE1} $RPM_BUILD_ROOT/%{_initddir}/%{name} Please, note, that %{_initddir} is unavailable on RHEL/CentOS, so if you plan to push the application into EPEL also, then you should consider using %{_initrddir} instead. Other things looks sane. Please, comment my notes, and we'll continue our journey. Peter, find my answers below: (In reply to comment #17) > (In reply to comment #16) > > > @xandry: I changed this using %attr(). Now rpmlint does not complain. I've > > installed this package and file permissions are correct. > > First, I'm afraid, that you should package it as is, w/o changing permissions. > There is one possible user case - running mpdscribble as a server on an iron > with multiple user accounts, and server's admin would like to hide last.fs > account settings from others. > You're absolutely right. But on the other hand rpmlint is mandatory step in package approval. So how are we going to deal with it? > Second, I don't see a reason in using killall in %preun section. I think that > killing all instances of the app, started by other users, is somewhat improper. > Yes, your're right. I'll change this. I took this %preun scriptlet from wpa_supplicant package - it makes sense there ;) > Also, I advice you a little simplification. Instead of > > #init scripts > install -d $RPM_BUILD_ROOT/%{_sysconfdir}/rc.d/init.d > install -m 0755 %{SOURCE1} $RPM_BUILD_ROOT/%{_sysconfdir}/rc.d/init.d/%{name} > > you may write > > install -D -m 0755 %{SOURCE1} > $RPM_BUILD_ROOT/%{_sysconfdir}/rc.d/init.d/%{name} OK. I'll merge those two lines into one. > Please, note, that %{_initddir} is unavailable on RHEL/CentOS, so if you plan > to push the application into EPEL also, then you should consider using > %{_initrddir} instead. > Hmm. This is the reason I used %{_sysconfdir}/rc.d/init.d Even if I'll not push the app into EPEL I think it's better to use generic macros. And personally I hate %{_initrddir} because the macro is confusing - it suggests it has something to do with initrd :/ So I'd rather keep %{_sysconfdir}/rc.d/init.d but if you insist - I'll change it to %{_initddir}. I'll prepare new package in the evening (don't have Fedora here). Thanks once again for your help, J. (In reply to comment #18) > > > @xandry: I changed this using %attr(). Now rpmlint does not complain. I've > > > installed this package and file permissions are correct. > > First, I'm afraid, that you should package it as is, w/o changing permissions. > > There is one possible user case - running mpdscribble as a server on an iron > > with multiple user accounts, and server's admin would like to hide last.fs > > account settings from others. > You're absolutely right. But on the other hand rpmlint is mandatory step in > package approval. So how are we going to deal with it? In this particular case we may ignore this rpmlint message. > > Please, note, that %{_initddir} is unavailable on RHEL/CentOS, so if you plan > > to push the application into EPEL also, then you should consider using > > %{_initrddir} instead. > Hmm. This is the reason I used %{_sysconfdir}/rc.d/init.d Then you should (for consistency) change %files section. Instead of %{_initddir}/%{name} you should add %{_sysconfdir}/rc.d/init.d/%{name} > So I'd rather keep %{_sysconfdir}/rc.d/init.d but if you insist - I'll change > it to %{_initddir}. No, I'm not insisting here - feel free to decide which one to choose by yourself :) *** Bug 529917 has been marked as a duplicate of this bug. *** OK, I've corrected spec file and rebuilt package. I haven't changed release number, I'd like to keep rel=1 as initial build in official system. New version of package is available under the same address: RPM: http://strony.aster.pl/muranow/fedora/mpdscribble-0.18.1-1.fc12.i686.rpm http://strony.aster.pl/muranow/fedora/mpdscribble-debuginfo-0.18.1-1.fc12.i686.rpm SRPM: http://strony.aster.pl/muranow/fedora/mpdscribble-0.18.1-1.fc12.src.rpm SPEC: http://strony.aster.pl/muranow/fedora/mpdscribble.spec One small issue still remains (fortunately, the last) - a path for the pid-file is set to /var/run/$prog/$prog.pid, but you didn't list /var/run/${name} in the %files section. Creating extra directory in /var/run, for storing pid-file, is an obvious solution for permissions issue, when main application runs from different user. Since mpdscribble runs from root, it doesn't make any sense. So, I advice you to change variable $pidfile in the init-script to /var/run/$prog.pid OK, done. Fresh packages are available under the same links as before. Ok, looks fine. Please, proceed with fedora-cvs flag. Please enter a cvs request template here so we know what branches you want and such. https://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure Set the fedora-cvs flag again when you have done so. New Package CVS Request ======================= Package Name: mpdscribble Short Description: A mpd client which submits information about tracks being played to last.fm Owners: jaroslav Branches: F-11 InitialCC: peter cvs done with F-12 branch added. Package: mpdscribble-0.18.1-1.fc13 Tag: dist-f13 Status: complete Built by: jaroslav ID: 140666 Started: Tue, 10 Nov 2009 21:02:33 UTC Finished: Tue, 10 Nov 2009 21:05:32 UTC mpdscribble-0.18.1-1.fc12 has been submitted as an update for Fedora 12. http://admin.fedoraproject.org/updates/mpdscribble-0.18.1-1.fc12 mpdscribble-0.18.1-1.fc11 has been submitted as an update for Fedora 11. http://admin.fedoraproject.org/updates/mpdscribble-0.18.1-1.fc11 mpdscribble-0.18.1-1.fc11 has been pushed to the Fedora 11 stable repository. If problems still persist, please make note of it in this bug report. mpdscribble-0.18.1-1.fc12 has been pushed to the Fedora 12 stable repository. If problems still persist, please make note of it in this bug report. |