Bug 181777 - Review Request: CCfits A C++ interface for cfitsio
Review Request: CCfits A C++ interface for cfitsio
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ed Hill
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-02-16 10:00 EST by Sergio Pascual
Modified: 2007-11-30 17:11 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-03-08 05:36:13 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Sergio Pascual 2006-02-16 10:00:56 EST
Spec Name or Url: http://t-rex.fis.ucm.es/~spr/CCfits.spec
SRPM Name or Url: http://t-rex.fis.ucm.es/~spr/CCfits-1.4-0.fc4.src.rpm
Description: CCfits is an object oriented interface to the cfitsio library. 
It is designed
to make the capabilities of cfitsio available to programmers working in C++.
It is written in ANSI C++ and implemented using the C++ Standard Library
with namespaces, exception handling, and member template functions.

This is my first request for a review, so I'm also in need of a sponsor.
Comment 1 Ed Hill 2006-02-19 13:27:01 EST
Hi Sergio, I started to do a review of this package and got this far:

good:
 + specfile is legible
 + builds on FC4 (will try in mock later)
 + dir ownership looks good
 + *.la files removed

needswork:
 - Something is the matter with the upstream server.  All my connections
   to it are timing out so I can't get a copy of the upstream code to
   compare.  Hopefully, it'll get fixed soon and we'll be able to proceed!

 - builds on FC4 and rpmlint returns the following:
W: CCfits summary-ended-with-dot A C++ interface for cfitsio (FITS File
Subroutine Library).
W: CCfits invalid-license GPL compatible (see License.txt)
E: CCfits binary-or-shlib-defines-rpath /usr/lib/libCCfits.so.0.0.0 ['/usr/lib']
W: CCfits-devel summary-ended-with-dot Headers for developing programs that will
use CCfits.
W: CCfits-devel invalid-license GPL compatible (see License.txt)
W: CCfits-docs invalid-license GPL compatible (see License.txt)
E: CCfits-docs script-without-shellbang
/usr/share/doc/CCfits-docs-1.4/html/support_subs.pl
E: CCfits-docs wrong-script-interpreter
/usr/share/doc/CCfits-docs-1.4/html/ccfitschange_sff.pl "/usr1/local/bin/perl5"
W: CCfits-docs doc-file-dependency
/usr/share/doc/CCfits-docs-1.4/html/ccfitschange_sff.pl /usr1/local/bin/perl5

 - The license is essentially BSD without the advertisement clause, so
   please list it as BSD (which is GPL-compatible)
 - Please change the main Summary: to "A C++ interface for cfitsio" and
   remove the trailing "."-s in the others
 - the "rpath" mentioned above will probably need to be fixed

And I'm sorry this isn't a full review -- am going to need to get a copy
from upstream to complete it.
Comment 2 Sergio Pascual 2006-02-20 15:34:25 EST
Hi Ed, I have fixed the points you mentioned:

* Changed license type to BSD. The license for CCfits is exactly the same as the
license of cfitsio, but in that package the license field of the rpm is GPL!

* Removed some perl files in the html directory
* Removed the trailing dots in Summary and
* Removed rpath in the shared library compile instruction.

The new spec and srpm are here:
http://t-rex.fis.ucm.es/~spr/CCfits.spec
http://t-rex.fis.ucm.es/~spr/CCfits-1.4-1.fc4.src.rpm
Comment 3 Ed Hill 2006-02-20 20:41:49 EST
Hi Sergio, please take a look at bug # 172042 where Matt points out that
BSD code linked against GPL code becomes GPL-ed.  I think thats correct 
but IANAL.  And I'll try to finish the review later this week or the 
coming weekend.
Comment 4 Ed Hill 2006-03-05 13:35:29 EST
Hi Sergio, the 1.4-1 SRPM and its clearly an improvement:

 + no more errors or warnings from rpmlint
 + builds in mock on FC4

One blocker remains, though.  The source copy in your 1.4-1 SRPM does not 
match the upstream copy.  I untarred both and a diff shows that upstream 
has apparently removed the "CCfits/License.txt" file.  And I don't see any 
mention of the license terms on the upstream web site, but I may have 
somehow missed it (please point it out if I did!).

Ultimately, I think the license is fine since works made by the US 
government for research purposes are frequently "distributable" and usable 
by all -- unless otherwise noted.  So, could you please create an updated 
SRPM that removes the need for the "CCfits/License.txt" file and I'll take 
a quick (and hopefully last!) one look?
Comment 5 Sergio Pascual 2006-03-06 16:07:08 EST
Great!

I looked for the license on the web site also and, as I didn't find it, I
contacted with the upstream mantainer. He sent me the License.txt file and also
told me that he has planned to include the license in the next upstream release.

The updated (and I hope final) spec and srpm without License.txt are here:
http://t-rex.fis.ucm.es/~spr/CCfits.spec
http://t-rex.fis.ucm.es/~spr/CCfits-1.4-2.fc4.src.rpm

Comment 6 Ed Hill 2006-03-06 21:44:15 EST
OK, I'm glad that the license will be included in future releases.  
Thats good because it clarifies things.  However, that does not mean 
that you should create your own tar-file (as you have done in the last 
two SRPMs) that include the license file.  What you should do is keep 
the source pristine (use an *exact* copy of the upstream tar file) and 
add the license file as a separate patch.

So we still have a blocker here and its the same as before: the souce 
does not match upstream!  Please fix it by either:

 - including the licence file as a aeparate patch, or
 - leaving out the license file (for now) and including it in a 
   later version -- once the upstream folks start shipping it in 
   the official sources

The choice is yours.  But the current form of the SRPM is not acceptable.
Comment 7 Sergio Pascual 2006-03-07 04:40:08 EST
Please check that http://t-rex.fis.ucm.es/~spr/CCfits-1.4-2.fc4.src.rpm does not
include License.txt
Comment 8 Ed Hill 2006-03-07 08:47:48 EST
Hi Sergio, the tar-file contained in your SRPM:

  84e8387ee5c399c7bf0888e14576bc2b  CCfits-1.4-2.fc4.src.rpm

still does not match the upstream tar file:

  707777558c46f153fe0b1d994bab4a52  CCfits-1.4.tar.gz
  230edb39de6a1e3dc6e0adda65d0676d  CCfits-1.4.tar.gz.upstream

and this is a blocker.  And, yes, it does appear that your tar file does 
not contain the License.txt file.  So, its possible that I confused the 
tar file from your SRPM and the one from upstream in the previous review 
when I was trying to see why they differed.  If so, I apologize for any 
confusion.

In any case, the point is that you should ship an exact copy of the 
upstream sources.  So please create a CCfits-1.4-3.fc4.src.rpm that has
an exact copy and I'll approve it.
Comment 9 Sergio Pascual 2006-03-07 09:37:48 EST
Hi Ed.

The problem was that the upstream source was changed without changing the
version number and I didn't notice it. The source now includes the license and
that confused us.

Here are the new spec and srpm:
http://t-rex.fis.ucm.es/~spr/CCfits-1.4-3.fc4.src.rpm 
http://t-rex.fis.ucm.es/~spr/CCfits.spec


Comment 10 Ed Hill 2006-03-07 10:02:10 EST
OK, I don't see any remaining blockers:

  a37b2dadbb4ca6791bab70ac31950a9e  CCfits-1.4-3.fc4.src.rpm

APPROVED.
Comment 11 Denis Leroy 2006-03-07 11:31:02 EST
The BuildRequires: gcc-c++ is not necessary.
Comment 12 Sergio Pascual 2006-03-08 05:22:26 EST
Yes, I see gcc-c++ is listed as a exception in
http://fedoraproject.org/wiki/Packaging/Guidelines

I will change it.

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