Red Hat Bugzilla – Bug 435018
Review Request: clipper - crystallographic object oriented library
Last modified: 2015-02-01 10:42:15 EST
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
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.
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
Updated files: Spec URL: http://www.stanford.edu/~fenn/packs/clipper.spec SRPM URL: http://www.stanford.edu/~fenn/packs/clipper-2.0-11.f8.src.rpm
More updates as per https://bugzilla.redhat.com/show_bug.cgi?id=435017#c2 new spec: http://www.stanford.edu/~fenn/packs/clipper.spec new srpm: http://www.stanford.edu/~fenn/packs/clipper-2.0-12.f8.src.rpm
Removed static libs. new spec: http://www.stanford.edu/~fenn/packs/clipper.spec new srpm: http://www.stanford.edu/~fenn/packs/clipper-2.0-13.f8.src.rpm
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)
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
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
You are not sponsored yet. So I have added FE-NEEDSPONSOR here also. Thanks
(Removing NEEDSPONSOR: bug 462250)
Would you rewrite your spec/srpm? I guess you once want to clean up your spec file.
(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
Rebuild failed on dist-f11: http://koji.fedoraproject.org/koji/taskinfo?taskID=928611
(In reply to comment #13) > Rebuild failed on dist-f11: > http://koji.fedoraproject.org/koji/taskinfo?taskID=928611 Added a patch to fix the build problems, tested on koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=929474 new spec: http://www.stanford.edu/~fenn/packs/clipper.spec new srpm: http://www.stanford.edu/~fenn/packs/clipper-2.0-17.f8.src.rpm
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
(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
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.
(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
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
Other things are okay. ------------------------------------------------------ This package (clipper) is APPROVED by mtasaka ------------------------------------------------------
(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
New Package CVS Request ======================= Package Name: clipper Short Description: crystallographic object oriented library Owners: timfenn Branches: F-9 F-10 EL-5 InitialCC: timfenn
cvs done.
cvs builds done, update request submitted to bodhi for F-9 and F-10.
Thank you.
Package Change Request ====================== Package Name: clipper New Branches: epel7 Owners: timfenn InitialCC: timfenn
Git done (by process-git-requests).