Bug 435017 - Review Request: ssm - coordinate superposition library
Summary: Review Request: ssm - coordinate superposition 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: mmdb
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-02-26 20:26 UTC by Tim Fenn
Modified: 2015-02-01 15:41 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-11-19 04:00:21 UTC
Type: ---
mtasaka: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
Proposed spec changes (817 bytes, patch)
2008-05-29 07:49 UTC, Ralf Corsepius
no flags Details | Diff

Description Tim Fenn 2008-02-26 20:26:31 UTC
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

Comment 1 Tim Fenn 2008-05-26 23:44:32 UTC
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

Comment 2 Ralf Corsepius 2008-05-27 01:04:31 UTC
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]


Comment 3 Tim Fenn 2008-05-27 09:55:08 UTC
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

Comment 4 Ralf Corsepius 2008-05-27 13:07:26 UTC
(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.



Comment 5 Tim Fenn 2008-05-27 21:18:17 UTC
(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


Comment 6 Ralf Corsepius 2008-05-28 05:03:00 UTC
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


Comment 7 Tim Fenn 2008-05-29 01:37:29 UTC
(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


Comment 8 Ralf Corsepius 2008-05-29 07:49:22 UTC
Created attachment 307021 [details]
Proposed spec changes

Attaching patch to fix remaining issues.

Comment 9 Tim Fenn 2008-05-30 03:42:22 UTC
(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


Comment 10 Tim Fenn 2008-10-26 00:28:42 UTC
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

Comment 11 Mamoru TASAKA 2008-11-08 07:35:23 UTC
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)

Comment 12 Tim Fenn 2008-11-10 18:26:45 UTC
(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

Comment 13 Mamoru TASAKA 2008-11-11 06:52:55 UTC
Okay.

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

Comment 14 Tim Fenn 2008-11-11 20:32:00 UTC
New Package CVS Request
=======================
Package Name: ssm
Short Description: coordinate superposition library
Owners: timfenn
Branches: F-9 F-10 EL-5
InitialCC: timfenn

Comment 15 Kevin Fenzi 2008-11-12 18:55:06 UTC
cvs done.

Comment 16 Tim Fenn 2008-11-13 00:27:27 UTC
cvs builds done, update request submitted to bodhi for F-9 and F-10.

Comment 17 Mamoru TASAKA 2008-11-19 04:00:21 UTC
Closing, thanks.

Comment 18 Tim Fenn 2015-01-31 20:28:08 UTC
Package Change Request
======================
Package Name: ssm
New Branches: epel7
Owners: timfenn
InitialCC: timfenn

Comment 19 Gwyn Ciesla 2015-02-01 15:41:57 UTC
Git done (by process-git-requests).


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