Bug 172042 - Review Request: cfitsio -- library to read and write FITS files.
Review Request: cfitsio -- library to read and write FITS files.
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ed Hill
David Lawrence
http://heasarc.gsfc.nasa.gov/docs/sof...
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-10-29 16:08 EDT by Matthew Truch
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-11-13 15:13:06 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
wtogami: fedora‑cvs+


Attachments (Terms of Use)
changes to the specfile (1.62 KB, patch)
2005-11-12 15:22 EST, Ed Hill
no flags Details | Diff

  None (edit)
Description Matthew Truch 2005-10-29 16:08:29 EDT
Spec Name or Url: http://matt.truch.net/fedora/cfitsio.spec
SRPM Name or Url: http://matt.truch.net/fedora/cfitsio-3.004-0.1.b.src.rpm
Description: 
CFITSIO is a library of C subroutines for reading and writing data files in 
FITS (Flexible Image Transport System) data format. CFITSIO simplifies the 
task of writing software that deals with FITS files by providing an easy to 
use set of high-level routines that insulate the programmer from the 
internal complexities of the FITS file format. At the same time, CFITSIO 
provides many advanced features that have made it the most widely used FITS 
file programming interface in the astronomical community. 

This is my first request for a review, so I'm also in need of a sponsor.
Comment 1 Matthew Truch 2005-11-05 15:52:47 EST
I've made a few fixes, available now as:

Spec Name or Url: http://matt.truch.net/fedora/cfitsio.spec
SRPM Name or Url: http://matt.truch.net/fedora/cfitsio-3.004-0.2.b.src.rpm

* Sun Oct 30 2005 Matthew Truch <matt at truch.net> - 3.004-0.2.b
- Include gcc-gfortran build requirment and make sure it gets used.
- Use macros instead of hard coded paths.
- Include home page in description

[Someone please sponsor me :-) ]
Comment 2 Ed Hill 2005-11-05 19:42:34 EST
Hi Matthew, I started to do a review and immediately noticed that the source 
tarball included in the above SRPM does not match the upstream souce.  The 
difference seems to be that your tarball has its contents packed within the 
directory "cfitsio-3.004" while upstream uses "cfitsio".  You can use the
upstream tarball unchanged with:
  %setup -q -n cfitsio
instead of
  %setup -q

Not to give you a hard time, but I consider a mismatch between the upstream
sources and the SRPM-distributed sources to be something that happens only 
in really unusual circumstances.  So, could you please update your SRPM so 
that it uses a tarball identical to the upstream?
Comment 3 Matthew Truch 2005-11-05 23:44:34 EST
No worries about the "hard time."  I'm new, and I agree, having the tarball
match is important.  I was unable to find the -n flag.

Updated spec and SRPM:

http://matt.truch.net/fedora/cfitsio.spec
http://matt.truch.net/fedora/cfitsio-3.004-0.3.b.src.rpm
Comment 4 Matthew Truch 2005-11-06 08:48:19 EST
I've properly required cfitsio for the -devel package.  Don't know how I missed
this before:

http://matt.truch.net/fedora/cfitsio.spec
http://matt.truch.net/fedora/cfitsio-3.004-0.4.b.src.rpm
Comment 5 Ed Hill 2005-11-06 08:54:22 EST
Hi Matthew, this is not a complete review but I think it catches most of
the remaining problems.  Please fix them and I'll do a (hopefully!) final
review.

nits:
 - please shorten the Summary: to something like
     "Library for manipulating FITS data files"
 - please use the:
    %post -p /sbin/ldconfig
    %postun -p /sbin/ldconfig
   syntax
 - rpmlint reports
     OK to ignore:
       W: cfitsio-devel no-documentation
     probably OK to ignore:
       W: cfitsio no-soname /usr/lib/libcfitsio.so
     these two need to be fixed:
       W: cfitsio one-line-command-in-%post /sbin/ldconfig
       W: cfitsio one-line-command-in-%postun /sbin/ldconfig
 - the license is actually "Distributable" (not GPL) and
     needs to be included with "%doc README Licence.txt"
 - the header files should be put into /usr/include/cfitsio or a
     similar location since they "pollute" the standard name space
     and one of them ("longnam.h") has a mighty generic name

good:
 + source matches upstream
 + builds in mock on FC-4
 + spec is simple and readable
Comment 6 Matthew Truch 2005-11-06 09:32:58 EST
(In reply to comment #5)
> nits:
>  - please shorten the Summary: to something like
>      "Library for manipulating FITS data files"

Done.

>  - please use the:
>     %post -p /sbin/ldconfig
>     %postun -p /sbin/ldconfig
>    syntax

Ditto.

>  - rpmlint reports
>      OK to ignore:
>        W: cfitsio-devel no-documentation
>      probably OK to ignore:
>        W: cfitsio no-soname /usr/lib/libcfitsio.so
>      these two need to be fixed:
>        W: cfitsio one-line-command-in-%post /sbin/ldconfig
>        W: cfitsio one-line-command-in-%postun /sbin/ldconfig

Fixed by implimenting the above changes.

>  - the license is actually "Distributable" (not GPL) and
>      needs to be included with "%doc README Licence.txt"

Included.  However, isn't what License.txt says is that if you compile cfitsio
with the zlib and AIPS code (included, and as I do as it's what cfitsio does by
default), which is GPLed, that cfitsio becomes GPL?  Of course, IANAL, but I
believe that cfitsio is GPL in this case.

I also included other documentation that I never noticed was in the tarball before.

>  - the header files should be put into /usr/include/cfitsio or a
>      similar location since they "pollute" the standard name space
>      and one of them ("longnam.h") has a mighty generic name

Done.  The configure script doesn't obey --includedir, so I had to move the
files manually.  I hope that's ok.  

Let me know how it is now:

http://matt.truch.net/fedora/cfitsio-3.004-0.5.b.src.rpm
Comment 7 Matthew Truch 2005-11-06 12:30:38 EST
I found another (possible) mistake.  cfitsio-devel didn't own the directory it
created (/usr/include/cfitsio), but should:  

http://matt.truch.net/fedora/cfitsio-3.004-0.6.b.src.rpm

PS - I should have read fully the PackageReviewGuidelines, not just the
PackagingGuidelines, PackageNamingGuidlines, and Extras/Contributors.  
Comment 8 Ed Hill 2005-11-12 15:22:24 EST
Created attachment 120989 [details]
changes to the specfile
Comment 9 Ed Hill 2005-11-12 15:31:07 EST
Hi Matthew, looking at the 3.004-0.6.b SRPM it seems that there were 
still a few minor nits, some of which I missed earlier.  They are all 
fixed in the above patch to the specfile and described here:

nits:
 - please delete the "/%{_includedir}/%{name}/*.h" since it
     results in the header files being listed twice (the
     previous "/%{_includedir}/%{name}" already includes all
     of them)
 - rpmlint reports:
       W: cfitsio no-soname /usr/lib/libcfitsio.so
     and the attached (above) spec is one way to fix it.  For
     more info on shared libs, please see:
       http://www.tldp.org/HOWTO/Program-Library-HOWTO/shared-libraries.html
 - please add "unset FC" to the end of the %build section
 - the macro usage can be simplified a bit and some of the
     "/"'s in some paths are redundant

good:
 + builds in mock on FC4
 + with the above patch, the only rpmlint complaint is:
       W: cfitsio-devel no-documentation
     which can be safely ignored

APPROVED.

Please feel free to make the above changes (or equivalent changes) after
you have checked it into CVS but before the first build request.
Comment 10 Michael Schwendt 2005-12-24 12:36:36 EST
> %build
> FC=g95
> export FC
> %configure --prefix=%{buildroot}/%{_prefix}
> --includedir=%{buildroot}/%{_includedir}/%{name}

Caution! %buildroot ought not be specified in the %build section.
It bears the risk that the paths enter binaries (or other files).
Only use %buildroot in the %install section.
Comment 11 Michael Schwendt 2006-03-19 11:08:12 EST
Ping!

Three months have passed without a response. And this spec file
remains a bad example. It's easy to fix:

--- cfitsio.spec.orig   2006-03-10 20:11:18.000000000 +0100
+++ cfitsio.spec        2006-03-19 17:06:48.000000000 +0100
@@ -39,14 +39,14 @@
 %build
 FC=f95
 export FC
-%configure --prefix=%{buildroot}/%{_prefix} --includedir=%{buildroot}/%{_includ
edir}/%{name}
+%configure
 make shared %{?_smp_mflags}
 unset FC
 
 %install
 rm -rf %{buildroot}
 mkdir -p %{buildroot}
-LIBDIR=%{_lib} INCLUDEDIR=include/%{name} make install
+make LIBDIR=%{_lib} INCLUDEDIR=include/%{name} CFITSIO_PREFIX=%{buildroot}%{_pr
efix} install
 pushd %{buildroot}%{_libdir}
 ln -s libcfitsio.so.0 libcfitsio.so
 popd
Comment 12 Matthew Truch 2006-03-19 12:57:56 EST
Sorry,  because the bug wasn't open, I forgot about this issue.  Fixed now. 
Comment 13 Matthew Truch 2007-07-27 11:00:26 EDT
Package Change Request
======================
Package Name: cfitsio
New Branches: EL-4 EL-5

Note You need to log in before you can comment on or make changes to this bug.