There is one slight problem with the package: -devel picks up a dependency on libopenblaso.so()(64bit), which isn't provided by any package.

I'd appreciate if someone can point out why this happens..

I ran a series of matrix diagonalization benchmarks, and on my Intel i7-2600 at work OpenBLAS is 9.6% faster than ATLAS, which is rather notable.

Furthermore, OpenBLAS also can thread more operations than ATLAS such as diagonalization, which makes it 42% faster when using 4 threads. (Yes, this is bad scaling.)

Actually, the issue with calling exit should not be ignored. Shared libraries have no business calling exit. The proper way is to return an error and let the calling application handle it.

Picking up for review.

FYI: I've already asked upstream to soname the libraries properly, and update LAPACK support to 3.3.1. According to upstream, the latter should happen in October.

Builds in mock rawhide/x86_64.

$ rpmlint /var/lib/mock/fedora-rawhide-x86_64/result
openblas.src:11: W: macro-in-comment %{version}
openblas.src:11: W: macro-in-comment %{alpha}
openblas.src:112: E: hardcoded-library-path in %{buildroot}/usr/lib/libopen*
openblas.src: W: invalid-url Source0: xianyi-OpenBLAS-v0.1alpha2.4-0-gfe7a932.tar.gz
openblas-static.x86_64: W: no-documentation
openblas-devel.x86_64: W: no-documentation
openblas-threads.x86_64: W: spelling-error Summary(en_US) pthreads -> threads, p threads, packthread
openblas-threads.x86_64: W: no-soname /usr/lib64/libopenblasp-r0.1alpha2.4.so
openblas-threads.x86_64: W: shared-lib-calls-exit /usr/lib64/libopenblasp-r0.1alpha2.4.so exit.5
openblas-threads.x86_64: W: no-documentation
openblas-openmp.x86_64: W: no-soname /usr/lib64/libopenblaso-r0.1alpha2.4.so
openblas-openmp.x86_64: W: shared-lib-calls-exit /usr/lib64/libopenblaso-r0.1alpha2.4.so exit.5
openblas-openmp.x86_64: W: no-documentation
openblas.x86_64: W: no-soname /usr/lib64/libopenblas-r0.1alpha2.4.so
openblas.x86_64: W: shared-lib-calls-exit /usr/lib64/libopenblas-r0.1alpha2.4.so exit.5
openblas.x86_64: W: no-documentation
7 packages and 0 specfiles checked; 1 errors, 15 warnings.

The gcc command which the library is linked with lacks -Wl,-soname=,
hence the no-soname warning from rpmlint. I think this must be fixed.

Fixing this properly might also require modifying the %files lists
apart from patching the Makefiles.


$ md5sum v0.1alpha2.4 xianyi-OpenBLAS-v0.1alpha2.4-0-gfe7a932.tar.gz 
c23bc85bc536b175533b862e964b4fe1  v0.1alpha2.4
c23bc85bc536b175533b862e964b4fe1  xianyi-OpenBLAS-v0.1alpha2.4-0-gfe7a932.tar.gz

The changes from GotoBLAS2 are mainly added support for the Chinese
Loongson CPU and some superficial changes like minor build system tweaks,
renamed files, new name and added copyright/license texts.


Bundles lapack-3.1.1 sources -> investigate unbundling, if sources are
necessary to build, contact lapack maintainer to add a -source subpackage
and BuildRequire it.

Bundles some files from lapack-3.0 sources, at least lapack/getri/*.f.

Bundles http://www.netlib.org/blas/blast-forum/cblas.tgz:CBLAS/testing
directory as ctest/ and (modified) test/. Also cblas.h is basically
the same, only reformatted and with parameter names changed.

Bundles a modified http://www.netlib.org/blas/blas.tgz:BLAS directory
as reference/.

I guess the above are GotoBLAS2's legacy.


make -C serial TARGET=CORE2 ...

Minimum requirements for Fedora are still Pentium Pro or newer.
Will this run on a Pentium Pro?


# Get rid of rpaths
for lib in %{buildroot}%{_libdir}/libopenblas{,o,p}-*.so; do
 execstack -c $lib
done

The comment seems wrong. Also, is that the only way to remove executable stack?

rm -rf %{buildroot} in %install and the %clean section are only necessary for EPEL.

There are no docs included in %files. I'd suggest at least these:
Changelog.txt
GotoBLAS*.txt
LICENSE <- especially important (MUST be included)
README

(In reply to comment #5)
> The gcc command which the library is linked with lacks -Wl,-soname=,
> hence the no-soname warning from rpmlint. I think this must be fixed.

I don't think the missing soname is a big issue, since the BLAS/LAPACK API has stabilized a *long* time ago.

I have reported the lack of soversioning upstream before asking for the review.

> Fixing this properly might also require modifying the %files lists
> apart from patching the Makefiles.

Sure. But I don't want to add soversions myself; that's the job of upstream.

> The changes from GotoBLAS2 are mainly added support for the Chinese
> Loongson CPU and some superficial changes like minor build system tweaks,
> renamed files, new name and added copyright/license texts.

Yes, I'd think the major part of the code is straight from GotoBLAS2, which was non-free for a long time. GotoBLAS is, however, dead nowadays, so packaging OpenBLAS seems a lot more sane.

> Bundles lapack-3.1.1 sources -> investigate unbundling, if sources are
> necessary to build, contact lapack maintainer to add a -source subpackage
> and BuildRequire it.
> 
> Bundles some files from lapack-3.0 sources, at least lapack/getri/*.f.

Maybe I'll need to ask for an exception. A generic -source package is not enough, since the build scripts assume a specific version.

Although, I see that this problem is solved in ATLAS by just BR'ing the static version of the LAPACK libraries. Maybe the same thing could be done with OpenBLAS as well.

> Bundles http://www.netlib.org/blas/blast-forum/cblas.tgz:CBLAS/testing
> directory as ctest/ and (modified) test/. Also cblas.h is basically
> the same, only reformatted and with parameter names changed.

Since OpenBLAS provides CBLAS functions, the headers have to be duplicated anyhow.

> Bundles a modified http://www.netlib.org/blas/blas.tgz:BLAS directory
> as reference/.

.. although it's not used anyhow; it's only used when a cross-check against the reference implementation is requested.

> Minimum requirements for Fedora are still Pentium Pro or newer.
> Will this run on a Pentium Pro?

Judging from GotoBLAS_01Readme.txt, the minimum CPU is Pentium3 or Athlon.
If someone still runs older systems, they can use ATLAS instead.

It would be of course possible to limit this package to only, say, x86_64 architecture, where it will run on every system.

> The comment seems wrong. Also, is that the only way to remove executable stack?

Good catch.

Yes, I think so. Gcc creates executable stacks whenever there's an assembly section without a GNU-stack note. And these aren't portable, they're not often used.

> rm -rf %{buildroot} in %install and the %clean section are only necessary for
> EPEL.

.. where this package will also be useful.

> There are no docs included in %files. I'd suggest at least these:

Added.

http://theory.physics.helsinki.fi/~jzlehtol/rpms/openblas.spec
http://theory.physics.helsinki.fi/~jzlehtol/rpms/openblas-0.1-2.alpha2.4.fc15.src.rpm

(In reply to comment #6)
> (In reply to comment #5)
> > Bundles lapack-3.1.1 sources -> investigate unbundling, if sources are
> > necessary to build, contact lapack maintainer to add a -source subpackage
> > and BuildRequire it.
> > 
> > Bundles some files from lapack-3.0 sources, at least lapack/getri/*.f.
> 
> Maybe I'll need to ask for an exception. A generic -source package is not
> enough, since the build scripts assume a specific version.
> 
> Although, I see that this problem is solved in ATLAS by just BR'ing the static
> version of the LAPACK libraries. Maybe the same thing could be done with
> OpenBLAS as well.

Please do. If you definitely need an exception, please file a ticket to the FPC: https://fedorahosted.org/fpc/newticket and be prepared to answer the https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries#Standard_questions

> > Minimum requirements for Fedora are still Pentium Pro or newer.
> > Will this run on a Pentium Pro?
> 
> Judging from GotoBLAS_01Readme.txt, the minimum CPU is Pentium3 or Athlon.
> If someone still runs older systems, they can use ATLAS instead.
> 
> It would be of course possible to limit this package to only, say, x86_64
> architecture, where it will run on every system.

OK. By the way, does it build on non-x86?

(In reply to comment #7)
> > > Minimum requirements for Fedora are still Pentium Pro or newer.
> > > Will this run on a Pentium Pro?
> > 
> > Judging from GotoBLAS_01Readme.txt, the minimum CPU is Pentium3 or Athlon.
> > If someone still runs older systems, they can use ATLAS instead.
> > 
> > It would be of course possible to limit this package to only, say, x86_64
> > architecture, where it will run on every system.
> 
> OK. By the way, does it build on non-x86?

I haven't tried.

I'll be taking a two-week vacation now, so I won't be able to look at this until I get back.

(In reply to comment #8)
> I'll be taking a two-week vacation now, so I won't be able to look at this
> until I get back.

Whoops...

Anyway, I have rebased the package to 0.2.5, otherwise the spec is pretty much the same as the one reviewed above.

I ran into some speed issues with ATLAS, so I'd like to get this package in the repos now. 

http://theory.physics.helsinki.fi/~jzlehtol/rpms/openblas.spec
http://theory.physics.helsinki.fi/~jzlehtol/rpms/openblas-0.2.5-1.fc18.src.rpm

koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4728927

Oh, I fixed the build issue on i386 and RHEL5 as well.

http://theory.physics.helsinki.fi/~jzlehtol/rpms/openblas.spec
http://theory.physics.helsinki.fi/~jzlehtol/rpms/openblas-0.2.5-2.fc18.src.rpm

Dominik - could you approve the review?

Use system version of LAPACK.

http://theory.physics.helsinki.fi/~jzlehtol/rpms/openblas.spec
http://theory.physics.helsinki.fi/~jzlehtol/rpms/openblas-0.2.5-3.fc18.src.rpm

... but don't include the reference implementations of functions that have an optimized implementation in openblas. 

http://theory.physics.helsinki.fi/~jzlehtol/rpms/openblas.spec
http://theory.physics.helsinki.fi/~jzlehtol/rpms/openblas-0.2.5-4.fc18.src.rpm

Build fails on EL-6/x86_64 at %prep stage:

Executing(%prep): /bin/sh -e /home/rathann/build/tmp/rpm-tmp.MbqSqa
...
+ mkdir netliblapack
+ cd netliblapack
+ ar x /usr/lib64/liblapack_pic.a
+ ar x /usr/lib64/liblapacke.a
ar: /usr/lib64/liblapacke.a: No such file or directory

Assuming that was a typo, I modified the above to use /usr/lib64/lapack.a, but then it fails to copy /usr/include/lapacke and /usr/include/lapack doesn't exist either.

I'm now test-building with that line commented.

Nope, no typo - liblapacke contains the C interfaces.

$ rpm -qf /usr/lib64/liblapacke.a 
lapack-static-3.4.1-2.fc18.x86_64

Looks like lapacke was integrated in lapack only in version 3.4.0, and it is thus unavailable in EPEL, which only has 3.2.1 (EL6) vs 3.0 (EL5).

Now this does raise once again the question of a bundled library exception in the case of EPEL.

Well, I remembered that lapacke support can be turned off. Here's a version that builds also in EPEL.

http://theory.physics.helsinki.fi/~jzlehtol/rpms/openblas.spec
http://theory.physics.helsinki.fi/~jzlehtol/rpms/openblas-0.2.5-5.fc18.src.rpm

Thanks, it builds on EPEL-6 fine now.

rpmlint output for rawhide build:
openblas-threads.x86_64: W: spelling-error Summary(en_US) pthreads -> threads, p threads, threader
openblas-threads.x86_64: E: no-ldconfig-symlink /usr/lib64/libopenblasp-r0.2.5.so
openblas-threads.x86_64: W: shared-lib-calls-exit /usr/lib64/libopenblasp-r0.2.5.so exit.5
openblas-threads.x86_64: W: no-documentation
openblas-static.x86_64: W: no-documentation
openblas-devel.x86_64: W: no-documentation
openblas.src:136: E: hardcoded-library-path in %{buildroot}/usr/lib/libopen*
openblas.src: W: invalid-url Source0: xianyi-OpenBLAS-v0.2.5-0-gf78eb33.tar.gz
openblas-openmp.x86_64: E: no-ldconfig-symlink /usr/lib64/libopenblaso-r0.2.5.so
openblas-openmp.x86_64: W: shared-lib-calls-exit /usr/lib64/libopenblaso-r0.2.5.so exit.5
openblas-openmp.x86_64: W: no-documentation
openblas.x86_64: W: shared-lib-calls-exit /usr/lib64/libopenblas-r0.2.5.so exit.5
7 packages and 0 specfiles checked; 3 errors, 9 warnings.

ldconfig will break the libopenblas.so.0 symlink after first run. You need to find another way to include the alternative (pthread, openmp) versions of the library. They all have the same SONAME, so the last one which gets installed will be pointed to by the libopenblas.so.0 symlink. In my case, it was openblas-threads.

More interestingly, rpmlint run on installed EPEL-6 packages shows this:
openblas.x86_64: W: shared-lib-calls-exit /usr/lib64/libopenblas-r0.2.5.so exit.5
openblas-openmp.x86_64: E: no-ldconfig-symlink /usr/lib64/libopenblaso-r0.2.5.so
openblas-openmp.x86_64: W: shared-lib-calls-exit /usr/lib64/libopenblaso-r0.2.5.so exit.5
openblas-openmp.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopenblaso-r0.2.5.so /usr/lib64/libgomp.so.1
openblas-openmp.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopenblaso-r0.2.5.so /lib64/libpthread.so.0
openblas-openmp.x86_64: W: no-documentation
openblas-threads.x86_64: W: spelling-error Summary(en_US) pthreads -> threads, p threads, threader
openblas-threads.x86_64: E: no-ldconfig-symlink /usr/lib64/libopenblasp-r0.2.5.so
openblas-threads.x86_64: W: shared-lib-calls-exit /usr/lib64/libopenblasp-r0.2.5.so exit.5
openblas-threads.x86_64: W: no-documentation
openblas-devel.x86_64: W: no-documentation
openblas-static.x86_64: W: no-documentation
5 packages and 0 specfiles checked; 2 errors, 10 warnings.

The unused-direct-shlib-dependency warnings for -openmp package are new and warrant some investigation. Perhaps the openmp version doesn't use openmp after all?

Regarding the source tarball, it looks like it was created from a different commit than the name claims:
$ tar xzf xianyi-OpenBLAS-v0.2.5-0-gf78eb33.tar.gz
$ ls -ld xianyi-OpenBLAS*
drwxr-xr-x. 12 rathann users    4096 Nov 26 10:32 xianyi-OpenBLAS-e42259c

Please identify the correct commit and follow the newly-approved guidelines for github sources: https://fedoraproject.org/wiki/User:Spot/GitHub_Guidelines

Minor nitpicks:
1. there's some trailing whitespace in line 31
2. most of the description is repeated in each package, you could use a macro to write out the common part in each subpackage

I've fixed everything else except the unused-direct-shlib-dependency stuff. I'll contact upstream about it.


http://theory.physics.helsinki.fi/~jzlehtol/rpms/openblas.spec
http://theory.physics.helsinki.fi/~jzlehtol/rpms/openblas-0.2.5-6.fc18.src.rpm

Turns out the problem was that in the OpenMP version you still need USE_THREADS=1. Fixed in the above.

Would it be possible to produce a 64bit integer variant say openblas64 by adding INTERFACE64=1? 

This would help making other numerical software 64 bit, notably umfpack (of suitesparse), scipy, octave, etc. There is clearly a need for this. You might need to also produce a 64bit LAPACK using -fdefault-integer-8 compiler flag. However I would be perfectly happy not having LAPACK.

I also must mention that the default BLAS used by MATLAB on 64 bit platforms uses 64 bit integers, that is -fdefault-integer-8 in FORTRAN and ptrdiff_t in C/C++.

(In reply to comment #21)
> Would it be possible to produce a 64bit integer variant say openblas64 by
> adding INTERFACE64=1? 

Yes, it would. But I figure the 4-byte integer variant is still needed as well.

This can be done pretty easily after the package is in Fedora.

Created attachment 677085 [details]
parallel installable libopenblas versions with same SONAME

Actually, I think it's wrong to resolve the file conflicts by changing the library SONAMEs. All versions, irrespective of the thread options, implement the same ABI. Let's go with atlas-style separate directories and ld.so.conf.d drop-ins. Suggested patch attached.

Then ATLAS does it wrong.

Putting the choice on system level is giving you the choice between a hammer and a sledgehammer, when you might want to use a hammer for some things and the sledgehammer for others.

I'd very much prefer to be able to install both a serial and an OpenMP version at the same time and compile my programs so that the serial version uses a serial version of OpenBLAS, and the OpenMP version uses an OpenMP version of OpenBLAS.

The alternative is that both use the serial version (screw up parallel runs' performance), or that both use the parallel version (screw up job balancing).

Now, this could of course be solved with environment modules, but I do think that the compile-time choice is the best one. After all, the threads-version of libopenblas has a different soname by default!

Go with environment modules, then. By using different SONAMEs, you're forcing users to relink if they want to switch from serial to parallel. Why do you think compile-time choice is best?

Coming from a high-performance computing background, I just listed the reasons above in comment #24.

Even the environment modules solution is really a pain in the ass, since what the environment module should really be doing is just making a dummy wrapper available that exports the libopenblas.so soname, but actually is just calling the functions from libopenblaso (or libopenblasp). This way SMP programs can still be using the SMP version.

IMNSHO if people don't have any idea of what they are doing, then they are free to link either against the serial, the pthreaded or the OpenMP version.

However, it really means a lot to be able to run serial programs in serial mode and parallel programs in parallel mode. This package is going to be used on a lot of computer clusters, where jobs are allocated a set amount of processors. Then, if a serial program that for some reason uses a parallel library is run, it can severely mess up the load balancing of said cluster. Most importantly, MPI jobs will be often totally stuck waiting on the one node where a single process is not playing nicely.

So, the reasoning boils down to a very simple fact: whether a program will be SMP is determined at compile time. Thus the choice of the flavor of library to use should also be done at compile time.

(In reply to comment #23)
> Actually, I think it's wrong to resolve the file conflicts by changing the
> library SONAMEs. All versions, irrespective of the thread options, implement
> the same ABI. Let's go with atlas-style separate directories and
> ld.so.conf.d drop-ins. Suggested patch attached.

.. continuing on ATLAS, its reasoning for the ld.so.conf.d dropins is wholly different.

ATLAS has separate libraries with separate sonames for serial (libcblas) and SMP/threaded (libptcblas). The same has been done in OpenBLAS wrt the serial vs the pthreads version, I only added the OpenMP version which also needs a separate soname.

Where ATLAS uses the dropins is not the choice between a serial and an SMP version, but rather a choice between multiple flavors of the libraries, e.g. 3dnow, sse, sse2, sse3 and so on. Here a system-wide choice is very sane, since the instruction set is always the same regardless of the job.

A similar approach is not necessary at all in OpenBLAS, since all the different versions are built in the same library, which picks out the optimal version for the processor in use at runtime.

So as a summary, the same methodology I have used is also used in ATLAS, due to the reson given in comment #27.

Alright then, APPROVED.

Thanks for the review!

New Package SCM Request
=======================
Package Name: openblas
Short Description: An optimized BLAS library based on GotoBLAS2
Owners: jussilehtola
Branches: F-17 F-18 EL-5 EL-6
InitialCC:

Git done (by process-git-requests).

For reference, I'm not building any non-x86(_64) binaries since upstream only supports x86 and Loongson; they don't have access to ia64, ppc, sparc and power architecture machines.

openblas-0.2.5-7.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/openblas-0.2.5-7.el6

openblas-0.2.5-7.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/openblas-0.2.5-7.fc18

openblas-0.2.5-7.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/openblas-0.2.5-7.el5

openblas-0.2.5-7.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/openblas-0.2.5-7.fc17

openblas-0.2.5-7.el5 has been pushed to the Fedora EPEL 5 testing repository.

openblas-0.2.5-10.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/openblas-0.2.5-10.fc19

openblas-0.2.5-10.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/openblas-0.2.5-10.el6

openblas-0.2.5-10.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/openblas-0.2.5-10.fc18

openblas-0.2.5-10.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/openblas-0.2.5-10.el5

FYI, this was hanging around since I was waiting for upstream to fix a rather nasty bug [1], and only recently received information that the bug is in the OpenBLAS LAPACK functions. I've now replaced the optimized versions with reference LAPACK ones, so that I can ship the updates in stable.

[1] https://github.com/xianyi/OpenBLAS/issues/191

openblas-0.2.5-10.fc18 has been pushed to the Fedora 18 stable repository.

openblas-0.2.5-10.fc19 has been pushed to the Fedora 19 stable repository.

openblas-0.2.5-10.el5 has been pushed to the Fedora EPEL 5 stable repository.

openblas-0.2.5-10.el6 has been pushed to the Fedora EPEL 6 stable repository.

(In reply to Susi Lehtola from comment #22)
> (In reply to comment #21)
> > Would it be possible to produce a 64bit integer variant say openblas64 by
> > adding INTERFACE64=1? 
> 
> Yes, it would. But I figure the 4-byte integer variant is still needed as
> well.
> 
> This can be done pretty easily after the package is in Fedora.
Any progress with regard to 64-bit interface? I would have used it for a new package I'm making for the Julia language.

No, I hadn't thought about it at all...

I believe we can't make the 64-bit interface the default, because this needs to be taken care of in the user side as well.

Anyway, patches are welcome.

Also, this is OT for this bug.
Please open a new one for the 64-bit interface.

Package Change Request
======================
Package Name: openblas
New Branches: epel7
Owners: jussilehtola

Git done (by process-git-requests).

openblas-0.2.11-1.el7 has been submitted as an update for Fedora EPEL 7.
https://admin.fedoraproject.org/updates/openblas-0.2.11-1.el7

openblas-0.2.11-1.el7 has been pushed to the Fedora EPEL 7 stable repository.