Bug 720998 (OpenNL)

Summary: Review Request: OpenNL - A library for solving sparse linear systems
Product: [Fedora] Fedora Reporter: Ankur Sinha (FranciscoD) <sanjay.ankur>
Component: Package ReviewAssignee: Richard Shaw <hobbes1069>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: hobbes1069, notting, package-review
Target Milestone: ---Flags: hobbes1069: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: OpenNL-3.2.1-5.fc15 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-08-03 22:54:51 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 721112    

Description Ankur Sinha (FranciscoD) 2011-07-13 14:11:50 UTC
Spec URL: http://ankursinha.fedorapeople.org/opennl/OpenNL.spec
SRPM URL: http://ankursinha.fedorapeople.org/opennl/OpenNL-3.2.1-1.fc15.src.rpm
Description: 
OpenNL (Open Numerical Library) is a library for solving sparse linear systems,
especially designed for the Computer Graphics community. The goal for OpenNL is
to be as small as possible, while offering the subset of functionalities
required by this application field. The Makefiles of OpenNL can generate a
single .c + .h file, very easy to integrate in other projects. The distribution
includes an implementation of our Least Squares Conformal Maps parameterization
method.

New version: OpenNL 3.2.1, includes support for CUDA and Fermi architecture
(Concurrent Number Cruncher and Nathan Bell's ELL formats)


OpenNL offers the following set of functionalities:

    Efficient sparse matrix data structure (for non-symmetric and symmetric
matrices)
    Iterative builder for sparse linear system
    Iterative builder for sparse least-squares problems
    Iterative solvers: conjugate gradient, BICGStab, GMRES
    Preconditionners: Jacobi, SSOR
    Iterative solver on the GPU (Concurrent Number Cruncher and Nathan Bell's
ELL)
    Sparse direct solvers: OpenNL can be linked with SuperLU
    Simple demo program with LSCM (Least Squares Conformal Maps)


=========================================================================

[ankur@ankur SRPMS]$ rpmlint  -i /var/lib/mock/fedora-rawhide-i386/result/*.rpm
OpenNL.i686: W: spelling-error %description -l en_US functionalities -> functionalists, functionality, functionalist
The value of this tag appears to be misspelled. Please double-check.

OpenNL.i686: W: spelling-error %description -l en_US parameterization -> characterization, computerization, polymerization
The value of this tag appears to be misspelled. Please double-check.

OpenNL.src: W: spelling-error %description -l en_US functionalities -> functionalists, functionality, functionalist
The value of this tag appears to be misspelled. Please double-check.

OpenNL.src: W: spelling-error %description -l en_US parameterization -> characterization, computerization, polymerization
The value of this tag appears to be misspelled. Please double-check.

OpenNL-devel.i686: E: zero-length /usr/share/doc/OpenNL-devel-3.2.1/examples/DATA/example_2.mtx
OpenNL-devel.i686: E: zero-length /usr/share/doc/OpenNL-devel-3.2.1/examples/DATA/example_1.mtx
4 packages and 0 specfiles checked; 2 errors, 4 warnings.

Empty files are for the examples.

Comment 1 Ankur Sinha (FranciscoD) 2011-07-14 14:41:18 UTC
Updated spec/srpm:

http://ankursinha.fedorapeople.org/opennl/OpenNL-3.2.1-2.fc15.src.rpm

http://ankursinha.fedorapeople.org/opennl/OpenNL.spec

* Thu Jul 14 2011 Ankur Sinha <ankursinha AT fedoraproject DOT org> - 3.2.1-2
- add patch to correct libname and versioning (courtesy of Richard Shaw)


Thanks!
Ankur

Comment 2 Richard Shaw 2011-07-14 15:53:22 UTC
Hey,

Just some quick questions about the spec file.

I'm pretty sure my patch update had cmake creating the correct library symlinks. Is there a particular reason you want to create them manually? Also, since you've hardcoded the names in %install and %files, you'll have to fix this on every version change. 

Other than that, everything looks good. I'm pulling the srpm now for the usual checks.

Richard

Comment 3 Ankur Sinha (FranciscoD) 2011-07-14 16:55:42 UTC
(In reply to comment #2)
> Hey,
> 
> Just some quick questions about the spec file.
> 
> I'm pretty sure my patch update had cmake creating the correct library
> symlinks. Is there a particular reason you want to create them manually? Also,
> since you've hardcoded the names in %install and %files, you'll have to fix
> this on every version change. 

Really?

With this in the spec:

--------------------------------------------------------------

# Install includes
install -d $RPM_BUILD_ROOT/%{_includedir}/NL/
cp -av src/NL/nl.h $RPM_BUILD_ROOT/%{_includedir}/
find src/NL/ -name "*.h" ! -name "nl.h" -execdir cp -av '{}' $RPM_BUILD_ROOT/%{_includedir}/NL/ \;

# Create the .so symlinks
#pushd $RPM_BUILD_ROOT/%{_libdir}
#    ln -sfv libopennl.so.3.2.1 libopennl.so
#    ln -sfv libopennl.so.3.2.1 libopennl.so.3
#popd

%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig

%files
%doc doc 
#%{_libdir}/libopennl.so.3.2.1
#%{_libdir}/libopennl.so.3

%files devel
%doc examples
#%{_libdir}/libopennl.so
%{_includedir}/*

-----------------------------------------------------------

mock fails saying this:

DEBUG: Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests) <= 4.6.0-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
DEBUG: Processing files: OpenNL-debuginfo-3.2.1-2.fc16.i686
DEBUG: Checking for unpackaged file(s): /usr/lib/rpm/check-files /builddir/build/BUILDROOT/OpenNL-3.2.1-2.fc16.i386
DEBUG: error: Installed (but unpackaged) file(s) found:
DEBUG:    /usr/lib/libopennl.so.3.2.1
DEBUG:     Installed (but unpackaged) file(s) found:
DEBUG:    /usr/lib/libopennl.so.3.2.1
DEBUG: RPM build errors:

--------------------------------------------------------------

which is why I created the symlinks manually. rpmlint also spewed the "no shared library symlink" error. Can you please recheck once? I don't think it generated the symlinks. 

I can replace use macros in the spec. I'll go do that :)

> 
> Other than that, everything looks good. I'm pulling the srpm now for the usual
> checks.
> 
> Richard

Comment 4 Ankur Sinha (FranciscoD) 2011-07-14 17:04:18 UTC
http://ankursinha.fedorapeople.org/opennl/OpenNL-3.2.1-3.fc15.src.rpm

http://ankursinha.fedorapeople.org/opennl/OpenNL.spec

* Thu Jul 14 2011 Ankur Sinha <ankursinha AT fedoraproject DOT org> - 3.2.1-3
- add version macros to soname etc.

Comment 5 Richard Shaw 2011-07-14 17:51:40 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > Hey,
> > 
> > Just some quick questions about the spec file.
> > 
> > I'm pretty sure my patch update had cmake creating the correct library
> > symlinks. Is there a particular reason you want to create them manually? Also,
> > since you've hardcoded the names in %install and %files, you'll have to fix
> > this on every version change. 
> 
> Really?

Yup! But I was concentrating so much on the build/cmake side I forgot to update the install side. My Fault!


> With this in the spec:
> 
> --------------------------------------------------------------
> 
> # Install includes
> install -d $RPM_BUILD_ROOT/%{_includedir}/NL/
> cp -av src/NL/nl.h $RPM_BUILD_ROOT/%{_includedir}/
> find src/NL/ -name "*.h" ! -name "nl.h" -execdir cp -av '{}'
> $RPM_BUILD_ROOT/%{_includedir}/NL/ \;
> 
> # Create the .so symlinks
> #pushd $RPM_BUILD_ROOT/%{_libdir}
> #    ln -sfv libopennl.so.3.2.1 libopennl.so
> #    ln -sfv libopennl.so.3.2.1 libopennl.so.3
> #popd
> 
> %post -p /sbin/ldconfig
> %postun -p /sbin/ldconfig
> 
> %files
> %doc doc 
> #%{_libdir}/libopennl.so.3.2.1
> #%{_libdir}/libopennl.so.3

They're not commented out in mine but I instead used one line:
%{_libdir}/libopennl.so.*

That way it picks up all the versioned libraries and leaves the .so for the     -devel package.

To fix "%install" I first need to understand why you're using the "install" command. Generally mkdir and cp (with appropriate option, -p, -a, -r, etc.) is sufficient. In my spec files (and many others I've learned from) install is generally used to correct things like permissions. Cmake *SHOULD* create libraries with correct permissions. If not, rpmlint will complain about it. That being said I think the library portion of %install could look like this:

"""
%install
# Install library files
mkdir -p $RPM_BUILD_ROOT/%{_libdir}
cp -p build/linux-Release/binaries/lib/libopennl.so* $RPM_BUILD_ROOT/%{_libdir}/
"""

Which will copy the library and symlinks (instead of just the library, which is what I forgot to fix). Also all the above changes will make all the global variables at the top of the spec unnecessary.

Comment 6 Ankur Sinha (FranciscoD) 2011-07-15 13:37:56 UTC
Hi Richard,

Not sure what's going on here:

# Mock says this during the build:
# DEBUG: *** WARNING: identical binaries are copied, not linked:
# DEBUG:         /usr/lib/libopennl.so.3.2.1
# DEBUG:    and  /usr/lib/libopennl.so.3

# DEBUG: *** WARNING: identical binaries are copied, not linked:
# DEBUG:         /usr/lib/libopennl.so
# DEBUG:    and  /usr/lib/libopennl.so.3.2.1
# Manually creating a symlink

So, instead of symlinking, it's creating new files? 

Thanks,
Ankur

Comment 7 Richard Shaw 2011-07-15 14:29:49 UTC
Ack! I've been spoiled by "make install" :)

I've fixed it and I'm doing a mock build now to make sure. Due to the symbolic links the "cp" command needed "-a" not "-p". It was copying the target of the symlink (the real library) instead of copying the symlink itself.

Ok, done with the mock build and unfortunately cmake does not create the library with the correct permissions (0775 instead of 0755). There may be a way to fix that during the build but the only reference I could find to PERMISSIONS was during "install" and of course we are manually installing.

Also, since we can't copy them over all at once I had to use some 'find' mojo. 

I know in some ways this seems like more work than what you were doing (and it may well be) but it's generally better not to rely on macros and hard coded paths whenever practical. Although in this particular case you were pretty sure this was the last version it's best to try and keep things automatic as possible. I'm pretty sure if there was a new patch release you could just upload the new source, change the version in the spec file, and rebuild. 

Double Ack! They don't include the patch level in the version for the source archive file name... Oh well. I think we're close enough :)

Here's the spec file with the fixes for the library install:

http://hobbes1069.fedorapeople.org/OpenNL.spec

Comment 8 Ankur Sinha (FranciscoD) 2011-07-15 15:12:51 UTC
(In reply to comment #7)
> Ack! I've been spoiled by "make install" :)
> 
> I've fixed it and I'm doing a mock build now to make sure. Due to the symbolic
> links the "cp" command needed "-a" not "-p". It was copying the target of the
> symlink (the real library) instead of copying the symlink itself.
> 
> Ok, done with the mock build and unfortunately cmake does not create the
> library with the correct permissions (0775 instead of 0755). There may be a way
> to fix that during the build but the only reference I could find to PERMISSIONS
> was during "install" and of course we are manually installing.
> 
> Also, since we can't copy them over all at once I had to use some 'find' mojo. 
> 
> I know in some ways this seems like more work than what you were doing (and it
> may well be) but it's generally better not to rely on macros and hard coded
> paths whenever practical. Although in this particular case you were pretty sure
> this was the last version it's best to try and keep things automatic as
> possible. I'm pretty sure if there was a new patch release you could just
> upload the new source, change the version in the spec file, and rebuild. 
> 
> Double Ack! They don't include the patch level in the version for the source
> archive file name... Oh well. I think we're close enough :)
> 
> Here's the spec file with the fixes for the library install:
> 
> http://hobbes1069.fedorapeople.org/OpenNL.spec



Corrected/modified

http://ankursinha.fedorapeople.org/opennl/OpenNL-3.2.1-4.fc15.src.rpm

http://ankursinha.fedorapeople.org/opennl/OpenNL.spec

Now, *everything* finally looks okay :P

Thanks Richard,
Ankur

Comment 9 Richard Shaw 2011-07-15 18:38:16 UTC
+: OK
-: must be fixed
=: should be fixed (at your discretion)
?: Question or clairification needed
N: not applicable

MUST:
[+] rpmlint output: shown in comment: No major issues.
[+] follows package naming guidelines
[+] spec file base name matches package name
[+] package meets the packaging guidelines
[+] package uses a Fedora approved license: BSD
[+] license field matches the actual license.
[+] license file is included in %doc
[+] spec file is in American English
[+] spec file is legible
[+] sources match upstream: md5sum matches (6e182f15bf9bc8ffe95547c1cdd7e7b4)
[+] package builds on at least one primary arch: Tested F14 x86_64
[N] appropriate use of ExcludeArch
[+] all build requirements in BuildRequires
[N] spec file handles locales properly
[+] ldconfig in %post and %postun
[+] no bundled copies of system libraries
[N] no relocatable packages
[+] package owns all directories that it creates
[+] no files listed twice in %files
[+] proper permissions on files
[+] consistent use of macros
[+] code or permissible content
[N] large documentation in -doc
[+] no runtime dependencies in %doc
[+] header files in -devel
[N] static libraries in -static
[+] .so in -devel
[+] -devel requires main package
[+] package contains no libtool archives
[N] package contains a desktop file, uses desktop-file-install/validate
[+] package does not own files/dirs owned by other packages
[+] all filenames in UTF-8

SHOULD:
[+] query upstream for license text
[N] description and summary contains available translations
[+] package builds in mock
[+] package builds on all supported arches
[?] package functions as described
[+] sane scriptlets
[+] subpackages require the main package
[N] placement of pkgconfig files
[N] file dependencies versus package dependencies
[N] package contains man pages for binaries/scripts

Ok, it's not a big deal but the only thing I would change is:

%doc doc

to

%doc doc/*

Right now documentation is going into:

/usr/share/doc/OpenNL-3.2.1/doc

which is redundant...

Let me know what you think!

Richard

Comment 10 Ankur Sinha (FranciscoD) 2011-07-17 14:00:29 UTC
Hi Richard, 

I've fixed the doc macro usage.

Freshly baked packages:

http://ankursinha.fedorapeople.org/opennl/OpenNL.spec

http://ankursinha.fedorapeople.org/opennl/OpenNL-3.2.1-5.fc15.src.rpm

Thanks!
Ankur

Comment 11 Richard Shaw 2011-07-17 14:28:57 UTC
Looks good to me!

*** APPROVED ***

Comment 12 Ankur Sinha (FranciscoD) 2011-07-17 16:53:13 UTC
Thank you for the review Richard! :D

Comment 13 Ankur Sinha (FranciscoD) 2011-07-17 16:54:18 UTC
New Package SCM Request
=======================
Package Name: OpenNL
Short Description: A library for solving sparse linear systems
Owners: ankursinha
Branches: f14 f15
InitialCC: susmit mrceresa

Comment 14 Gwyn Ciesla 2011-07-17 17:17:05 UTC
Git done (by process-git-requests).

Comment 15 Fedora Update System 2011-07-17 17:56:09 UTC
OpenNL-3.2.1-5.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/OpenNL-3.2.1-5.fc14

Comment 16 Fedora Update System 2011-07-17 17:56:18 UTC
OpenNL-3.2.1-5.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/OpenNL-3.2.1-5.fc15

Comment 17 Fedora Update System 2011-07-18 22:35:26 UTC
OpenNL-3.2.1-5.fc15 has been pushed to the Fedora 15 testing repository.

Comment 18 Fedora Update System 2011-08-03 22:54:45 UTC
OpenNL-3.2.1-5.fc14 has been pushed to the Fedora 14 stable repository.

Comment 19 Fedora Update System 2011-08-03 22:55:58 UTC
OpenNL-3.2.1-5.fc15 has been pushed to the Fedora 15 stable repository.