Bug 435018 - Review Request: clipper - crystallographic object oriented library
Summary: Review Request: clipper - crystallographic object oriented library
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: gpp4 mmdb
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-02-26 20:26 UTC by Tim Fenn
Modified: 2015-02-01 15:42 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2008-11-19 01:26:58 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
Patch to compile -19 (407 bytes, patch)
2008-11-15 15:42 UTC, Mamoru TASAKA
no flags Details | Diff

Description Tim Fenn 2008-02-26 20:26:35 UTC
Spec URL: http://www.stanford.edu/~fenn/packs/clipper.spec
SRPM URL: http://www.stanford.edu/~fenn/packs/clipper-2.0-10.f8.src.rpm
Description: The aim of the project is to produce a set of object-oriented libraries for the organisation of crystallographic data and the performance of crystallographic computation. The libraries are designed as a framework for new crystallographic software, which will allow the full power of modern programming techniques to be exploited by the developer. This will lead to greater functionality from simpler code which will be easier to develop and debug.

Also see:
http://www.ysbl.york.ac.uk/~cowtan/clipper/clipper.html

Comment 1 Brian Pepple 2008-05-25 20:22:26 UTC
Couple of items that need to be fixed before giving this a proper review:

1. Remove Packager tag.  That is set in the build system.
2. There's no URL tag.
3. The Source file doesn't contain a url.
4. Package contains static library.  Refer:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-82d97fc4a3421310f4e2971180e4165965b65662
5. Package fails to build, due to some missing build requirements (libmmdb-devel
, libgpp4-devel) that aren't available in Fedora.  If you've submitted them for
review, they should be a blocker on this bug.

Comment 2 Tim Fenn 2008-05-26 23:49:18 UTC
Sorry for my clumsiness, I'm still getting accustomed to the Fedora packaging
scheme.

1. Remove Packager tag.  That is set in the build system.

Done.

2. There's no URL tag.

Fixed.

3. The Source file doesn't contain a url.

Fixed, and patches separated out to delineate differences from upstream.

4. Package contains static library.  Refer:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-82d97fc4a3421310f4e2971180e4165965b65662

Fixed (static libraries added to -static package)

5. Package fails to build, due to some missing build requirements (libmmdb-devel
, libgpp4-devel) that aren't available in Fedora.  If you've submitted them for
review, they should be a blocker on this bug.

Done - see:
https://bugzilla.redhat.com/show_bug.cgi?id=435015
https://bugzilla.redhat.com/show_bug.cgi?id=435016

Comment 6 Rakesh Pandit 2008-08-16 15:41:47 UTC

rpmlint output on srpm:
[rpmbuild@rocky clipper]$ rp -i clipper-2.0-13.f8.src.rpm 
clipper.src:40: W: setup-not-quiet
Use the -q option to the %setup macro to avoid useless build output from
unpacking the sources.

clipper.src: W: summary-not-capitalized clipper C++ crystallographic library
Summary doesn't begin with a capital letter.

clipper.src: W: no-version-in-last-changelog
The last changelog entry doesn't contain a version. Please insert the version
that is coherent with the version of the package and rebuild it.

1 packages and 0 specfiles checked; 0 errors, 3 warnings.


After using libmmdb-devel libgpp4-devel packages, clipper failed to built on my i386

make[3]: Entering directory `/home/rpmbuild/rpm/BUILD/clipper-2.0/clipper/mmdb'
/bin/sh ../../libtool --tag=CXX   --mode=compile g++ -DHAVE_CONFIG_H -I. -I../../clipper -I../../clipper   -I/usr/include/gpp4   -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -fno-strict-aliasing -c -o clipper_mmdb.lo clipper_mmdb.cpp
mkdir .libs
 g++ -DHAVE_CONFIG_H -I. -I../../clipper -I../../clipper -I/usr/include/gpp4 -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -fno-strict-aliasing -c clipper_mmdb.cpp  -fPIC -DPIC -o .libs/clipper_mmdb.o
clipper_mmdb.cpp: In member function 'void clipper::MMDBResidue::set_inscode(const clipper::String&)':
clipper_mmdb.cpp:228: error: 'strncpy' was not declared in this scope
make[3]: *** [clipper_mmdb.lo] Error 1
make[3]: Leaving directory `/home/rpmbuild/rpm/BUILD/clipper-2.0/clipper/mmdb'
make[2]: *** [all-recursive] Error 1
make[2]: Leaving directory `/home/rpmbuild/rpm/BUILD/clipper-2.0/clipper'
make[1]: *** [all] Error 2
make[1]: Leaving directory `/home/rpmbuild/rpm/BUILD/clipper-2.0/clipper'
make: *** [all-recursive] Error 1
error: Bad exit status from /home/rpmbuild/rpm/tmp/rpm-tmp.54706 (%build)


RPM build errors:
    Bad exit status from /home/rpmbuild/rpm/tmp/rpm-tmp.54706 (%build)

Comment 7 Tim Fenn 2008-08-21 14:36:13 UTC
Sorry for the late reply, I've been away.

(In reply to comment #6)
> 
> rpmlint output on srpm:
> [rpmbuild@rocky clipper]$ rp -i clipper-2.0-13.f8.src.rpm 
> clipper.src:40: W: setup-not-quiet
> Use the -q option to the %setup macro to avoid useless build output from
> unpacking the sources.
> 
> clipper.src: W: summary-not-capitalized clipper C++ crystallographic library
> Summary doesn't begin with a capital letter.
> 
> clipper.src: W: no-version-in-last-changelog
> The last changelog entry doesn't contain a version. Please insert the version
> that is coherent with the version of the package and rebuild it.
> 

done.

> 
> make[3]: Entering directory 
> clipper_mmdb.cpp: In member function 'void
> clipper::MMDBResidue::set_inscode(const clipper::String&)':
> clipper_mmdb.cpp:228: error: 'strncpy' was not declared in this scope
> make[3]: *** [clipper_mmdb.lo] Error 1
> error: Bad exit status from /home/rpmbuild/rpm/tmp/rpm-tmp.54706 (%build)
> 

I've included a patch to #include <string> in the appropriate files.

new spec: http://www.stanford.edu/~fenn/packs/clipper.spec
new srpm: http://www.stanford.edu/~fenn/packs/clipper-2.0-14.f8.src.rpm

Comment 8 Tim Fenn 2008-09-11 00:05:45 UTC
Cleaned up most rpmlint problems with the -devel package:

new spec: http://www.stanford.edu/~fenn/packs/clipper.spec
new srpm: http://www.stanford.edu/~fenn/packs/clipper-2.0-15.f8.src.rpm

Comment 9 Rakesh Pandit 2008-09-29 12:38:17 UTC
You are not sponsored yet. So I have added FE-NEEDSPONSOR here also.

Thanks

Comment 10 Mamoru TASAKA 2008-10-23 18:24:59 UTC
(Removing NEEDSPONSOR: bug 462250)

Comment 11 Mamoru TASAKA 2008-11-11 06:55:31 UTC
Would you rewrite your spec/srpm? I guess you once want
to clean up your spec file.

Comment 12 Tim Fenn 2008-11-11 21:15:13 UTC
(In reply to comment #11)
> Would you rewrite your spec/srpm? I guess you once want
> to clean up your spec file.

Yes, now that all the deps are cleaned up, I did a bit of a cleaning/fixing:

new spec: http://www.stanford.edu/~fenn/packs/clipper.spec
new srpm: http://www.stanford.edu/~fenn/packs/clipper-2.0-16.f8.src.rpm

Comment 13 Mamoru TASAKA 2008-11-12 16:21:55 UTC
Rebuild failed on dist-f11:
http://koji.fedoraproject.org/koji/taskinfo?taskID=928611

Comment 15 Mamoru TASAKA 2008-11-13 09:35:06 UTC
For -17:

* License
  - Well, the description of the license in the source codes seems
    somewhat confusing, however the license tag can be safe
    with "LGPLv2+".

* Source1
  - Is this source created by yourself or can it be downloaded
    somewhere? (If some URL exists, write the URL like
    Source0)

* Dependency for -devel subpackage
  - Installed clipper.pc contains:
--------------------------------------------------------
     8  Requires: mmdb gpp4
    10  Libs: -L/usr/lib -lclipper -lsrfftw -lsfftw -lm 
--------------------------------------------------------
    This means that -devel subpackage must have
    "Requires: mmdb-devel gpp4-devel fftw2-devel", however
    - Would you check if "Requires: gpp4" and
      "Libs: -lstfftw -lsfftw" are really needed?
      Installed clipper-devel header files do not seem to
      depend on any header files in gpp4-devel or fftw2-devel",
      so I guess these Requires and Libs can be removed

    ! Note: "Requires: mmdb" is still needed (this means
      that clipper-devel must have "Requires: mmdb-devel")
      because clipper/mmdb/clipper_mmdb.h contains:
--------------------------------------------------------
    50  
    51  #include <mmdb/mmdb_manager.h>
    52  
--------------------------------------------------------

* %setup
  - I guess %setup can be replaced by
--------------------------------------------------------
%prep
%setup -q -c -a 1
--------------------------------------------------------

! Timestamps
  - Not a blocker, however for packages using "install-sh" to
    install files, 
--------------------------------------------------------
make install DESTDIR=$RPM_BUILD_ROOT CPPROG="cp -p"
--------------------------------------------------------
    to keep timestamps on installed files

* %defattr
  - Now we recommend %defattr(-,root,root,-)

* Documents
  - The file "INSTALL" is usually for people who wants to compile/
    install the package by himself and not needed for rpm users.

* Directory ownership issue
  - %_includedir/clipper is not owned by any packages
    ref:
    https://fedoraproject.org/wiki/Packaging/UnownedDirectories#Wildcarding_Files_inside_a_Created_Directory

* rpmlint
--------------------------------------------------------
clipper.i386: E: zero-length /usr/share/doc/clipper-2.0/ChangeLog
clipper-devel.i386: W: wrong-file-end-of-line-encoding /usr/share/doc/clipper-devel-2.0/dox/develop.dox
clipper-devel.i386: W: wrong-file-end-of-line-encoding /usr/share/doc/clipper-devel-2.0/dox/coordtypes.dox
clipper-devel.i386: W: spurious-executable-perm /usr/share/doc/clipper-devel-2.0/dox/wheretolook.dox
--------------------------------------------------------
  - Including zero size "ChangeLog" seems meaningless
  - Please fix CRLF (Windows-like) end-of-line endcodings.
  - Usually files included as %doc must have 0644 permission

Comment 16 Tim Fenn 2008-11-13 20:49:56 UTC
(In reply to comment #15)
> For -17:
> 
> * License
>   - Well, the description of the license in the source codes seems
>     somewhat confusing, however the license tag can be safe
>     with "LGPLv2+".
> 

OK.

> * Source1
>   - Is this source created by yourself or can it be downloaded
>     somewhere? (If some URL exists, write the URL like
>     Source0)
> 

Created by me - I've submitted it upstream as well.

> * Dependency for -devel subpackage
>   - Installed clipper.pc contains:
> --------------------------------------------------------
>      8  Requires: mmdb gpp4
>     10  Libs: -L/usr/lib -lclipper -lsrfftw -lsfftw -lm 
> --------------------------------------------------------
>     This means that -devel subpackage must have
>     "Requires: mmdb-devel gpp4-devel fftw2-devel", however
>     - Would you check if "Requires: gpp4" and
>       "Libs: -lstfftw -lsfftw" are really needed?
>       Installed clipper-devel header files do not seem to
>       depend on any header files in gpp4-devel or fftw2-devel",
>       so I guess these Requires and Libs can be removed
> 

several of the functions under the ccp4/ folder are wrapped C++ calls to gpp4 functions, and functions in core/fftmap.h contain wrapped fftw calls.  I've added the Requires to -devel.

> 
> * %setup
>   - I guess %setup can be replaced by
> --------------------------------------------------------
> %prep
> %setup -q -c -a 1
> --------------------------------------------------------
> 

done.

> ! Timestamps
>   - Not a blocker, however for packages using "install-sh" to
>     install files, 
> --------------------------------------------------------
> make install DESTDIR=$RPM_BUILD_ROOT CPPROG="cp -p"
> --------------------------------------------------------
>     to keep timestamps on installed files
> 

done.

> * %defattr
>   - Now we recommend %defattr(-,root,root,-)
> 

oops, fixed.

> * Documents
>   - The file "INSTALL" is usually for people who wants to compile/
>     install the package by himself and not needed for rpm users.
> 

removed.

> * Directory ownership issue
>   - %_includedir/clipper is not owned by any packages
>     ref:
>    

Ah, thanks!  fixed.

> 
> * rpmlint
> --------------------------------------------------------
> clipper.i386: E: zero-length /usr/share/doc/clipper-2.0/ChangeLog
> clipper-devel.i386: W: wrong-file-end-of-line-encoding
> /usr/share/doc/clipper-devel-2.0/dox/develop.dox
> clipper-devel.i386: W: wrong-file-end-of-line-encoding
> /usr/share/doc/clipper-devel-2.0/dox/coordtypes.dox
> clipper-devel.i386: W: spurious-executable-perm
> /usr/share/doc/clipper-devel-2.0/dox/wheretolook.dox
> --------------------------------------------------------
>   - Including zero size "ChangeLog" seems meaningless
>   - Please fix CRLF (Windows-like) end-of-line endcodings.
>   - Usually files included as %doc must have 0644 permission

removed the changelog file, fixed CRLF foo and file perms.

new spec: http://www.stanford.edu/~fenn/packs/clipper.spec
new srpm: http://www.stanford.edu/~fenn/packs/clipper-2.0-18.f8.src.rpm

Comment 17 Mamoru TASAKA 2008-11-14 08:31:35 UTC
For -18:

(In reply to comment #16)
> (In reply to comment #15)

> > * Directory ownership issue
> >   - %_includedir/clipper is not owned by any packages
> >     ref:
>
> Ah, thanks!  fixed.

  - This time build.log says:
------------------------------------------------------------
  2116  + exit 0
  2117  warning: File listed twice: /usr/include/clipper/ccp4
  2118  warning: File listed twice: /usr/include/clipper/ccp4/ccp4_map_io.h
  2119  warning: File listed twice: /usr/include/clipper/ccp4/ccp4_mtz_io.h
  2120  warning: File listed twice: /usr/include/clipper/ccp4/ccp4_mtz_types.h
  2121  warning: File listed twice: /usr/include/clipper/ccp4/ccp4_utils.h
  2122  warning: File listed twice: /usr/include/clipper/cif
------------------------------------------------------------
    As written on the wiki I quoted on my comment 15,
    %files entry
------------------------------------------------------------
%files
%{_includedir}/clipper
------------------------------------------------------------
    contains the directory %_includedir/clipper _and_ all
    files/directories/etc under %_includedir/clipper.

? Static linkage
  - The following command
------------------------------------------------------------
$ rpm -ql clipper | grep /usr/bin | xargs ldd -r
------------------------------------------------------------
    shows no binaries in clipper rpm under %_bindir do not depend
    on libclipper.so. This seems rather strange.

    For example, caniso binaries are created like:
------------------------------------------------------------
  1842  g++ -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -fno-strict-aliasing -o caniso caniso.o  ../clipper/mmdb/.libs/libclippermmdb.a ../clipper/cif/.libs/libclippercif.a ../clipper/cns/.libs/libclippercns.a ../clipper/minimol/.libs/libclipperminimol.a ../clipper/phs/.libs/libclipperphs.a ../clipper/ccp4/.libs/libclipperccp4.a -lgpp4 -lmmdb ../clipper/contrib/.libs/libclippercontrib.a ../clipper/core/.libs/libclippercore.a -lsrfftw -lsfftw -lm  
------------------------------------------------------------
    So these binaries use static libclipperXXXX.a. I guess these
    binaries should use system wide libclipper.so.

Comment 18 Tim Fenn 2008-11-14 19:27:48 UTC
(In reply to comment #17)
> For -18:
> 
> (In reply to comment #16)
> > (In reply to comment #15)
> 
> > > * Directory ownership issue
> > >   - %_includedir/clipper is not owned by any packages
> > >     ref:
> >
> > Ah, thanks!  fixed.
> 
>   - This time build.log says:
> ------------------------------------------------------------
>   2116  + exit 0
>   2117  warning: File listed twice: /usr/include/clipper/ccp4
>   2118  warning: File listed twice: /usr/include/clipper/ccp4/ccp4_map_io.h
>   2119  warning: File listed twice: /usr/include/clipper/ccp4/ccp4_mtz_io.h
>   2120  warning: File listed twice: /usr/include/clipper/ccp4/ccp4_mtz_types.h
>   2121  warning: File listed twice: /usr/include/clipper/ccp4/ccp4_utils.h
>   2122  warning: File listed twice: /usr/include/clipper/cif
> ------------------------------------------------------------
>     As written on the wiki I quoted on my comment 15,
>     %files entry
> ------------------------------------------------------------
> %files
> %{_includedir}/clipper
> ------------------------------------------------------------
>     contains the directory %_includedir/clipper _and_ all
>     files/directories/etc under %_includedir/clipper.
> 

Sorry, I was rushing a bit - removed the extraneous entries.

> ? Static linkage
>   - The following command
> ------------------------------------------------------------
> $ rpm -ql clipper | grep /usr/bin | xargs ldd -r
> ------------------------------------------------------------
>     shows no binaries in clipper rpm under %_bindir do not depend
>     on libclipper.so. This seems rather strange.
> 

fixed Makefile.am for the examples

new spec: http://www.stanford.edu/~fenn/packs/clipper.spec
new srpm: http://www.stanford.edu/~fenn/packs/clipper-2.0-19.f8.src.rpm

Comment 19 Mamoru TASAKA 2008-11-15 15:42:00 UTC
Created attachment 323692 [details]
Patch to compile -19

Rebuild failed:
http://koji.fedoraproject.org/koji/taskinfo?taskID=934459

The attached patch will fix this issue:
http://koji.fedoraproject.org/koji/taskinfo?taskID=934517

Comment 20 Mamoru TASAKA 2008-11-15 15:43:22 UTC
Other things are okay.

------------------------------------------------------
   This package (clipper) is APPROVED by mtasaka
------------------------------------------------------

Comment 21 Tim Fenn 2008-11-15 23:42:16 UTC
(In reply to comment #19)
> Created an attachment (id=323692) [details]
> Patch to compile -19
> 

Thanks, applied!

new spec: http://www.stanford.edu/~fenn/packs/clipper.spec
new srpm: http://www.stanford.edu/~fenn/packs/clipper-2.0-20.f8.src.rpm

Comment 22 Tim Fenn 2008-11-15 23:43:33 UTC
New Package CVS Request
=======================
Package Name: clipper
Short Description: crystallographic object oriented library
Owners: timfenn
Branches: F-9 F-10 EL-5
InitialCC: timfenn

Comment 23 Kevin Fenzi 2008-11-16 20:32:05 UTC
cvs done.

Comment 24 Tim Fenn 2008-11-18 23:15:09 UTC
cvs builds done, update request submitted to bodhi for F-9 and F-10.

Comment 25 Mamoru TASAKA 2008-11-19 01:26:58 UTC
Thank you.

Comment 26 Tim Fenn 2015-01-31 20:30:29 UTC
Package Change Request
======================
Package Name: clipper
New Branches: epel7
Owners: timfenn
InitialCC: timfenn

Comment 27 Gwyn Ciesla 2015-02-01 15:42:15 UTC
Git done (by process-git-requests).


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