Bug 558061 - Review Request: levmar - Levenberg-Marquardt nonlinear least squares optimization
Summary: Review Request: levmar - Levenberg-Marquardt nonlinear least squares optimiza...
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:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-01-23 12:01 UTC by Eric Smith
Modified: 2012-03-28 20:39 UTC (History)
3 users (show)

Fixed In Version: levmar-2.5-6.el6
Clone Of:
Environment:
Last Closed: 2010-02-02 01:15:03 UTC
Type: ---
Embargoed:
susi.lehtola: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Eric Smith 2010-01-23 12:01:09 UTC
This is my first package, so I'm seeking a sponsor.

Spec URL: http://www.brouhaha.com/~eric/software/fedora/f12/levmar/levmar.spec
SRPM URL: http://www.brouhaha.com/~eric/software/fedora/f12/levmar/levmar-2.5-1.fc12.src.rpm
Description: 
levmar is a native ANSI C implementation of the Levenberg-Marquardt
optimization algorithm.  Both unconstrained and constrained (under linear
equations, inequality and box constraints) Levenberg-Marquardt variants are
included.  The LM algorithm is an iterative technique that finds a local
minimum of a function that is expressed as the sum of squares of nonlinear
functions.  It has become a standard technique for nonlinear least-squares
problems and can be thought of as a combination of steepest descent and the
Gauss-Newton method.  When the current solution is far from the correct on,
the algorithm behaves like a steepest descent method: slow, but guaranteed
to converge.  When the current solution is close to the correct solution, it
becomes a Gauss-newton method.

Comment 1 Susi Lehtola 2010-01-24 00:39:24 UTC
Hi,

go through the Fedora guidelines, most importantly
 http://fedoraproject.org/wiki/Packaging/Guidelines
 http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
Additionally to the Packaging Guidelines, there are a bunch of language / application specific guidelines that are linked to in the Packaging Guidelines.

Here are some tricks of the trade:
http://fedoraproject.org/wiki/Packaging_tricks
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
http://fedoraproject.org/wiki/Common_Rpmlint_issues

***

A few notes:

- Drop the explicit requires, they're not necessary (and are in fact forbidden by the guidelines).

- Do the conversion
 dos2unix README.txt
in %prep (after the %setup line), not in %build.

- Define macros for the major and minor revision, i.e.
 %global major 2
 %global minor 2
and use for example
 ln -s liblevmar.so.%{major}.%{minor} %{buildroot}%{_libdir}/liblevmar.so.%{major}

- I suggest using the install command: instead of
 mkdir -p %{buildroot}%{_includedir} %{buildroot}%{_libdir} %{buildroot}%{_bindir}
 cp levmar.h %{buildroot}%{_includedir}
 cp sobj/liblevmar.so.2.2 %{buildroot}%{_libdir}
 cp lmdemo %{buildroot}%{_bindir}
you can just run
 install -D -p -m 755 sobj/liblevmar.so.%{major}.%{minor} %{buildroot}%{_libdir}/liblevmar.so.%{major}.%{minor}
 install -D -p -m 644 levmar.h %{buildroot}%{_includedir}/levmar.h
 install -D -p -m 755 lmdemo %{buildroot}%{_bindir}/lmdemo
which creates the necessary directores automatically, sets the correct permissions and also preserves time stamps (although this is not strictly necessary).

- Normally subpackages are declared at the beginning of the spec file, after the %description. Please move the declaration of the -devel package to the beginning (just before %prep).

- The -devel requirement should be
 Requires:	liblevmar = %{version}-%{release}

- I'm not really sure if the demo should be in the main package; I think it would make more sense in -devel.

Comment 2 Eric Smith 2010-01-24 01:20:29 UTC
Hi!  Thanks for the great advice!  I'd read the packaging requirements but obviously managed to overlook a few things.  I've updated the spec and SRPM based on your comments:

Spec URL: http://www.brouhaha.com/~eric/software/fedora/f12/levmar/levmar.spec
SRPM URL:
http://www.brouhaha.com/~eric/software/fedora/f12/levmar/levmar-2.5-2.fc12.src.rpm

Comment 3 Eric Smith 2010-01-24 02:42:05 UTC
Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1940699

Note to potential sponsors:  I've done an unofficial package review of ByAIML, bug #557948.

Comment 4 Susi Lehtola 2010-01-24 09:27:50 UTC
I'm a sponsor, and will take this review.

Comment 5 Susi Lehtola 2010-01-24 10:01:31 UTC
levmar.x86_64: W: shared-lib-calls-exit /usr/lib64/liblevmar.so.2.2 exit.5
levmar-debuginfo.x86_64: E: debuginfo-without-sources
levmar-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 1 errors, 2 warnings.

The first warning is just a sign of unrefined coding (quite normal in scientific packages). The third one is OK.

The second one, however, needs to be fixed. The build is not using the Fedora optimization flags:

gcc -fPIC -ULINSOLVERS_RETAIN_MEMORY  -O3 -funroll-loops -Wall  -c lm.c -o sobj/lm.o
gcc -fPIC -ULINSOLVERS_RETAIN_MEMORY  -O3 -funroll-loops -Wall  -c Axb.c -o sobj/Axb.o
gcc -fPIC -ULINSOLVERS_RETAIN_MEMORY  -O3 -funroll-loops -Wall  -c misc.c -o sobj/misc.o
gcc -fPIC -ULINSOLVERS_RETAIN_MEMORY  -O3 -funroll-loops -Wall  -c lmlec.c -o sobj/lmlec.o
gcc -fPIC -ULINSOLVERS_RETAIN_MEMORY  -O3 -funroll-loops -Wall  -c lmbc.c -o sobj/lmbc.o
gcc -fPIC -ULINSOLVERS_RETAIN_MEMORY  -O3 -funroll-loops -Wall  -c lmblec.c -o sobj/lmblec.o
gcc -fPIC -ULINSOLVERS_RETAIN_MEMORY  -O3 -funroll-loops -Wall  -c lmbleic.c -o sobj/lmbleic.o
gcc -fPIC -ULINSOLVERS_RETAIN_MEMORY  -O3 -funroll-loops -Wall    -c -o lmdemo.o lmdemo.c
gcc -shared -Wl,-soname,liblevmar.so.2 -o sobj/liblevmar.so.2.2 sobj/lm.o sobj/Axb.o sobj/misc.o sobj/lmlec.o sobj/lmbc.o sobj/lmblec.o sobj/lmbleic.o #-llapack -lblas -lf2c
ln -s liblevmar.so.2.2 sobj/liblevmar.so
gcc  lmdemo.o -o lmdemo -Lsobj -llevmar -llapack -lblas -lf2c  -lm -u MAIN__

Just run
 make CFLAGS="%{optflags} -funroll-loops -fPIC" -f Makefile.so %{?_smp_mflags}
to use the correct optimization flags.

**

You don't need f2c, it's not used anywhere (LAPACK is compiled with gfortran in Fedora). Drop the requirement from the spec file and drop the linking command from the Makefile.

**

I'd move the definitions of major and minor to the top of the spec file, and add a comment e.g.

 # SOlib major and minor version
 %global major 2
 %global minor 2

**

Use %{version} on the Source-line:
Source0: http://www.ics.forth.gr/~lourakis/levmar/levmar-%{version}.tgz

**

Your patch is missing its comment from the spec file:
 # Patch to fix compilation of the shared library and compile the demo program
 Patch0: levmar-makefile.patch

Patches aren't normally compressed in Fedora, as they will be stored in CVS which is only reasonable for text files. Don't compress patch0.

**

More than half of the patch is junk:
diff -uNr levmar-2.5.orig/Makefile.so~ levmar-2.5/Makefile.so~

I suggest you create patches with gendiff
 $ cd levmar-2.5
 $ cp -a Makefile{,.orig}
 (edit Makefile to suit your needs)
 $ cd ..
 $ gendiff levmar-2.5 .orig > levmar-2.5-makefile.patch

which doesn't give you such complications.

**

I'm not sure what -P 0 does in the %patch, as it's not documented on the patch man page. I believe you can drop it.

You may want to add "-b .makefile" to the patch line, though, so that patch creates backups before applying the patch. If you need to refine your patches later on, then you just edit the patched file and create a new patch with gendiff.

Comment 6 Susi Lehtola 2010-01-24 10:03:36 UTC
Oh and one more thing: you're using ${RPM_BUILD_ROOT} in %clean, but elsewhere %{buildroot}. Mixing of styles ${RPM_BUILD_ROOT} & ${RPM_OPT_FLAGS} vs. %{buildroot} & %{optflags} is forbidden in Fedora.

Please be consistent and use only %{buildroot}.

Comment 7 Susi Lehtola 2010-01-24 10:25:31 UTC
Convince me that you know the packaging guidelines by doing another package submission and another informal review (go through what you missed in bug #557948), and I will sponsor you.

Please review only packages *not* marked with FE-NEEDSPONSOR. I will have to do the full formal review after you to check that you have got everything correctly. Once I have sponsored you you will be able to do formal reviews of your own.

Comment 8 Eric Smith 2010-01-25 04:40:20 UTC
Thanks for all of the advice, and the sponsorship offer.

"-P 0" selects patch 0; earlier I had multiple patches, but the others proved to be unnecessary.  I've removed it, and made the other changes you've recommended.  Here are the updated spec and SPRM:

Spec URL: http://www.brouhaha.com/~eric/software/fedora/f12/levmar/levmar.spec
SRPM URL:
http://www.brouhaha.com/~eric/software/fedora/f12/levmar/levmar-2.5-3.fc12.src.rpm    
Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1942076

Comment 9 Susi Lehtola 2010-01-25 07:15:03 UTC
(In reply to comment #8)
> "-P 0" selects patch 0; earlier I had multiple patches, but the others proved
> to be unnecessary.  I've removed it, and made the other changes you've
> recommended.  Here are the updated spec and SPRM:

Oh, right. But you can also do it with just 
 %patch0

You're losing the time stamp of README.txt in the conversion. Add the -k switch to the dos2unix command, or use the sed command in

http://fedoraproject.org/wiki/Packaging_tricks#Remove_DOS_line_endings

Comment 10 Susi Lehtola 2010-01-25 07:18:05 UTC
I'll do the full review once you have made another submission and another informal review.

If you're looking for something to package, you could find e.g. PRNG useful, which doesn't seem to be currently in Fedora.
 http://statmath.wu.ac.at/prng/

Comment 11 Eric Smith 2010-01-27 08:06:45 UTC
I've submitted an RPM of lcdtest. The review request is bug #559117.

Comment 12 Susi Lehtola 2010-01-28 19:28:48 UTC
rpmlint output:
levmar.x86_64: W: shared-lib-calls-exit /usr/lib64/liblevmar.so.2.2 exit.5
levmar-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 2 warnings.

These are OK. However, by installing the library and running rpmlint on it I get


levmar.x86_64: W: shared-lib-calls-exit /usr/lib64/liblevmar.so.2.2 exit.5
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 spotf2_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 sqrt
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 spotrs_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 sgeqrf_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 dgemm_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 dgeqp3_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 ssytrs_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 sqrtf
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 dtrtrs_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 sgeqp3_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 dpotf2_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 dtrtri_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 dgetrf_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 sgesvd_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 ssytrf_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 sgemm_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 sgetrs_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 dorgqr_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 spotrf_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 sgetrf_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 dpotrf_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 dsytrf_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 dsytrs_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 dgeqrf_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 pow
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 strtrs_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 dpotrs_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 strtri_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 sorgqr_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 dgesvd_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 log10
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 dgetrs_
levmar.x86_64: W: undefined-non-weak-symbol /usr/lib64/liblevmar.so.2.2 log10f
1 packages and 0 specfiles checked; 0 errors, 34 warnings.

so it actually seems that -lm -llapack is missing from the library link command.

As the library itself uses LAPACK, I recommend that you change
 BuildRequires: lapack-devel
to
 BuildRequires: atlas-devel
the library from "-llapack" to "-L%{_libdir}/atlas -llapack" and the blas library from "-lblas" to "-L%{_libdir}/atlas -lf77blas -latlas" since ATLAS is a lot faster than reference BLAS and LAPACK.


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. OK
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
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. NEEDSWORK
- Time stamp is lost in %prep phase.

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. N/A
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
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. N/A
MUST: Desktop files are installed properly. N/A

MUST: No file conflicts with other packages and no general names. ~OK
- I don't know about "lmdemo", I really would name it to "levmardemo".
- Currently there doesn't seem to be any package providing /usr/bin/lmdemo. Still, I'd contact upstream to ask for a rename in the next release.

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 13 Eric Smith 2010-01-28 20:57:02 UTC
I'm reluctant to replace lapack with Atlas due to my lack of experience with the latter.  I'd rather stick with lapack for now, and consider switching to Atlas at some later date.  (I'll be more enthusiastic about switching if anyone using the Fedora levmar package requests the switch, and even more so if they try it and report that it works well and is noticably faster for their application.)

I found and fixed two more problems in the upstream Makefile.so and have corrected them in my patch.  I have notified the upstream author and provided the patch, a description of all of the problems I've found in Makefile.so, and also about adding building the lmdemo program to it, and mentioning your request to change the name lmdemo to levmardemo.

Here's the updated package that fixes the link and preserves the README timestamp.

Spec URL: http://www.brouhaha.com/~eric/software/fedora/f12/levmar/levmar.spec
SRPM URL:
http://www.brouhaha.com/~eric/software/fedora/f12/levmar/levmar-2.5-4.fc12.src.rpm 
Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1950603

Comment 14 Susi Lehtola 2010-01-28 21:32:11 UTC
(In reply to comment #13)
> I'm reluctant to replace lapack with Atlas due to my lack of experience with
> the latter.  I'd rather stick with lapack for now, and consider switching to
> Atlas at some later date.  (I'll be more enthusiastic about switching if anyone
> using the Fedora levmar package requests the switch, and even more so if they
> try it and report that it works well and is noticably faster for their
> application.)

ATLAS is just another implementation of the same routines (and it actually uses LAPACK itself for many things). For instance in Octave we have just changed to ATLAS, which results in speedups of an order of magnitude or so. So there really is a huge difference.

Comment 15 Susi Lehtola 2010-01-28 21:55:10 UTC
You can change to atlas by changing lapack-devel -> atlas-devel and defining LAPACKLIBS="-L%{_libdir}/atlas -llapack" in the make.

**

All issues have been fixed, so this package is

APPROVED

Comment 16 Eric Smith 2010-01-28 22:47:01 UTC
New Package CVS Request
=======================
Package Name: levmar
Short Description: Levenberg-Marquardt nonlinear least squares algorithm
Owners: brouhaha
Branches: F-12
InitialCC:

Comment 17 Kevin Fenzi 2010-01-31 18:55:56 UTC
CVS done (by process-cvs-requests.py).

Comment 18 Fedora Update System 2010-02-01 01:35:43 UTC
levmar-2.5-4.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/levmar-2.5-4.fc12

Comment 19 Fedora Update System 2010-02-02 01:14:57 UTC
levmar-2.5-4.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 20 Eric Smith 2012-03-11 07:24:36 UTC
Package Change Request
======================
Package Name: levmar
New Branches: el6
Owners: brouhaha

Comment 21 Gwyn Ciesla 2012-03-12 12:01:15 UTC
Git done (by process-git-requests).

Comment 22 Fedora Update System 2012-03-12 19:21:06 UTC
levmar-2.5-6.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/levmar-2.5-6.el6

Comment 23 Fedora Update System 2012-03-28 20:39:50 UTC
levmar-2.5-6.el6 has been pushed to the Fedora EPEL 6 stable repository.


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