Bug 529466 - Review Request: linbox - C++ Library for High-Performance Linear Algebra
Summary: Review Request: linbox - C++ Library for High-Performance Linear Algebra
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Susi Lehtola
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 476299 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-10-17 01:36 UTC by Thomas Spura
Modified: 2009-10-21 21:14 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-10-21 21:14:23 UTC
Type: ---
Embargoed:
susi.lehtola: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Thomas Spura 2009-10-17 01:36:40 UTC
Spec URL: http://tomspur.fedorapeople.org/review/linbox.spec
SRPM URL: http://tomspur.fedorapeople.org/review/linbox-1.1.6-2.fc11.src.rpm
Description:
LinBox is a C++ template library for exact, high-performance linear
algebra computation with dense, sparse, and structured matrices over
the integers and over finite fields.

__________________________

$ rpmlint linbox.spec ../SRPMS/linbox-1.1.6-2.fc11.src.rpm ../RPMS/x86_64/linbox-* ../RPMS/noarch/linbox-doc-1.1.6-2.fc11.noarch.rpm 
linbox.x86_64: W: shared-lib-calls-exit /usr/lib64/liblinboxsage.so.0.0.0 exit.5
linbox.x86_64: W: shared-lib-calls-exit /usr/lib64/liblbdriver.so.0.0.0 exit.5
5 packages and 1 specfiles checked; 0 errors, 2 warnings.

Builds in koji:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1751108
__________________________

This is part of packaging sage into fedora [1].

[1]
https://fedoraproject.org/wiki/SIGs/SciTech/SAGE#Missing_required_components

__________________________

Patches are send upstream.

Comment 1 Thomas Spura 2009-10-17 01:38:23 UTC
*** Bug 476299 has been marked as a duplicate of this bug. ***

Comment 2 Susi Lehtola 2009-10-17 07:19:23 UTC
- Change
 BuildRequires:  /usr/bin/aclocal
to
 BuildRequires:  automake

- Your patches are missing comments.

- Drop INSTALL from %doc, at least
 http://www.linalg.org/linbox-html/INSTALL
doesn't contain any relevant information.

Comment 3 Thomas Spura 2009-10-17 17:45:35 UTC
Thanks for the advices.

Done in:
Spec URL: http://tomspur.fedorapeople.org/review/linbox.spec
SRPM URL: http://tomspur.fedorapeople.org/review/linbox-1.1.6-3.fc11.src.rpm

Comment 4 D Haley 2009-10-18 05:35:11 UTC
Some quick comments.

 Either (1) Add the following to the spec -- Do we need to make a separate tarball? or (2) notify upstream to have the licencing headers attached. If you do notify upstream, I note that some headers are LGPLv2+ -- would upstream be able to specify LGPLv2+ to all code?


  #Examples and tests dir contains copyrighted code, not marked with any lic header.                                        | 
  #Upstream has been notified to assist in re-licencing.                                                                    | 
  rm -f ./examples/charpoly.C ./examples/checksolve.C ./examples/det.C ./examples/doubledet.C ./examples/echelon.C \
   ./examples/fields/ex-fields-archetype.C ./examples/fields/ex-fields.C ./examples/fields/modular-int.C \
   ./examples/graph-charpoly.C ./examples/minpoly.C ./examples/rank.C ./examples/smith.C ./examples/solve.C \
   ./examples/sparseelimdet.C ./examples/sparseelimrank.C ./examples/valence.C                                       
                                                                                                                            

  rm -f ./tests/test-bitonic-sort.C ./tests/test-block-ring.C ./tests/test-companion.C ./tests/test-ffpack.C \              
   ./tests/test-hilbert.C ./tests/test-last-invariant-factor.C ./tests/test-matrix-stream.C \                               
   ./tests/test-modular-balanced-int.C ./tests/test-ntl-hankel.C ./tests/test-ntl-RR.C \                                    
   ./tests/test-ntl-sylvester.C ./tests/test-ntl-toeplitz.C ./tests/test-quad-matrix.C \                                    
   ./tests/test-rational-solver-adaptive.C ./tests/test-rational-solver.C \                                                  
   /tests/test-scalar-matrix.C ./tests/test-smith-form-adaptive.C ./tests/test-smith-form-binary.C \                        
   /tests/test-smith-form.C ./tests/test-smith-form-iliopoulos.C ./tests/test-subiterator.C \                              
   /tests/test-subvector.C ./tests/test-toeplitz-det.C ./tests/test-triplesbb.C ./tests/test-zero-one.C \                   
   /tests/test-zo.C                                                                                                         


As with givaro (bug #475065), the -config script does not emit properly if --cflags --lflags are both used. The following patch can be applied

--- linbox-config.in	2008-06-14 11:21:00.000000000 +1000
+++ linbox-config.in.new	2009-10-10 16:35:45.000000000 +1100
@@ -55,11 +55,11 @@
 	;;
 
     --cflags)
-       	echo -I${includedir} @GMP_CFLAGS@ @NTL_CFLAGS@ @GIVARO_CFLAGS@ @SACLIB_CFLAGS@ @LIDIA_CFLAGS@ 
+       	echo -n " -I${includedir} @GMP_CFLAGS@ @NTL_CFLAGS@ @GIVARO_CFLAGS@ @SACLIB_CFLAGS@ @LIDIA_CFLAGS@"
        	;;
 
     --libs)
-       	echo -L${libdir} -llinbox @LIDIA_LIBS@  @NTL_LIBS@ @GIVARO_LIBS@ @SACLIB_LIBS@ @GMP_LIBS@ @BLAS_LIBS@
+       	echo -n " -L${libdir} -llinbox @LIDIA_LIBS@  @NTL_LIBS@ @GIVARO_LIBS@ @SACLIB_LIBS@ @GMP_LIBS@ @BLAS_LIBS@"
        	;;
 
     *)
@@ -69,5 +69,6 @@
     esac
     shift
 done
+echo
 
 exit 0


On Linux spiderbox 2.6.27.37-170.2.104.fc10.i686, I am getting rpmlint errors. If you don't have access to this arch, you can use the test machines  (https://fedoraproject.org/wiki/Test_Machine_Resources_For_Package_Maintainers)

linbox.i386: E: shlib-with-non-pic-code /usr/lib/liblinboxsage.so.0.0.0
linbox.i386: W: shared-lib-calls-exit /usr/lib/liblinboxsage.so.0.0.0 exit
linbox.i386: E: shlib-with-non-pic-code /usr/lib/liblbdriver.so.0.0.0
linbox.i386: W: shared-lib-calls-exit /usr/lib/liblbdriver.so.0.0.0 exit
4 packages and 0 specfiles checked; 2 errors, 2 warnings.

Comment 5 D Haley 2009-10-18 05:38:05 UTC
Hmm. Maybe you can't use the test machines -- no 686. At any rate, I'm still getting those errors.

Comment 6 Susi Lehtola 2009-10-18 08:03:16 UTC
(In reply to comment #4)
> Some quick comments.
> 
>  Either (1) Add the following to the spec -- Do we need to make a separate
> tarball? or (2) notify upstream to have the licencing headers attached. If you
> do notify upstream, I note that some headers are LGPLv2+ -- would upstream be
> able to specify LGPLv2+ to all code?

This is really not necessary. Licensing and copyright are wholly different things. When some of the files that are included don't specify a license that they're under, it is assumed that they are under the license of COPYING. (IANAL, though.)

See the order of operations at
http://fedoraproject.org/wiki/Licensing/FAQ#How_do_I_figure_out_what_version_of_the_GPL.2FLGPL_my_package_is_under.3F

In this case the attached COPYING is a generic LGPLv2 COPYING without an author's specification of which version is to be used, thus the license tag for the files without a license statement is LGPLv2+.

However, you should still contact upstream and ask them to add the missing license headers, which makes things easier for everybody.

> linbox.i386: E: shlib-with-non-pic-code /usr/lib/liblinboxsage.so.0.0.0
> linbox.i386: W: shared-lib-calls-exit /usr/lib/liblinboxsage.so.0.0.0
> exit
> linbox.i386: E: shlib-with-non-pic-code /usr/lib/liblbdriver.so.0.0.0
> linbox.i386: W: shared-lib-calls-exit /usr/lib/liblbdriver.so.0.0.0
> exit
> 4 packages and 0 specfiles checked; 2 errors, 2 warnings.  

Calling exit happens quite a lot with scientific libraries. To fix the non-pic code error just add -fPIC to %{optflags}.

Comment 7 D Haley 2009-10-18 08:27:13 UTC
Just to clarify what I am talking about:

As an example looking at test-ring-block.C:

  1
  2
  3 /* tests/test-block-ring.C
  4  * Copyright (C) 2007 b d saunders
  5  *
  6  */
  7

That has a copyright header, and as the tests are not automated, and we are not using them, we don't need the file.

Comment 8 Susi Lehtola 2009-10-18 09:09:55 UTC
Yes, I understood what you were talking about. Still, the file doesn't specify a license different from that of COPYING, so it is assumed that the file is under the same license as the rest of linbox.

Feel free to block FE-LEGAL if you disagree. I find that removing the files is unnecessary.

**

Thomas: there are automated tests available, so add

 %check
 make -C tests check

to the spec file.

Comment 9 Thomas Spura 2009-10-18 12:11:19 UTC
I'm also assuming that the file is under the same license as the rest of linbox, if no license is specified, so I let the rm -f .... out for now.

Hmm, they are not packaged anyway. Only the tests are used now in %check, if there are licensing problems, wouldn't just remove the %check again resolve this?
If there are no licensing problems, maybe the examples directory would belong to the -doc package.
What do you think?


I just started a discussion upstream in the linbox-devel googlegroup and let you know, what (and if) happens. [1]
Also about the exit call and the patches…


%changelog
- %%check
- patch for --cflags and --lflags in config

[1] http://groups.google.com/group/linbox-devel

Spec URL: http://tomspur.fedorapeople.org/review/linbox.spec
SRPM URL: http://tomspur.fedorapeople.org/review/linbox-1.1.6-4.fc11.src.rpm

Comment 10 D Haley 2009-10-18 13:40:51 UTC
>Feel free to block FE-LEGAL if you disagree.
I'll just request that upstream be notified.

Comment 11 Susi Lehtola 2009-10-18 20:30:16 UTC
rpmlint output:
linbox.x86_64: W: shared-lib-calls-exit /usr/lib64/liblinboxsage.so.0.0.0 exit.5
linbox.x86_64: W: shared-lib-calls-exit /usr/lib64/liblbdriver.so.0.0.0 exit.5
5 packages and 0 specfiles checked; 0 errors, 2 warnings.

(Same warnings on i386, so everything is OK.)


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK

MUST: The spec file for the package is legible and macros are used consistently. NEEDSWORK
- You are still missing comments from the patches. Please add comments in the spec file to indicate what the patch does.

MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license. OK
- There are some files also in linbox/ that are missing proper license headers. Ask upstream to add them.

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A
MUST: Optflags are used and time stamps preserved. OK
MUST: Packages containing shared library files must call ldconfig. OK
MUST: A package must own all directories that it creates or require the package that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK


MUST: Large documentation files must go in a -doc subpackage. OK
- Note that
 %doc htmldocs
includes the htmldocs directory, so you currently have
 /usr/share/doc/linbox-doc-1.1.6/htmldocs/
in -doc. Instead you should use
 %doc htmldocs/*

- Instead of
 # These docs are installed to the wrong place (/usr/doc), so we move
 # them to -doc.
 mv %{buildroot}%{_prefix}/doc ./htmldocs
and in %files of -doc
 %doc htmldocs
I suggest
 # Remove docs that are installed in the wrong place
 rm -rf %{buildroot}%{_prefix}/doc
and in %files of -doc
 %doc doc/linbox.html doc/linbox-html
as this is surer to preserve time stamps.

- Also, note that
 BuildArch:      noarch
only works on Fedora >= 11. If you want to build for EPEL, use
 %if 0%{?fedora} > 10 || 0%{?rhel} > 5
  BuildArch: noarch
 %endif


MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. NEEDSWORK
- Add NEWS, README and TODO.

MUST: Header files must be in a -devel package. OK
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. OK
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. OK
MUST: Packages does not contain any .la libtool archives. OK
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK

Comment 12 Thomas Spura 2009-10-19 00:05:13 UTC
(In reply to comment #10)
> >Feel free to block FE-LEGAL if you disagree.
> I'll just request that upstream be notified.  

Answer at [1]
Patches will be in next relase and headers maybe corrected. Not done so far, not by intention…

[1] http://groups.google.com/group/linbox-devel/browse_thread/thread/6bd180a8ac29ca58


NEEDSWORK done:
- Patches have comments
- Add NEWS, README and TODO to %doc


This won't go into EPEL for now, because givario is missing there (sure, it's an option, but sage depends on it, too).


Different done:
- %doc htmldocs/* instead %doc htmldocs:
  in configure is a docdir option, but now they are installed in
  /usr/share/doc/linbox-html and not in /usr/share/doc/linbox-doc-1.1.6
  only one file /usr/share/doc/linbox.html needs to rm.

Anything should be fine in:
Spec URL: http://tomspur.fedorapeople.org/review/linbox.spec
SRPM URL: http://tomspur.fedorapeople.org/review/linbox-1.1.6-5.fc11.src.rpm

Comment 13 Susi Lehtola 2009-10-19 07:18:02 UTC
(In reply to comment #12)
> Different done:
> - %doc htmldocs/* instead %doc htmldocs:
>   in configure is a docdir option, but now they are installed in
>   /usr/share/doc/linbox-html and not in /usr/share/doc/linbox-doc-1.1.6
>   only one file /usr/share/doc/linbox.html needs to rm.

That won't do. You're breaking the naming convention in /usr/share/doc. You have to place the files under /usr/share/doc/%{name}-%{version}/ (or /usr/share/doc/%{name}-doc-%{version}/ ).

Note that linbox.html is the index page of the documentation and must not be removed.

Comment 14 Susi Lehtola 2009-10-19 07:35:27 UTC
My suggestion is still to use
 # Remove docs that are installed in the wrong place
 rm -rf %{buildroot}%{_prefix}/doc
in %install and
 %doc doc/linbox.html doc/linbox-html
in %files of -doc.

Comment 15 Thomas Spura 2009-10-19 12:03:41 UTC
(In reply to comment #13)
> (In reply to comment #12)
> > Different done:
> > - %doc htmldocs/* instead %doc htmldocs:
> >   in configure is a docdir option, but now they are installed in
> >   /usr/share/doc/linbox-html and not in /usr/share/doc/linbox-doc-1.1.6
> >   only one file /usr/share/doc/linbox.html needs to rm.
> 
> That won't do. You're breaking the naming convention in /usr/share/doc. You
> have to place the files under /usr/share/doc/%{name}-%{version}/ (or
> /usr/share/doc/%{name}-doc-%{version}/ ).

Arrg, I suspected something like this :(

> 
> Note that linbox.html is the index page of the documentation and must not be
> removed.

No, it's not, there is just a redirecting to linbox/index.html, so linbox.html isself is unecessary to me.

The rest is installed with: %doc doc/linbox-html/*

So the index file lives now at /usr/share/doc/linbox-doc-1.1.6/index.html

Spec URL: http://tomspur.fedorapeople.org/review/linbox.spec
SRPM URL: http://tomspur.fedorapeople.org/review/linbox-1.1.6-6.fc11.src.rpm

Comment 16 Susi Lehtola 2009-10-19 12:16:09 UTC
OK, everything seems fine now.

APPROVED

Comment 17 Thomas Spura 2009-10-19 12:32:52 UTC
Thanks for the review.

New Package CVS Request
=======================
Package Name: linbox
Short Description: C++ Library for High-Performance Linear Algebra
Owners: tomspur
Branches: F-11 F-12 
InitialCC:

Comment 18 Kevin Fenzi 2009-10-19 15:56:00 UTC
cvs done.

Comment 19 Fedora Update System 2009-10-19 18:11:37 UTC
linbox-1.1.6-6.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/linbox-1.1.6-6.fc11

Comment 20 Fedora Update System 2009-10-19 18:14:43 UTC
linbox-1.1.6-6.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/linbox-1.1.6-6.fc12

Comment 21 Fedora Update System 2009-10-21 00:40:54 UTC
linbox-1.1.6-6.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 22 Thomas Spura 2009-10-21 21:14:23 UTC
linbox is not shipped as an update anymore, it's pushed into f12-dist, but Fedora Update System does not close this bug.

-> Doing so.


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