Red Hat Bugzilla – Bug 435017
Review Request: ssm - coordinate superposition library
Last modified: 2015-02-01 10:41:57 EST
Spec URL: http://www.stanford.edu/~fenn/packs/ssm.spec SRPM URL: http://www.stanford.edu/~fenn/packs/ssm-0.1-2.f8.src.rpm Description: SSM is a macromolecular coordinate superposition library, written by Eugene Krissinel of the EBI. The library implements the SSM algorithm of protein structure comparison in three dimensions, which includes an original procedure of matching graphs built on the protein's secondary-structure elements, followed by an iterative three-dimensional alignment of protein backbone Calpha atoms. Also see: http://www.ebi.ac.uk/msd-srv/ssm/ http://www.bioxray.dk/~mok/ssm.php
Source RPM and spec file updated to bring RPM inline with current Fedora guidelines. Spec URL: http://www.stanford.edu/~fenn/packs/ssm.spec SRPM URL: http://www.stanford.edu/~fenn/packs/ssm-0.1-3.f8.src.rpm
some comments based on coarse visual inspection of the rpm.spec: MUSTFIX: - Don't ship *.la's Remove them in %install SHOULD: - %makeinstall is considered obsolete. Better use "make DESTDIR=${RPM_BUILD_ROOT} install" if feasible. CONSIDER: - Feel strongly encouraged not to ship static libraries (consider adding --disable-static to %configure) [Analogous comments also apply to further packages related to this package you currently have pending for review]
MUSTFIX: - Don't ship *.la's Remove them in %install done SHOULD: - %makeinstall is considered obsolete. Better use "make DESTDIR=${RPM_BUILD_ROOT} install" if feasible. done CONSIDER: - Feel strongly encouraged not to ship static libraries (consider adding --disable-static to %configure) Unfortunately, many scientific folk still use statically linked libs when building execs, and like to have them available. [Analogous comments also apply to further packages related to this package you currently have pending for review] done new spec: http://www.stanford.edu/~fenn/packs/ssm.spec new url: http://www.stanford.edu/~fenn/packs/ssm-0.1-4.f8.src.rpm
(In reply to comment #3) > CONSIDER: > - Feel strongly encouraged not to ship static libraries (consider adding > --disable-static to %configure) > > Unfortunately, many scientific folk still use statically linked libs when > building execs, and like to have them available. These people should reconsider their habits. They should learn to use LD_LIBRARY_PATH and to bundle the shared libs their applications are using. To me, presence of static libs is sufficient reason for not wanting to continue this review.
(In reply to comment #4) > (In reply to comment #3) > > > CONSIDER: > > - Feel strongly encouraged not to ship static libraries (consider adding > > --disable-static to %configure) > > > > Unfortunately, many scientific folk still use statically linked libs when > > building execs, and like to have them available. > These people should reconsider their habits. They should learn to use > LD_LIBRARY_PATH and to bundle the shared libs their applications are using. > > To me, presence of static libs is sufficient reason for not wanting to continue > this review. > Removed. new spec: http://www.stanford.edu/~fenn/packs/ssm.spec new srpm: http://www.stanford.edu/~fenn/packs/ssm-0.1-5.f8.src.rpm
Thanks for not shipping static libs :) Several major issues: * Building in mocks fails: ... checking for MMDB... configure: error: The pkg-config script could not be found or is too old. Make sure it is in your PATH or set the PKG_CONFIG environment variable to the full path to pkg-config. Alternatively, you may set the environment variables MMDB_CFLAGS and MMDB_LIBS to avoid the need to call pkg-config. See the pkg-config man page for more details. ... => Missing "BuildRequires: mmdb-devel" * *-devel ships a *.pc => "Requires: pkgconfig" needs to be added to *-devel * Your BuildRoot: doesn't comply to the FPG. Please use one of the options described in https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag * /sbin/ldconfig scriptlets missing from "main-package" cf. https://fedoraproject.org/wiki/PackagingDrafts/ScriptletSnippets#Shared_libraries
(In reply to comment #6) > Thanks for not shipping static libs :) > :) Thanks for the help in finding my silly errors! > Several major issues: > > => Missing "BuildRequires: mmdb-devel" > Done. > * *-devel ships a *.pc > => "Requires: pkgconfig" needs to be added to *-devel > Done. > * Your BuildRoot: doesn't comply to the FPG. > Please use one of the options described in > https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag > Fixed. > * /sbin/ldconfig scriptlets missing from "main-package" > cf. > https://fedoraproject.org/wiki/PackagingDrafts/ScriptletSnippets#Shared_libraries > Done. new spec: http://www.stanford.edu/~fenn/packs/ssm.spec new srpm: http://www.stanford.edu/~fenn/packs/ssm-0.1-6.f8.src.rpm
Created attachment 307021 [details] Proposed spec changes Attaching patch to fix remaining issues.
(In reply to comment #8) > Created an attachment (id=307021) [edit] > Proposed spec changes > > Attaching patch to fix remaining issues. > applied. new spec: http://www.stanford.edu/~fenn/packs/ssm.spec new srpm: http://www.stanford.edu/~fenn/packs/ssm-0.1-7.f8.src.rpm
Changed naming from libssm to ssm (see https://bugzilla.redhat.com/show_bug.cgi?id=435015 and discussion therein). new spec: http://www.stanford.edu/~fenn/packs/ssm.spec new srpm: http://www.stanford.edu/~fenn/packs/ssm-0.1-8.f8.src.rpm
For -8: * License - The license tag should be LGPLv2+. * BuildRequires - This package won't build without "BuildRequires: mmdb-devel". * configure option ------------------------------------------------------------ 55 ++ pkg-config --variable=prefix mmdb 56 + ./configure --build=i386-redhat-linux-gnu --host=i386-redhat-linux-gnu --target=i386-redhat-linux-gnu --program-prefix= --prefix=/usr --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib --libexecdir=/usr/libexec --localstatedir=/var --sharedstatedir=/usr/com --mandir=/usr/share/man --infodir=/usr/share/info --with-mmdb=/usr --disable-static 57 configure: WARNING: unrecognized options: --with-mmdb ------------------------------------------------------------ - Is the option "--with-mmdb=foo" useful? * Timestamps ------------------------------------------------------------ make install DESTDIR=$RPM_BUILD_ROOT install='install -p' ------------------------------------------------------------ - The last argument should be "INSTALL='install -p'". * Document - There is no need that -devel subpackage should own the same document files in main package. * rpmlint ** undefined-non-weak-symbol - "$ rpmlint ssm" (please try rpmlint for 'installed' ssm) shows that libssm.so.0 contains many undefined non-weak symbols. ! You can check this also by $ ldd -r %_libdir/libssm.so.0 For packages providing -devel subpackage this cannot be allowed because this causes linkage error. I guess making libssm.so linked against libmmdb.so will fix this issue (because LD_PRELOAD=%_libdir/libmmdb.so.0.0.0 ldd -r %_libdir/libssm.so.0.0.0 shows no undefined non-weak symbols)
(In reply to comment #11) > For -8: > > * License > - The license tag should be LGPLv2+. > Fixed. > * BuildRequires > - This package won't build without "BuildRequires: mmdb-devel". > Fixed. > * configure option > ------------------------------------------------------------ > 55 ++ pkg-config --variable=prefix mmdb > 56 + ./configure --build=i386-redhat-linux-gnu > --host=i386-redhat-linux-gnu --target=i386-redhat-linux-gnu --program-prefix= > --prefix=/usr --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin > --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include > --libdir=/usr/lib --libexecdir=/usr/libexec --localstatedir=/var > --sharedstatedir=/usr/com --mandir=/usr/share/man --infodir=/usr/share/info > --with-mmdb=/usr --disable-static > 57 configure: WARNING: unrecognized options: --with-mmdb > ------------------------------------------------------------ > - Is the option "--with-mmdb=foo" useful? > I forgot to include a patch to one of the makefiles to go along with the configure patch - fixed. > * Timestamps > ------------------------------------------------------------ > make install DESTDIR=$RPM_BUILD_ROOT install='install -p' > ------------------------------------------------------------ > - The last argument should be "INSTALL='install -p'". > Fixed. > * Document > - There is no need that -devel subpackage should own the same > document files in main package. > Removed. > * rpmlint > ** undefined-non-weak-symbol > - "$ rpmlint ssm" (please try rpmlint for 'installed' ssm) shows > that libssm.so.0 contains many undefined non-weak symbols. > ! You can check this also by $ ldd -r %_libdir/libssm.so.0 > For packages providing -devel subpackage this cannot be > allowed because this causes linkage error. > > I guess making libssm.so linked against libmmdb.so will fix this > issue > (because > LD_PRELOAD=%_libdir/libmmdb.so.0.0.0 ldd -r %_libdir/libssm.so.0.0.0 > shows no undefined non-weak symbols) Yeah, this all gets cleaned up with proper linking to libmmdb. new spec: http://www.stanford.edu/~fenn/packs/ssm.spec new srpm: http://www.stanford.edu/~fenn/packs/ssm-0.1-9.f8.src.rpm
Okay. --------------------------------------------------- This package (ssm) is APPROVED by mtasaka ---------------------------------------------------
New Package CVS Request ======================= Package Name: ssm Short Description: coordinate superposition 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.
Closing, thanks.
Package Change Request ====================== Package Name: ssm New Branches: epel7 Owners: timfenn InitialCC: timfenn
Git done (by process-git-requests).