Bug 504430

(In reply to comment #0)
> Here is rpmlint:
> rpmlint healpy-0.9.6-1.fc10.i386.rpm
> healpy.i386: E: explicit-lib-dependency python-matplotlib
> 
> I don't know what to do about this error since the package requires
> python-matplotlib for plotting, a major purpose of the program.  

That's completely fine. rpmlint is most likely confused by "lib" substring, wrongly concluding that it is a package that contains an ELF shared library that  should be depended on automatically. It's perfectly OK to depend on a python library explicitely.

- The BRs are incorrect.
 BuildRequires:	python
should be
 BuildRequires: python-devel
and
 BuildRequires:	gcc-c++
can be safely dropped (it's in the default buildroot).

?? The package uses BOTH python_sitearch AND python_sitelib?? (Are you building on x86, then these will point to the same place?)

- Why on Earth do you
 mkdir -p %{buildroot}%{_datadir}/%{name}/
 mv %{buildroot}%{python_sitelib}/%{name}/data %{buildroot}%{_datadir}/%{name}/data
as this will probably break functionality..?

- As the Healpix C++ package is included in the distribution, you need to find a way to remove it and use healpix-c++-devel instead.

(In reply to comment #2)
> - The BRs are incorrect.
>  BuildRequires: python
> should be
>  BuildRequires: python-devel

Sorry if I made a mistake here, but the packaging guidelines for Python state to use python, not python-devel: https://fedoraproject.org/wiki/Packaging/Python


> ?? The package uses BOTH python_sitearch AND python_sitelib?? (Are you building
> on x86, then these will point to the same place?)

Again, sorry if this was a mistake, this package has C/C++ libraries and the above guidelines seem to suggest you use python_sitelib for normal Python modules and python_sitearch for "libraries (like those written in C)".

I've never dealt with a Python program with these kind of libraries before so I got confused.

> 
> - Why on Earth do you
>  mkdir -p %{buildroot}%{_datadir}/%{name}/
>  mv %{buildroot}%{python_sitelib}/%{name}/data
> %{buildroot}%{_datadir}/%{name}/data
> as this will probably break functionality..?
> 

Because they are just example .fits files.  However, I will put them back. 

> - As the Healpix C++ package is included in the distribution, you need to find
> a way to remove it and use healpix-c++-devel instead.  

Okay, I will do this and the other things.

Okay, first of all I forgot to thank you for the review.  

I've patched the source so it doesn't build the Healpix-c++ package and also so that it links against the healpix-c++ and cfitsio libraries already in Fedora.

I also think I have corrected the other mistakes you mentioned, unless I misunderstood.  Thanks again.  The new files are here with no change to rpmlint.

Spec URL: http://jsmidt.fedorapeople.org/healpy.spec
SRPM URL: http://jsmidt.fedorapeople.org/healpy-0.9.6-1.fc10.src.rpm

- You have
 BuildRequires:	healpix-c++-devel
 BuildRequires:	cfitsio-devel
so
 BuildRequires:	healpix-c++
 BuildRequires:	cfitsio
is redundant (these are pulled in by the -devel packages).

- You are not building in %build. Use
 python setup.py build
in %build and
 python setup.py install -O1 --skip-build --root %{buildroot} 
in %install.

- Without looking at its contents, INSTALL shouldn't probably be in %doc (if its only contents is instructions for installation from source, then it shouldn't be included).

- You must in any case own the directory
 %{python_sitearch}/%{name}/
so you can drop the three last lines from the %files section.

- Add comment about the patch. Also, you could remove the internal healpix and cfitsio libraries from the extracted tarball in the setup phase so that one can be sure that they are not used instead of the Fedora packages.

(In reply to comment #5)
> - You have
>  BuildRequires: healpix-c++-devel
>  BuildRequires: cfitsio-devel
> so
>  BuildRequires: healpix-c++
>  BuildRequires: cfitsio
> is redundant (these are pulled in by the -devel packages).
> 

I meant:
 
Requires: healpix-c++
Requires: cfitsio

   so I believe this is now fixed.


> - You are not building in %build. Use
>  python setup.py build
> in %build and
>  python setup.py install -O1 --skip-build --root %{buildroot} 
> in %install.

Done

> 
> - Without looking at its contents, INSTALL shouldn't probably be in %doc (if
> its only contents is instructions for installation from source, then it
> shouldn't be included).
>

Done
 
> - You must in any case own the directory
>  %{python_sitearch}/%{name}/
> so you can drop the three last lines from the %files section.
> 

Done.  I believe I did what you meant.

> - Add comment about the patch. 

Done.

> Also, you could remove the internal healpix and
> cfitsio libraries from the extracted tarball in the setup phase so that one can
> be sure that they are not used instead of the Fedora packages.  

Done.  I have removed the entire directory containing the healpix and cfitsio libraries in the setup phase.

The new files are again at:
Spec URL: http://jsmidt.fedorapeople.org/healpy.spec
SRPM URL: http://jsmidt.fedorapeople.org/healpy-0.9.6-1.fc10.src.rpm

(In reply to comment #6)
> (In reply to comment #5)
> >  BuildRequires: healpix-c++
> >  BuildRequires: cfitsio
> > is redundant (these are pulled in by the -devel packages).
> 
> I meant:
> 
> Requires: healpix-c++
> Requires: cfitsio
> 
>    so I believe this is now fixed.

Nope, that is in collision with
http://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires

Also, you really should increment the release tag every time you make a new revision. Not incrementing it makes it harder for me to keep track of what has been done.

Also, the package does not build in mock. You need at least BR: numpy, maybe you need also the other python packages as BR.

Thanks for all your patience with this review.  I have fixed the issues you mentioned and now the package builds in mock.


The new files are again at:
Spec URL: http://jsmidt.fedorapeople.org/healpy.spec
SRPM URL: http://jsmidt.fedorapeople.org/healpy-0.9.6-2.fc10.src.rpm

Well, this should be Release 4, since:

-1 was initial release
-2 was build against healpix-c++ and cfitsio instead of the bundled ones
-3 was removing bundled healpix and cfitsio altogether and changing duplicated BR:s into R:s
-4 dropping the extra R:s

**

- You should probably
 %build
 export CFLAGS="%{optflags} -fopenmp"
to enable threading support.

- g++ flags are not used. This needs to be fixed.

- The package *does not contain any license statement at all*. It cannot go in until you get upstream to put the license in the tarball, the order of preference is
1. source code headers stating the file(s) are under GPLv2+
2. COPYING stating healpy is under GPLv2+
3. plain GPLv(N) COPYING file, which means that code is under GPL+.

> - The package *does not contain any license statement at all*. It cannot go in
> until you get upstream to put the license in the tarball, the order of
> preference is

I have contacted upstream.  I will let you know when the License has been properly included.

The upstream Author made a new version with GPLv2 headers in the source files.

I have added the gcc and g++ build flags.  I have taken upstream's minimum package dependency versions recommendations and added them to the spec file.

This package builds successfully  on all platforms for Fedora 10 and 11 using Koji:
https://koji.fedoraproject.org/koji/taskinfo?taskID=1409599
https://koji.fedoraproject.org/koji/taskinfo?taskID=1409604

There are is no rpmlint warnings or errors on my Fedora 11 system.

The new files are again at:
Spec URL: http://jsmidt.fedorapeople.org/healpy.spec
SRPM URL: http://jsmidt.fedorapeople.org/healpy-0.9.6.1-1.fc11.src.rpm

rpmlint output:
healpy.x86_64: E: non-standard-executable-perm /usr/lib64/python2.6/site-packages/healpy/_healpy_sph_transform_lib.so 0775
healpy.x86_64: E: non-standard-executable-perm /usr/lib64/python2.6/site-packages/healpy/_healpy_pixel_lib.so 0775
healpy.x86_64: E: non-standard-executable-perm /usr/lib64/python2.6/site-packages/healpy/_healpy_fitsio_lib.so 0775
3 packages and 0 specfiles checked; 3 errors, 0 warnings.

- Fix the permissions, they should be 755.


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
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. NEEDSWORK
- This is a python module, so the correct name is python-healpy.

MUST: The spec file name must match the base package %{name}. OK
- Remember to change this as well.

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
- GPLv2+ license headers are now present and correctly picked up by licensecheck.pl
- The test file test/test_fit_dipole.py is still missing a license. This is not a problem per se, since the license of the package is already clear from the other files.

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. N/A
MUST: Optflags are used and time stamps preserved. OK
MUST: Packages containing shared library files must call ldconfig. N/A
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: Debuginfo package is complete. 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. N/A

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. NEEDSWORK
- Add ChangeLog and test/test_fit_dipole.py to %doc.

MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. N/A
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: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. NEEDSWORK
- License file is still missing.

SHOULD: The package builds in mock. OK

(In reply to comment #13)
> rpmlint output:
> healpy.x86_64: E: non-standard-executable-perm
> /usr/lib64/python2.6/site-packages/healpy/_healpy_sph_transform_lib.so 0775
> healpy.x86_64: E: non-standard-executable-perm
> /usr/lib64/python2.6/site-packages/healpy/_healpy_pixel_lib.so 0775
> healpy.x86_64: E: non-standard-executable-perm
> /usr/lib64/python2.6/site-packages/healpy/_healpy_fitsio_lib.so 0775
> 3 packages and 0 specfiles checked; 3 errors, 0 warnings.
> 
> - Fix the permissions, they should be 755.
> 

This is so weird.  On my system they are 755 so rpmlint isn't complaining.  However, I have forced them to by 755 so Done.



> MUST: The package must be named according to the Package Naming Guidelines.
> NEEDSWORK
> - This is a python module, so the correct name is python-healpy.

Sorry, I didn't know this is a hard rule since the python modules it depends on, numpy, scipy and pyfits don't follow it. 

However fixed: Done.



> SHOULD: If the package does not include license text(s) as separate files from
> upstream, the packager should query upstream to include it. NEEDSWORK
> - License file is still missing.

It turns out the COPYING file is in the directory containinh Healpix-c++ which I was deleting.  It is now copied to %%doc.  Done

The new files are again at:
Spec URL: http://jsmidt.fedorapeople.org/healpy.spec
SRPM URL: http://jsmidt.fedorapeople.org/python-healpy-0.9.6.1-2.fc11.src.rpm

Sorry, here is the spec file: http://jsmidt.fedorapeople.org/python-healpy.spec

(In reply to comment #14)
> > MUST: The package must be named according to the Package Naming Guidelines.
> > NEEDSWORK
> > - This is a python module, so the correct name is python-healpy.
> 
> Sorry, I didn't know this is a hard rule since the python modules it depends
> on, numpy, scipy and pyfits don't follow it. 
> 
> However fixed: Done.

*whoops*

I though the guideline was that the name must begin with python- if it doesn't start with py, but actually it is "If the upstream source has "py" (or "Py") in its name, you can use that name for the package. So, for example, pygtk is acceptable."

http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Addon_Packages_.28python_modules.29

So the name should be after all just healpy. Sorry for the mixup :)

> 
> So the name should be after all just healpy. Sorry for the mixup :)  


Okay, name changed back.

The new files are again at:
Spec URL: http://jsmidt.fedorapeople.org/healpy.spec
SRPM URL: http://jsmidt.fedorapeople.org/healpy-0.9.6.1-3.fc11.src.rpm

- The COPYING file actually belongs to healpix c++, but it doesn't matter since both are gpl.

Everything seems to be fixed now, so the package is


APPROVED

New Package CVS Request
=======================
Package Name: healpy
Short Description: A python wrapper of the healpix library
Owners: jsmidt
Branches: F-10 F-11 EL-5
InitialCC:

CVS done.

healpy-0.9.6.1-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/healpy-0.9.6.1-3.fc11

healpy-0.9.6.1-3.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/healpy-0.9.6.1-3.fc10

healpy-0.9.6.1-3.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

healpy-0.9.6.1-3.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.