Fedora Merge Review: python-pyblock http://cvs.fedora.redhat.com/viewcvs/devel/python-pyblock/ Initial Owner: pjones
rpmlint output: python-pyblock.src: W: no-url-tag python-pyblock.x86_64: E: explicit-lib-dependency libbdevid python-pyblock.x86_64: E: explicit-lib-dependency libbdevid-python python-pyblock.x86_64: E: explicit-lib-dependency libselinux python-pyblock.x86_64: W: no-url-tag python-pyblock-debuginfo.x86_64: W: no-url-tag 3 packages and 0 specfiles checked; 3 errors, 3 warnings. [jzlehtol@politzer result]$ less build.log - The source url disclaimer must be added http://fedoraproject.org/wiki/Packaging/SourceURL#We_are_Upstream - Explicit lib dependencies are probably what you would expect for this package, so that's OK. - The %files sections is a bit too complicated, since the following does the same thing: %files %defattr(-,root,root) /%{python_sitelib}/block/ %{_docdir}/pyblock-%{version}/ - Moreover, I don't like that the docdir is not /usr/share/doc/python-pyblock-version. What I'd do is put mv ${RPM_BUILD_ROOT}/%{_docdir}/pyblock-%{version} doc after 'make install' and then list the documentation in %files as %doc doc/* - Change the %define's into %global's. *** MUST: The spec file for the package is legible and macros are used consistently. ~OK MUST: The package must be named according to the Package Naming Guidelines. OK MUST: The spec file name must match the base package %{name}. OK MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines. OK MUST: The License field in the package spec file must match the actual license. OK MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK MUST: The package MUST successfully compile and build into binary rpms. OK MUST: The spec file MUST handle locales properly. OK MUST: Optflags are used and time stamps preserved. NEEDSFIX - %{optflags} are not respected!! - Add the definition %{!?pyver: %global pyver %(%{__python} -c "import sys ; print sys.version[:3]")} and build with make CFLAGS="%{optflags} -I/usr/include/python%{pyver} -fPIC" %{?_smp_mflags} which also enables SMP make. MUST: Packages containing shared library files must call ldconfig. OK MUST: A package must own all directories that it creates or require the package that owns the directory. OK MUST: Files only listed once in %files listings. OK MUST: Permissions on files must be set properly. OK MUST: Clean section exists. OK MUST: Large documentation files must go in a -doc subpackage. OK MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK MUST: Header files must be in a -devel package. OK MUST: Static libraries must be in a -static package. OK MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. OK MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. OK MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. OK MUST: Packages does not contain any .la libtool archives. OK MUST: Desktop files are installed properly. OK MUST: No file conflicts with other packages and no general names. OK MUST: Buildroot cleaned before install. OK SHOULD: %{?dist} tag is used in release. OK SHOULD: The package builds in mock. OK SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. NEEDSFIX? - GPLv2 is included, GPLv3 is not.
ping pjones
ping again?
Hi, Thanks for being persistent! I'm the one doing most pyblock work now a days and I was not aware of the merge review until your ping. A few of the issues from your original review had already been addressed in newer versions, I've fixed the remaining issues in python-pyblock-0.49-1.fc14 Note I've not changed the dir where the documentation gets installed as I see no reason for that. Thanks and Regards, Hans
(In reply to comment #1) > - The source url disclaimer must be added > http://fedoraproject.org/wiki/Packaging/SourceURL#We_are_Upstream This has actually changed since my review. Now we don't allow exceptions of any kind, so you must create a project homepage at e.g. Fedora Hosted https://fedorahosted.org/web/. ** The BuildRoot tag is obsolete. Please modernize to %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX) or remove it altogether, since it's no longer needed on newer Fedoras. ** rpmlint now stands as: python-pyblock.src:12: W: macro-in-comment %{version} python-pyblock.src:12: W: macro-in-comment %{release} python-pyblock.src: W: invalid-url Source0: pyblock-0.49.tar.bz2 python-pyblock.x86_64: W: private-shared-object-provides /usr/lib64/python2.6/site-packages/block/dmraidmodule.so dmraidmodule.so.0.49()(64bit) python-pyblock.x86_64: W: private-shared-object-provides /usr/lib64/python2.6/site-packages/block/dmmodule.so dmmodule.so.0.49()(64bit) 3 packages and 0 specfiles checked; 0 errors, 5 warnings. You might want to prepend the macros in comments with an additional % to prevent expansion. ** The build system is IMHO rather unusual for a python module. This probably explains the lack of an egg that is normally present in Python packages. ** Note that the package could also be named simply 'pyblock', as this is allowed per the Naming Guidelines. ** (In reply to comment #5) > Note I've not changed the dir where the documentation > gets installed as I see no reason for that. Now the location of the documentation is rather unorthodox, since the name of the docdir does not equal that of the package itself. ** Otherwise, everything seems to be in condition. Please address the issues listed above.
<The below comment is purely mine and in no way is a comment which I make on behalf of or is endorsed by my employer> First of all let me start with a generic statement about the contents of your last comment. Once I became aware of this merge review I put it on my to do list as I'm a long time Fedora contributor (for many years before I joined Red Hat) and a Fedora Packaging Committee (the Committee which creates the guidelines) member. As such I strongly believe in the review process and that the merge reviews are important. I must say however that I'm very disappointed about your response to my attempt to get python-pyblock into shape. Almost all of your comments are about things which the guidelines either do not specify at all, or clearly allow. Getting such a pedantic response to cleaning up a package, does not motivate me to do further work on this or other merge reviews. I also think that if this is the way how you are handling other merge reviews, you are likely creating unnecessary friction / resistance there as well. (In reply to comment #6) > (In reply to comment #1) > > - The source url disclaimer must be added > > http://fedoraproject.org/wiki/Packaging/SourceURL#We_are_Upstream > > This has actually changed since my review. Now we don't allow exceptions of any > kind, so you must create a project homepage at e.g. Fedora Hosted > https://fedorahosted.org/web/. > Yes we are upstream, but this is a very Fedora specific bit of code which is only used by anaconda, which is only used in Fedora. Thus there is no value in creating a website and doing tarbal releases as no other project / person is using python-pyblock (*) or has shown any interest in it. Thus creating a website and a download area would only serve to generate work, quickly get very stale, and serve no useful purpose. AFAIK there is no rule that a project homepage MUST be created, and a thorough search through all guidelines has not found me no such a rule either, quoting from: http://fedoraproject.org/wiki/Packaging/SourceURL "There are several cases where upstream is not providing the source to you in an upstream tarball. In these cases you must document how to generate the tarball used in the rpm either through a spec file comment or a script included as a separate SourceX:." Which is exactly which this package is doing. IOW this package is fully following the guidelines as written wrt the source url. *) Unlike for example pyparted the python parted bindings which are also written and maintained by the anaconda team, but which are of interest to others and the anaconda team thus has created a website with a download area for. > ** > > The BuildRoot tag is obsolete. Please modernize to > %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX) > or remove it altogether, since it's no longer needed on newer Fedoras. > Yes this is no longer the recommend way of doing things, but it is still allowed, see: http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag Which links to: http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#BuildRoot_tag Needless pedantic comment 1. > ** > > rpmlint now stands as: > > python-pyblock.src:12: W: macro-in-comment %{version} > python-pyblock.src:12: W: macro-in-comment %{release} > python-pyblock.src: W: invalid-url Source0: pyblock-0.49.tar.bz2 > python-pyblock.x86_64: W: private-shared-object-provides > /usr/lib64/python2.6/site-packages/block/dmraidmodule.so > dmraidmodule.so.0.49()(64bit) > python-pyblock.x86_64: W: private-shared-object-provides > /usr/lib64/python2.6/site-packages/block/dmmodule.so dmmodule.so.0.49()(64bit) > 3 packages and 0 specfiles checked; 0 errors, 5 warnings. > > You might want to prepend the macros in comments with an additional % to > prevent expansion. These comments are never stored in a generated rpm in anyway and thus are never used in an expanded fashion, so escaping the macros in the comment serves no purpose other then making the comment harder to read. Also in all my 200+ package submissions I've never had this remark before -> Needless pedantic comment 2. > ** > > The build system is IMHO rather unusual for a python module. This probably > explains the lack of an egg that is normally present in Python packages. The guidelines deliberately say absolutely nothing about "allowed" build systems -> Needless pedantic comment 3. > ** > > Note that the package could also be named simply 'pyblock', as this is allowed > per the Naming Guidelines. The current name is allowed and renaming existing packages is a pain, this is not just pedantic it is plain bad advice. Needless pedantic comment 4. > > ** > > (In reply to comment #5) > > Note I've not changed the dir where the documentation > > gets installed as I see no reason for that. > > Now the location of the documentation is rather unorthodox, since the name of > the docdir does not equal that of the package itself. The guidelines do not specify where a package is allowed or not allowed to install documentation (as long as it follows the FHS) -> Needless pedantic comment 5. > > ** > > Otherwise, everything seems to be in condition. Please address the issues > listed above. As explained above I see no need to address any of these non "issues", please approve this merge review. Regards, Hans
I'm sorry if my pedantic style has in any way offended you. The main thing holding back the review was the source url. The "we are upstream" exception was discussed in FESCo and was removed. http://meetbot.fedoraproject.org/fedora-meeting/2009-08-07/fedora-meeting.2009-08-07-17.00.log.html#l-87 IIRC the reasoning was that "we are upstream" is not a valid excuse, as in the spirit of free software the code should be made easily availables to other people as well. And, if I'm not mistaken, fedorahosted.org was set up after discussion to provide a means for the easy distribution of the sources. Every one of the former "we are upstream" packages (including Anaconda, http://git.fedoraproject.org/git/anaconda.git that you mentioned in comment #7) that I have looked at in the recent past have become available on fedorahosted.org. The project list is rather lengthy already. ** Had a maintainer had a look at this review when I originally made it, more than a year ago, the source url discussion would not have taken place as the guidelines changed later on. It's not in my interests to bug people unwarrantedly, so this package is APPROVED although I still think the source should be made available at fedorahosted.org.
OK, I see a link has been added to fedorahosted.org. Thanks! Closing.