Bug 435017 - Review Request: ssm - coordinate superposition library
Review Request: ssm - coordinate superposition library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On: mmdb
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-26 15:26 EST by Tim Fenn
Modified: 2015-02-01 10:41 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-11-18 23:00:21 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
limburgher: fedora‑cvs+


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

  None (edit)
Description Tim Fenn 2008-02-26 15:26:31 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
Comment 1 Tim Fenn 2008-05-26 19:44:32 EDT
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-26 21:04:31 EDT
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 05:55:08 EDT
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 09:07:26 EDT
(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 17:18:17 EDT
(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 01:03:00 EDT
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-28 21:37:29 EDT
(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 03:49:22 EDT
Created attachment 307021 [details]
Proposed spec changes

Attaching patch to fix remaining issues.
Comment 9 Tim Fenn 2008-05-29 23:42:22 EDT
(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-25 20:28:42 EDT
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 02:35:23 EST
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 13:26:45 EST
(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 01:52:55 EST
Okay.

---------------------------------------------------
    This package (ssm) is APPROVED by mtasaka
---------------------------------------------------
Comment 14 Tim Fenn 2008-11-11 15:32:00 EST
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 13:55:06 EST
cvs done.
Comment 16 Tim Fenn 2008-11-12 19:27:27 EST
cvs builds done, update request submitted to bodhi for F-9 and F-10.
Comment 17 Mamoru TASAKA 2008-11-18 23:00:21 EST
Closing, thanks.
Comment 18 Tim Fenn 2015-01-31 15:28:08 EST
Package Change Request
======================
Package Name: ssm
New Branches: epel7
Owners: timfenn
InitialCC: timfenn
Comment 19 Jon Ciesla 2015-02-01 10:41:57 EST
Git done (by process-git-requests).

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