Bug 435016 (mmdb) - Review Request: mmdb - MMDB coordinate library
Summary: Review Request: mmdb - MMDB coordinate library
Keywords:
Status: CLOSED NEXTRELEASE
Alias: mmdb
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 435017 435018
TreeView+ depends on / blocked
 
Reported: 2008-02-26 20:26 UTC by Tim Fenn
Modified: 2008-11-08 07:16 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-11-08 07:16:20 UTC
Type: ---
Embargoed:
j: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Tim Fenn 2008-02-26 20:26:25 UTC
Spec URL: http://www.stanford.edu/~fenn/packs/mmdb.spec
SRPM URL: http://www.stanford.edu/~fenn/packs/mmdb-1.09.1-2.f8.src.rpm
Description: MMDB is CCP4's macromolecular coordinate library, written by Eugene Krissinel of the EBI. The Coordinate Library is designed to assist CCP4 developers in working with coordinate files. The major source of coordinate information remains the PDB files, although more information becomes available in mmCIF format.

The Library features working with both file formats plus an internal binary format portable between different platforms. This is achieved at uniformity of the Library's interface functions, so that there is no difference in handling different formats.

The Library provides various high-level tools for working with coordinate files, which include not only reading and writing, but also orthogonal-fractional coordinate transforms, generation of symmetry mates, editing the molecular structure and some others. The Library is supposed as a general low-level tool for unifying the coordinate-related operations.

This distribution is based on the last free version of mmdb, namely 1.09. Eugene's latest version of mmdb (1.10) is licensed under the non-free CCP4 license.

Also see:
http://www.ebi.ac.uk/~keb/cldoc/
http://www.bioxray.dk/~mok/mmdb.php

Comment 1 Tim Fenn 2008-05-26 23:44:38 UTC
Source RPM and spec file updated to bring RPM inline with current Fedora guidelines.

Spec URL: http://www.stanford.edu/~fenn/packs/mmdb.spec
SRPM URL: http://www.stanford.edu/~fenn/packs/mmdb-1.09.1-3.f8.src.rpm

Comment 3 Tim Fenn 2008-05-27 21:18:13 UTC
Removed static libs.

new spec: http://www.stanford.edu/~fenn/packs/mmdb.spec
new srpm: http://www.stanford.edu/~fenn/packs/mmdb-1.09.1-5.f8.src.rpm


Comment 4 Ralf Corsepius 2008-05-28 05:23:43 UTC
Similar issues as with ssm:

* 
# rpmlint mdb-1.09.1-5.fc9.x86_64.rpm 
mmdb.x86_64: E: library-without-ldconfig-postin /usr/lib64/libmmdb.so.0.0.0
mmdb.x86_64: E: library-without-ldconfig-postun /usr/lib64/libmmdb.so.0.0.0
Your main package needs ldconfig scriptlets

mmdb.x86_64: W: no-version-in-last-changelog
Please appropriate version numbers to your changelog entries in mmdb.spec

* BuildRoot doesn't comply to FPG

* mmdb-devel.x86_64: W: no-dependency-on mmdb
The *-devel package must depend upon the main-package
=> Add
Requires: mmdb = %{version}-%{release}
to *-devel

* mmdb-devel contains *.pc
=> Add
Requires: pkgconfig
to *-devel

* mmdb-devel contains binaries and temporary files (*.o, .libs) in
/usr/share/doc/mmdb-devel-1.09.1/Examples
This is doesn't work, is multiply broken and therefore is not allowed.
The easiest work-around to this is not to ship these files.

If you want to ship the sources, you'd have to remove all binaries and temporary
files from ../doc/../Examples in %install

If you want to ship the binaries (may-be accompanied by the sources), you'd have
to install the binaries elsewhere (e.g. %{libdir}/mmdb/Examples) with all
temporary files removed.


Comment 5 Tim Fenn 2008-05-29 01:40:37 UTC
(In reply to comment #4)
> Similar issues as with ssm:
> 

All comments to ssm propogated here.

> * 
> # rpmlint mdb-1.09.1-5.fc9.x86_64.rpm 
> mmdb.x86_64: E: library-without-ldconfig-postin /usr/lib64/libmmdb.so.0.0.0
> mmdb.x86_64: E: library-without-ldconfig-postun /usr/lib64/libmmdb.so.0.0.0
> Your main package needs ldconfig scriptlets
> 

Done.

> mmdb.x86_64: W: no-version-in-last-changelog
> Please appropriate version numbers to your changelog entries in mmdb.spec
> 

Done.

> * BuildRoot doesn't comply to FPG
> 

Fixed.

> * mmdb-devel.x86_64: W: no-dependency-on mmdb
> The *-devel package must depend upon the main-package
> => Add
> Requires: mmdb = %{version}-%{release}
> to *-devel
> 

Fixed.

> * mmdb-devel contains *.pc
> => Add
> Requires: pkgconfig
> to *-devel
> 

Fixed.

> * mmdb-devel contains binaries and temporary files (*.o, .libs) in
> /usr/share/doc/mmdb-devel-1.09.1/Examples
> This is doesn't work, is multiply broken and therefore is not allowed.
> The easiest work-around to this is not to ship these files.
> 
> If you want to ship the sources, you'd have to remove all binaries and temporary
> files from ../doc/../Examples in %install
> 

I added the following to %install:

pushd Examples ; make clean ; rm -rf .deps _deps ; popd

is that OK?

new spec: http://www.stanford.edu/~fenn/packs/mmdb.spec
new srpm: http://www.stanford.edu/~fenn/packs/mmdb-1.09.1-6.f8.src.rpm


Comment 6 Ralf Corsepius 2008-05-29 03:05:44 UTC
(In reply to comment #5)
Much better now.

However, one major issue remains:
# rpmlint libmmdb-1.09.1-6.fc8.x86_64.rpm
libmmdb.x86_64: E: library-without-ldconfig-postin /usr/lib64/libmmdb.so.0.0.0
libmmdb.x86_64: E: library-without-ldconfig-postun /usr/lib64/libmmdb.so.0.0.0

You renamed "main-package" into libmmdb 
=> Add "-n libmmdb" to %post and %postun

--- mmdb.spec.orig       2008-05-29 04:29:27.000000000 +0200
+++ mmdb.spec    2008-05-29 04:35:08.000000000 +0200
@@ -104,9 +104,9 @@
 
 pushd Examples ; make clean ; rm -rf .deps _deps ; popd
 
-%post -p /sbin/ldconfig
+%post -n libmmdb -p /sbin/ldconfig
 
-%postun -p /sbin/ldconfig
+%postun -n libmmdb -p /sbin/ldconfig
 
 %clean
 rm -rf %{buildroot}


> > * mmdb-devel contains binaries and temporary files (*.o, .libs) in
> > /usr/share/doc/mmdb-devel-1.09.1/Examples

> I added the following to %install:
> 
> pushd Examples ; make clean ; rm -rf .deps _deps ; popd
> 
> is that OK?
Well, it helps for now (not a review blocker), except that it breaks 
"rpmbuild -bi --short-circuit", because it removes files being generated
in %build, which will kill subsequent "-bi --short-circuit".

# rpmbuild -bb mmdb.spec
# rpmbuild -bi --short-circuit mmdb.spec
...
Making install in Examples
...
Makefile:336: .deps/coorfile.Po: No such file or directory
Makefile:337: .deps/extr_exmp1.Po: No such file or directory
Makefile:338: .deps/sel_exmp1.Po: No such file or directory
Makefile:339: .deps/sel_exmp2.Po: No such file or directory
Makefile:340: .deps/sel_exmp3.Po: No such file or directory
Makefile:341: .deps/sel_exmp4.Po: No such file or directory
Makefile:342: .deps/sel_exmp5.Po: No such file or directory
Makefile:343: .deps/sel_exmp6.Po: No such file or directory
Makefile:344: .deps/spseatoms.Po: No such file or directory
make[1]: *** No rule to make target `.deps/spseatoms.Po'.  Stop.
...
make: *** [install-recursive] Error 1
error: Bad exit status from /var/tmp/rpm-tmp.6865 (%install)

Avoiding this issue is a bit tricky. 

One way is to copy Examples/ to a different directory inside of the build-tree,
(manually) remove the temporary files there and let %doc pickup this directory
instead of the original Examples/

Another way is to modify Examples/Makefile.am, such that "make install" installs
the files you want to be installed.


I'll approve the package, once the ldconfig-fix is applied.

BTW: Are you familiar with rpmlint? Though it's far from being perfect, it helps
you avoiding many of the "obvious" packaging mistakes.


Comment 7 Tim Fenn 2008-05-30 03:42:13 UTC
(In reply to comment #6)
> (In reply to comment #5)
> Much better now.
> 
> However, one major issue remains:
> # rpmlint libmmdb-1.09.1-6.fc8.x86_64.rpm
> libmmdb.x86_64: E: library-without-ldconfig-postin /usr/lib64/libmmdb.so.0.0.0
> libmmdb.x86_64: E: library-without-ldconfig-postun /usr/lib64/libmmdb.so.0.0.0
> 
> You renamed "main-package" into libmmdb 
> => Add "-n libmmdb" to %post and %postun
> 

done.

> 
> > > * mmdb-devel contains binaries and temporary files (*.o, .libs) in
> > > /usr/share/doc/mmdb-devel-1.09.1/Examples
> 
> > I added the following to %install:
> > 
> > pushd Examples ; make clean ; rm -rf .deps _deps ; popd
> > 
> > is that OK?
> Well, it helps for now (not a review blocker), except that it breaks 
> "rpmbuild -bi --short-circuit", because it removes files being generated
> in %build, which will kill subsequent "-bi --short-circuit".
> 
> # rpmbuild -bb mmdb.spec
> # rpmbuild -bi --short-circuit mmdb.spec
> ...
> Making install in Examples
> ...
> Makefile:336: .deps/coorfile.Po: No such file or directory
> Makefile:337: .deps/extr_exmp1.Po: No such file or directory
> Makefile:338: .deps/sel_exmp1.Po: No such file or directory
> Makefile:339: .deps/sel_exmp2.Po: No such file or directory
> Makefile:340: .deps/sel_exmp3.Po: No such file or directory
> Makefile:341: .deps/sel_exmp4.Po: No such file or directory
> Makefile:342: .deps/sel_exmp5.Po: No such file or directory
> Makefile:343: .deps/sel_exmp6.Po: No such file or directory
> Makefile:344: .deps/spseatoms.Po: No such file or directory
> make[1]: *** No rule to make target `.deps/spseatoms.Po'.  Stop.
> ...
> make: *** [install-recursive] Error 1
> error: Bad exit status from /var/tmp/rpm-tmp.6865 (%install)
> 
> Avoiding this issue is a bit tricky. 
> 
> One way is to copy Examples/ to a different directory inside of the build-tree,
> (manually) remove the temporary files there and let %doc pickup this directory
> instead of the original Examples/
> 

went with this approach.

> 
> I'll approve the package, once the ldconfig-fix is applied.
> 
> BTW: Are you familiar with rpmlint? Though it's far from being perfect, it helps
> you avoiding many of the "obvious" packaging mistakes.
> 

I just installed it and have started using it (noticed it mentioned on
fedora-list, but never actually checked it out).  Thanks for the pointer!

new spec: http://www.stanford.edu/~fenn/packs/mmdb.spec
new srpm: http://www.stanford.edu/~fenn/packs/mmdb-1.09.1-7.f8.src.rpm


Comment 8 Ralf Corsepius 2008-05-30 05:16:19 UTC
One final, last minute issue:

Where is the upstream of this package?

AFAIU, the original tarballs are those in
http://www.ebi.ac.uk/~keb/cldoc/downloads/
while
ftp://ftp.bioxray.au.dk/pub/mok/src/ 
hosts repackaged tarballs w/ autotool-support having been added. 

This wouldn't be much of a problem, if the original upstream was dead, but it
apparently isn't. It released an update a couple of days ago.

The normal approach in Fedora would be to use the original upstream and to
provide patches (which may originate from different origins) against the
original upstream. Otherwise things easily tend to diverge.

What do you propose to do about this? Is bioxray actively working on this
package's sources?



Comment 9 Tim Fenn 2008-05-30 09:34:00 UTC
(In reply to comment #8)
> One final, last minute issue:
> 
> Where is the upstream of this package?
> 
> AFAIU, the original tarballs are those in
> http://www.ebi.ac.uk/~keb/cldoc/downloads/
> while
> ftp://ftp.bioxray.au.dk/pub/mok/src/ 
> hosts repackaged tarballs w/ autotool-support having been added. 
> 
> This wouldn't be much of a problem, if the original upstream was dead, but it
> apparently isn't. It released an update a couple of days ago.
> 

See:

http://www.bioxray.dk/~mok/mmdb.php

quote:
This distribution is based on the last free version of mmdb, namely 1.09.
Eugene's latest version of mmdb (1.10) is licensed under the non-free CCP4 license.

The licensing, however, seems to have switched back to the LGPL:

http://www.ccp4.ac.uk/ccp4license.php

I can bring this issue up with those involved and see what the current status
is.  My hope is that the current fork will only be temporary.


Comment 10 Ralf Corsepius 2008-05-30 14:07:02 UTC
Technically, you can consider this package approved, however ... this package
needs a legal review - Spot this pumpkin is yours.

In my understanding, this package, using the tarball Tim is using (an apparent
fork from it's original upstream), is licenced under a "LGPLv2+ with exceptions".


Comment 11 Tom "spot" Callaway 2008-05-30 15:51:13 UTC
Blocking FE-Legal until I get a chance to do a proper review of this.

Comment 12 Tom "spot" Callaway 2008-06-04 20:54:31 UTC
Well, looking at this, it looks like both the 1.09.1 fork and the 1.10 version
are under Free licenses (LGPLv2 and LGPLv3 respectively). Lifting FE-Legal.

Comment 13 Jason Tibbitts 2008-10-23 00:27:09 UTC
Hmm, something went off the tracks here.  Ralf indicated that he approves this, but never assigned it to himself, set any flags or even CC'd himself.

The package in comment #7 still builds on rawhide and at a cursory glance it seems OK; if you're still interested in getting this in then I'll go ahead and give it a full looking over and get you through the process of being sponsored and importing, building and releasing the package.

Comment 14 Tim Fenn 2008-10-23 01:35:41 UTC
(In reply to comment #13)
> Hmm, something went off the tracks here.  Ralf indicated that he approves this,
> but never assigned it to himself, set any flags or even CC'd himself.
> 
> The package in comment #7 still builds on rawhide and at a cursory glance it
> seems OK; if you're still interested in getting this in then I'll go ahead and
> give it a full looking over and get you through the process of being sponsored
> and importing, building and releasing the package.

Yes, thanks - I'd also like to finish off:

https://bugzilla.redhat.com/show_bug.cgi?id=435015
https://bugzilla.redhat.com/show_bug.cgi?id=435017
https://bugzilla.redhat.com/show_bug.cgi?id=435018

(this package blocks the latter 2)

Comment 15 Jason Tibbitts 2008-10-23 04:08:34 UTC
Well, one ticket at a time.  And once this one is done, any packager can review any of those tickets.  However, there are a few issues here:

The tarball in the source tree differs from the tarball downloaded from the Source0 URL.  In fact, the diff is almost 37000 lines long.  Any idea what's going on?

I don't understand why you explicitly %define version and release.  If you just use:
  Version: 1.09.1
  Release: 7
then %version and %release are defined for you in the same way that %name is.

I would argue that according to the COPYING file, the License: should be "LGPLv2 with exceptions" but spot above says LGPLv2.  I'll double check with spot tomorrow.

I don't think its a blocker, but the first paragraph of %description is loaded with acronyms which are completely meaningless to most people.  A little elaboration there would be welcome.

Any reason why parallel make is not being used?  You should use it if possible, but if it breaks you should include a comment to that effect.

The AUTHORS, README and COPYING files are duplicated between the packages.  There's no need to do that.

X source files do not match upstream.
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summaries are OK.
* descriptions are OK (could perhaps use some elaboration of acronyms).
* dist tag is present.
* build root is OK.
? license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* debuginfo package looks complete.
* rpmlint is silent.
* final provides and requires are sane:
  libmmdb-1.09.1-7.fc10.x86_64.rpm
   libmmdb.so.0()(64bit)
   libmmdb = 1.09.1-7.fc10
   libmmdb(x86-64) = 1.09.1-7.fc10
  =
   /sbin/ldconfig
   libgcc_s.so.1()(64bit)
   libgcc_s.so.1(GCC_3.0)(64bit)
   libmmdb.so.0()(64bit)
   libstdc++.so.6()(64bit)
   libstdc++.so.6(CXXABI_1.3)(64bit)
   libstdc++.so.6(GLIBCXX_3.4)(64bit)

  libmmdb-devel-1.09.1-7.fc10.x86_64.rpm
   libmmdb-devel = 1.09.1-7.fc10
   libmmdb-devel(x86-64) = 1.09.1-7.fc10
  =
   libmmdb = 1.09.1-7.fc10
   libmmdb.so.0()(64bit)
   pkgconfig

* %check is not present; no test suite upstream (that I could see).
* shared libraries present:
   unversioned .so links are in the -devel package
   ldconfig called properly
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files.
* scriptlets OK (ldconfig).
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headersa re in the -devel package.
* pkgconfig files in -devel package; pkgconfig dependency is present.
* no static libraries.
* no libtool .la files.

Comment 16 Tom "spot" Callaway 2008-10-23 13:15:51 UTC
(In reply to comment #15)

> I would argue that according to the COPYING file, the License: should be
> "LGPLv2 with exceptions" but spot above says LGPLv2.  I'll double check with
> spot tomorrow.

Yeah, this is right, it should be "LGPLv2 with exceptions". Not sure why I thought differently before.

Comment 17 Tim Fenn 2008-10-23 17:00:53 UTC
(In reply to comment #15)
> 
> The tarball in the source tree differs from the tarball downloaded from the
> Source0 URL.  In fact, the diff is almost 37000 lines long.  Any idea what's
> going on?
> 

Oops - I had used a .tar.gz I created from a "make build" of the original package.  Fixed.

> I don't understand why you explicitly %define version and release.  If you just
> use:
>   Version: 1.09.1
>   Release: 7
> then %version and %release are defined for you in the same way that %name is.
> 

Removed.

> I would argue that according to the COPYING file, the License: should be
> "LGPLv2 with exceptions" but spot above says LGPLv2.  I'll double check with
> spot tomorrow.
> 

Fixed.

> I don't think its a blocker, but the first paragraph of %description is loaded
> with acronyms which are completely meaningless to most people.  A little
> elaboration there would be welcome.
> 

Sorry, alot of the stuff in the spec is cruft from the original author.  Rewritten.

> Any reason why parallel make is not being used?  You should use it if possible,
> but if it breaks you should include a comment to that effect.
> 

It simply wasn't present before.  It seems to work OK, included now.

> The AUTHORS, README and COPYING files are duplicated between the packages. 
> There's no need to do that.
> 

Fixed.

new spec: http://www.stanford.edu/~fenn/packs/mmdb.spec
new srpm: http://www.stanford.edu/~fenn/packs/mmdb-1.09.1-8.f8.src.rpm

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

Comment 19 Tim Fenn 2008-10-26 00:16:34 UTC
Changed naming from libmmdb to mmdb (see https://bugzilla.redhat.com/show_bug.cgi?id=435015 and discussion therein).

new spec: http://www.stanford.edu/~fenn/packs/mmdb.spec
new srpm: http://www.stanford.edu/~fenn/packs/mmdb-1.09.1-9.f8.src.rpm

Comment 20 Jason Tibbitts 2008-10-27 02:37:19 UTC
OK, the tarball matches what I get from the upstream web site now.

rpmlint has grown a new complaint:
  mmdb.x86_64: W: shared-lib-calls-exit /usr/lib64/libmmdb.so.0.0.0 
   exit.5
I didn't see this last time, which implies that either you had fixed the two places where exit() is called in the your modified tarball or that rpmlint has grown this complaint recently.  There was an rpmlint update three days ago so that might be it.

In any case, this is a rather odd thing for a shared library to do; I'd expect abnormal exits to call abort() instead, assuming they actually need to stop program execution.  Obviously you wouldn't expect any normal exits from library code, but upstream software authors often do rather dumb things.  I don't really see this as a blocker, however; it's more something that should be reported as a bug upstream.

In any case, the issues I raised earlier are fixed, and the above isn't a blocker, I'd say we're done.

APPROVED

Comment 21 Tim Fenn 2008-10-27 20:35:01 UTC
New Package CVS Request
=======================
Package Name: mmdb
Short Description: macromolecular coordinate library
Owners: timfenn
Branches: F-9 F-10 EL-5
InitialCC: timfenn

Comment 22 Kevin Fenzi 2008-10-29 21:37:12 UTC
cvs done.

Comment 23 Tim Fenn 2008-10-30 01:59:16 UTC
tagged/built for all branches (still awaiting response from upstream re. library-calls-exit issue)

Comment 24 Mamoru TASAKA 2008-11-08 07:16:20 UTC
For now closing.


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