Bug 470173 - Review Request: m4ri - Linear Algebra over F_2
Summary: Review Request: m4ri - Linear Algebra over F_2
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-11-06 05:11 UTC by Conrad Meyer
Modified: 2008-12-04 18:54 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-12-04 18:54:23 UTC
Type: ---
Embargoed:
j: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Conrad Meyer 2008-11-06 05:11:14 UTC
Spec URL: http://konradm.fedorapeople.org/fedora/SPECS/m4ri.spec
SRPM URL: http://konradm.fedorapeople.org/fedora/SRPMS/m4ri-20081028-1.fc9.src.rpm
Description:
M4RI is a library for fast arithmetic with dense matrices over F_2.
The name M4RI comes from the first implemented algorithm: The "Method
of the Four Russians" inversion algorithm published by Gregory Bard.
M4RI is used by the Sage mathematics software and the PolyBoRi library.

Comment 1 Jason Tibbitts 2008-11-06 18:19:17 UTC
The header files seem to be in the main package.  Is this intentional?

Comment 2 Conrad Meyer 2008-11-06 18:50:23 UTC
Er, nope. Fixing.

Comment 4 Jason Tibbitts 2008-11-08 00:14:44 UTC
I'm seeing several issues with this package.

The COPYING file contains v2 of the GPL, but the code itself does not specify a version.  Their web site indicates GPLv2+, but I do not know if the web site is a sufficient statement of intent.  According to the licensing FAQ, the web site isn't consulted about this.  Blocking FE-Legal for a ruling.

The "testsuite" directory would seem to include a test suite.  Is it possible to run it at build time?  If so, it needs to be run unless there's a compelling reason not to do so.

A shared library is installed but ldconfig is not called.  In addition, this package seems to have a rather odd library versioning convention.  The usual method is to have the library version after the ".so" but this package has it before.  I'm afraid I don't understand why it would be doing this differently than almost all other libraries.

The static library is not permitted in the -devel package.  http://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries
This is the "static libraries and shared libraries" case.

rpmlint says only:
  m4ri-devel.x86_64: W: no-documentation
which is fine.

* source files match upstream:
   6e30b50b74c72ceca431461d471e38f682d7a6ad1c2d07db28806fff1d3e30e8  
   m4ri-20081028.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
? license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper (none).
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* debuginfo package looks complete.
* rpmlint has acceptable complaints.
* final provides and requires are sane:
  m4ri-20081028-2.fc10.x86_64.rpm
   libm4ri-0.0.20081029.so()(64bit)
   m4ri = 20081028-2.fc10
   m4ri(x86-64) = 20081028-2.fc10
  =

  m4ri-devel-20081028-2.fc10.x86_64.rpm
   m4ri-static = 20081028-2.fc10
   m4ri-devel = 20081028-2.fc10
   m4ri-devel(x86-64) = 20081028-2.fc10
  =
   libm4ri-0.0.20081029.so()(64bit)
   m4ri = 20081028-2.fc10

X %check is not present but a test suite seems to exist.
X shared libraries are installed but ldconfig is not called.
* 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
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers are in the -devel package.
* no pkgconfig files.
X static libraries are in the -devel package.
* no libtool .la files.

Comment 5 Conrad Meyer 2008-11-08 00:56:46 UTC
(In reply to comment #4)
> I'm seeing several issues with this package.
> 
> The COPYING file contains v2 of the GPL, but the code itself does not specify a
> version.  Their web site indicates GPLv2+, but I do not know if the web site is
> a sufficient statement of intent.  According to the licensing FAQ, the web site
> isn't consulted about this.  Blocking FE-Legal for a ruling.

Right, I believed I marked the package GPLv2+ accordingly, but it'd be good to hear from Legal.

> The "testsuite" directory would seem to include a test suite.  Is it possible
> to run it at build time?  If so, it needs to be run unless there's a compelling
> reason not to do so.

No. Or at least when I tried running them, it didn't work. (It tries to build the tests using system libraries, without those in the buildroot. Is there any easy fix for that?)

> A shared library is installed but ldconfig is not called.  In addition, this
> package seems to have a rather odd library versioning convention.  The usual
> method is to have the library version after the ".so" but this package has it
> before.  I'm afraid I don't understand why it would be doing this differently
> than almost all other libraries.

Beats me.

> The static library is not permitted in the -devel package. 
> http://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries
> This is the "static libraries and shared libraries" case.

Alright.

> ? license field matches the actual license.
> X %check is not present but a test suite seems to exist.
> X shared libraries are installed but ldconfig is not called.
> X static libraries are in the -devel package.

Added ldconfig, moved static libraries to -static subpackage.

New spec/srpm:
http://konradm.fedorapeople.org/fedora/SPECS/m4ri.spec
http://konradm.fedorapeople.org/fedora/SRPMS/m4ri-20081028-3.fc9.src.rpm

Comment 6 Jason Tibbitts 2008-11-10 04:21:56 UTC
Note that if we only trust the code and docs in the tarball, the license is GPL+ as far as I can tell.  If we additionally consult the web site, its GPLv2+.  Unfortunately we have to be precise here.  Ultimately, clarification from upstream is the best step.  An email from them is sufficient; a fixed tarball is ideal but not necessary.  Otherwise we'll wait to see what the legal folks have to say.

The new package builds fine; rpmlint spews a no-documentation complaint about the -static package but that's nothing to worry about.

About the library versioning thing, my concern is that something built against this package will end up needing -devel installed at runtime because the linker won't understand the different versioning convention and will end up with a dependency on libm4ri.so instead of libm4ri-0.0.20081029.so.  This should be relatively easy to verify if you have some software which uses this library around to check.

Comment 7 Conrad Meyer 2008-11-10 08:39:03 UTC
(In reply to comment #6)
> Note that if we only trust the code and docs in the tarball, the license is
> GPL+ as far as I can tell.  If we additionally consult the web site, its
> GPLv2+.  Unfortunately we have to be precise here.  Ultimately, clarification
> from upstream is the best step.  An email from them is sufficient; a fixed
> tarball is ideal but not necessary.  Otherwise we'll wait to see what the legal
> folks have to say.

Is an email from them much better than the front page of their website proclaiming GPLv2+? I've sent the maintainer an email about it anyways and await a reply.

> The new package builds fine; rpmlint spews a no-documentation complaint about
> the -static package but that's nothing to worry about.
> 
> About the library versioning thing, my concern is that something built against
> this package will end up needing -devel installed at runtime because the linker
> won't understand the different versioning convention and will end up with a
> dependency on libm4ri.so instead of libm4ri-0.0.20081029.so.  This should be
> relatively easy to verify if you have some software which uses this library
> around to check.

Sorry, I don't have any software around using this library. The goal is to eventually get Sage itself packaged properly, and this is one of the subprojects it encompasses.

Comment 8 Conrad Meyer 2008-11-10 09:51:17 UTC
Alright, I got a reply from the current maintainer. He has this to say:

"""
Hi,

sorry for the mess. The software is definitely GPLv2+. The discrepancy in the 
source files is due to my inability to copy'n'paste the right legal blah. I 
can provide a new tarbar with updated legal stuff in the source files if 
needed.

About:
> In addition, this package seems to have a rather odd library versioning 
> convention.  The usual  method is to have the library version after
> the ".so" but this package has it before.  I'm afraid I don't understand why
> it would be doing this differently than almost all other libraries.

This difference is again not intentional. I though autotools would just do the 
right thing (tm) but somewhere on the line I seem to screw up setting that 
up. I will try to figure out where it went wrong and fix it somewhen in the 
near future.
"""

In other words, upstream is alive and well and is willing to work with us to get things done properly.

Comment 9 Jason Tibbitts 2008-11-10 14:04:18 UTC
My understa(In reply to comment #7)
> Is an email from them much better than the front page of their website
> proclaiming GPLv2+?

Yes, unless you have some sort of copyright statement on the web site indicating that its authorship is the same as the software.  I won't pretend to speak for the lawyers, but as I understand things the idea is to discern the intent of the copyright holders of the software.  An email, while obviously not falsifiable by any stretch of the imagination, has been deemed sufficient for this purpose.  Of course, that email must be included with the software as documentation.

> Sorry, I don't have any software around using this library.

Well, then your options are:

* Find some way to test it by linking some code against it and seeing what happens.

* Talk to someone who knows enough about the linker to answer the question.

* Wait for upstream to change things.

* Simply refuse to do one of the above and I'll return this ticket to the queue; perhaps someone will approve it over my objections.  Let me know if this is what you prefer.

Comment 10 Tom "spot" Callaway 2008-11-10 17:47:34 UTC
When no other information is available, and we have a high degree of confidence that the website is maintained/created by the copyright holder of the licensed, we can use it as licensing intent (or versioning intent).

It's always preferrable to get upstream to clarify this specifically, which you've done. Ideally, you'll want to get upstream to specifically note this in the source tarball, with proper code attribution (see: http://www.fsf.org/licensing/licenses/gpl-howto.html , search for "copying permission statement").

Lifting FE-Legal.

Comment 11 Kevin Kofler 2008-11-11 00:20:16 UTC
What will actually be required at runtime depends on what the DT_SONAME entry for the library is. If it includes the version, it's right, if it doesn't, it needs to be fixed.

Comment 12 Kevin Kofler 2008-11-11 00:22:05 UTC
Oh, and to figure it out:
readelf -d yourlibrary.so | grep SONAME
or:
eu-readelf -d yourlibrary.so | grep SONAME
(readelf is part of binutils, eu-readelf of elfutils.)

Comment 13 Kevin Kofler 2008-11-11 00:27:32 UTC
As for some programs to test the library with, try running make in its testsuite folder. Then use readelf -d test_multiplication.c | grep NEEDED (or eu-readelf) to check what it's requiring.

Comment 14 Conrad Meyer 2008-11-11 00:44:07 UTC
For example:

$ readelf -d test_multiplication | grep NEEDED
 0x0000000000000001 (NEEDED)             Shared library: [libm4ri-0.0.20081029.so]
$ readelf -d /usr/lib64/libm4ri-0.0.20081029.so |grep SONAME
 0x000000000000000e (SONAME)             Library soname: [libm4ri-0.0.20081029.so]


On my x86_64 system.

Comment 15 Kevin Kofler 2008-11-11 00:49:39 UTC
FE-Legal was already lifted by spot, why did you readd it?

Comment 16 Kevin Kofler 2008-11-11 00:50:40 UTC
The output from comment #14 looks OK to me, the versioning system is working properly.

Comment 17 Conrad Meyer 2008-11-11 00:52:53 UTC
Did I re-add it? I didn't intend to.

Comment 18 Jason Tibbitts 2008-11-27 04:15:39 UTC
Did I miss a comment from you which answered my question?  I was going to set needinfo to you.

Comment 19 Kevin Kofler 2008-11-27 04:18:50 UTC
What question? The library versioning issue? Is the evidence provided by Conrad and me not enough to prove that it's working fine?

Comment 20 Jason Tibbitts 2008-12-02 15:34:44 UTC
I don't know; I'm not going to pretend that I've understood what comment #14 means, and I haven't seen an explicit comment from the submitter of this ticket directly answering the issue I raised in the initial review regarding whether or not the -devel package ends up being required at runtime.  All I got was "Beats me" and then several comments regarding DT_SONAME which I might research more deeply if I didn't have 30 other review tickets in flight at the moment.

Comment 21 Conrad Meyer 2008-12-02 16:31:05 UTC
The -devel package does not end up being required at runtime. The .so required at runtime is the one in %{_libdir} and the above comments agree.

Comment 22 Jason Tibbitts 2008-12-02 23:20:33 UTC
OK, in that case I think this is fine.  It would still be good to include that email from upstream with the license information as documentation, but you can do that at your leisure.

APPROVED

Comment 23 Conrad Meyer 2008-12-02 23:32:18 UTC
I'll include the email before import. Thanks very much for the review!

New Package CVS Request
=======================
Package Name: m4ri
Short Description: Linear Algebra over F_2
Owners: konradm
Branches: F-10 F-9
InitialCC:

Comment 24 Kevin Fenzi 2008-12-04 01:00:28 UTC
cvs done.

Comment 25 Conrad Meyer 2008-12-04 18:54:23 UTC
http://koji.fedoraproject.org/koji/taskinfo?taskID=977087

Built for rawhide. Closing.


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