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.
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 :-) ]
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?
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
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
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
(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
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.
Created attachment 120989 [details] changes to the specfile
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.
> %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.
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
Sorry, because the bug wasn't open, I forgot about this issue. Fixed now.
Package Change Request ====================== Package Name: cfitsio New Branches: EL-4 EL-5