Added LAPACK library detection mechanisms to compile and test library…

[KJ7LNW]

Unified @pkgs list into @libsets in find_libs() to group libraries by
dependency.

Compatibility with the 0.33 release is important across multiple UNIX
deployments. The first section discusses how the new code is equivalent to the
old code in terms of functionality, while extending the detection to make
actual FORTRAN calls to test that the libraries work. In addition, libraries
with multiple dependencies are tested together instead and independently,
instead of only independently as previously designed.

This commit is documented in sections

1. Overview of the current implementaiton
2. Design goals for the new implementation
3. Devel::CheckLib limitations
4. Implementation details
5. Validation

ORIGINAL IMPLEMENTATION

The previous code was as follows. The indented lines are original code, unindented lines are commentary. There are 3 major parts to the original library detection mechanism:

#  Part 1: This section tries to find LAPACK by searching
# for -llapack and -lblas.  If neither PkgConfig->find(),
# ExtUtils::PkgConfig->libs, nor `pkg-config` can find the
# libraries, then '-L/usr/lib/atlas -llapack -lblas -latlas'
# is forced as a default:
	my @pkgs = qw(lapack blas);
	our $libs0 = (
	  eval {require PkgConfig; join ' ', map PkgConfig->find($_)->get_ldflags, @pkgs} ||
	  eval {require ExtUtils::PkgConfig; join ' ', map ExtUtils::PkgConfig->libs($_), @pkgs} ||
	  `pkg-config @pkgs --libs` ||
	  '-L/usr/lib/atlas -llapack -lblas -latlas'

	# Finally, any FORTRAN libraries are added to $lib0:
	) . ' ' . ExtUtils::F77->runtime;

	our $inc = '-DF77_USCORE='. (ExtUtils::F77->trail_ ? '_' : '');
	{

#  Part 2a: use check_lib() to strip any libraries added above
# (lapack, blas) that fail to link as tested by check_lib().
# They may fail to link for one of two reasons.  Either (a) they
# do not exist on the host, or (b) that fail to link independently
# (ie, lapack may depend on blas or vice-versa):

	  # work around Devel::CheckLib not doing Text::ParseWords::shellwords
	  use Text::ParseWords qw(shellwords);
	  my @libpath = grep -d, map {my $r=$_;$r=~s/^-L//?$r:()} my @sw = shellwords $libs0;
	  my @lib = grep check_lib(lib=>$_), map {my $r=$_;$r=~s/^-l//?$r:()} @sw;
	  $libs0 = join ' ', (map qq{-L"$_"}, @libpath), (map "-l$_", @lib);

#  Part 2b: Emit a warning and `exit 0` if compilation fails.
# This is a sticky issue, because Devel::CheckLib has some very old
# known bugs.  Notably, RT75803: (10 years ago): "Allow function
# option to be used without lib option".        Simply stated, if 'lib=>'
# is not passed to check_lib_or_exit() then it will do nothing,
# and return success.  PDL::LinearAlgebra commit 1d25c1b8 _does_
# have a 'lib=>' option, but _it was removed_ in e1b65760, at
# which point the following code may as well have been deleted:

	  my $f77_uscore = (ExtUtils::F77->trail_ ? '_' : '');
	  check_lib_or_exit(
		ldflags => $libs0,
		header => [($^O =~ /MSWin/ ? 'float.h' : ()), qw(stdio.h math.h)],
		function => ...
	  )
	}

So we can distill the above code down to two items:

1. Add the FORTRAN libs determined by ExtUtils::F77
2. Add -llapack, -lblas, and -latlas if pkg-config cannot find them.

NEW DESIGN

The new design intends to satisfy the following:

1. Define an array of reasonable library combinations that are compatible with existing UNIX deployments
2. By default, the first successful library is selected.
   • A library is "successful" if it can compile and link FORTRAN functions.
3. Allow users who have multiple LAPACK/BLAS implementations to select a specific implementation if they wish.
4. Provide adequate debug output for troubleshooting test reports.
5. Linux and other UNIXen that support compling and linking with -l<lib> and -L<dir> linker options are treated the same. While there may be special cases for libraries (openblas vs atlas), there is no special-case for UNIX-like operating systems.
6. Library detection/selection for Windows will not be modified.
7. Safety: Use shell_quote/shellwords to make sure library paths are clean.

BUGS IN Devel::CheckLib

(They are commented in the code as well, so pasted here for good visibility)

In addition, Devel::CheckLib fails to provide argc to main() before about v1.11, so a Devel::CheckLib version requirement was added.

IMPLEMENTATION

We have written a wrapper new_check_lib() around Devel::CheckLib::check_lib() to work around the issues above. This is the comment in the code:

Configurability with environment variables:

1. PDL_LA_LDFLAGS='-L<dir> -l<lib>...':
   The user can set this to specify their own linker flags (for example, to link with the Intel MKL library).

2. PDL_LA_DEBUG=1: Pass debug=>1 to Devel::CheckLib::check_lib()

3. PDL_LA_LIB_INDEX=<n>: Select a LAPACK/BLAS library.

When Makefile.PL runs, it emits output like the following. The user can specify PDL_LA_LIB_INDEX=<n> to choose the implementation that succeeded. This defaults to 0 if not specified:

	  [0] found (openblaso): -Llibgfortran.a -L/usr/lib -L/opt/intel/compilers_and_libraries_2020.4.304/linux/mkl/lib/intel64_lin/ -lgfortran -lm -lopenblaso
		  missing (openblas_openmp)
	  [1] found (tatlas): -Llibgfortran.a -L/usr/lib -L/opt/intel/compilers_and_libraries_2020.4.304/linux/mkl/lib/intel64_lin/ -L/usr/lib64/atlas/ -lgfortran -lm -ltatlas
	  [2] found (openblasp): -Llibgfortran.a -L/usr/lib -L/opt/intel/compilers_and_libraries_2020.4.304/linux/mkl/lib/intel64_lin/ -lgfortran -lm -lopenblasp
		  missing (openblas_pthreads)
	  [3] found (openblas): -Llibgfortran.a -L/usr/lib -L/opt/intel/compilers_and_libraries_2020.4.304/linux/mkl/lib/intel64_lin/ -lgfortran -lm -lopenblas
		  missing (openblas_serial)
	  [4] found (satlas): -Llibgfortran.a -L/usr/lib -L/opt/intel/compilers_and_libraries_2020.4.304/linux/mkl/lib/intel64_lin/ -L/usr/lib64/atlas/ -lgfortran -lm -lsatlas
	  [5] found (atlas): -Llibgfortran.a -L/usr/lib -L/opt/intel/compilers_and_libraries_2020.4.304/linux/mkl/lib/intel64_lin/ -L/usr/lib64/atlas/ -lgfortran -lm -lsatlas
		  missing (lapack_atlas)
	  [6] found (lapack): -Llibgfortran.a -L/usr/lib -L/opt/intel/compilers_and_libraries_2020.4.304/linux/mkl/lib/intel64_lin/ -lgfortran -lm -llapack
		  missing (blas)
	  [7] found (lapack blas): -Llibgfortran.a -L/usr/lib -L/opt/intel/compilers_and_libraries_2020.4.304/linux/mkl/lib/intel64_lin/ -lgfortran -lm -llapack -lblas
	  [8] found (lapack blas atlas): -Llibgfortran.a -L/usr/lib -L/opt/intel/compilers_and_libraries_2020.4.304/linux/mkl/lib/intel64_lin/ -L/usr/lib64/atlas/ -lgfortran -lm -llapack -lblas -lsatlas
	  [9] found (mkl_rt): -Llibgfortran.a -L/usr/lib -L/opt/intel/compilers_and_libraries_2020.4.304/linux/mkl/lib/intel64_lin/ -lgfortran -lm -lmkl_rt

Safety:

• -L flags are passed before -l flags so the linker can know the paths before hunting for the libraries.
• Any linker options that match neither -l<lib> nor -L<dir> are passed verbatim.
• Options are handled as lists except when generating an LDFLAGS-style output, in which case shell_quote and shellwords are used to properly parse and escape the input and output.

The library detection code at the top of Makefile.PL is now a single line:

	our $libs0 = find_libs(ExtUtils::F77->runtime, $ENV{PDL_LA_LDFLAGS});

The list of available library combinations are in @libsets atop of find_libs():

  my @libsets = (
      # Empty case if PDL_LA_LDFLAGS satisfies the build
      [],

      # Threaded OpenBLAS/ATLAS
      ['openblaso'],
      ['tatlas'],

      # [ ... many others ...]

      # And finally the options from the original implementation if none of the above options match:
      ['blas'],
      ['lapack', 'blas'],
      ['lapack', 'blas', 'atlas'],

      # Experimental Intel Math Kernel Library support:
      ['mkl_rt'],
    );

Notable changes:

FORTRAN check function:
- Added library code to test dgebal_ existence
- Added note about compile problems when missing a leading empty line
- Added 'return 0' to the end
- Calls to ExtUtils::F77->trail_ ? '_' : '' were replaced with $f77_uscore

Notes:
The windows library detection was not modified in this patch.
The 'shellwords' work-around was not changed, but the diff looks that way

VALIDATION

Oracle Linux 8

The following library combinations pass make test on my Oracle Linux 8
deployment:

	for i in {0..8}; do export i; echo === $i; (PDL_LA_LIB_INDEX=$i perl Makefile.PL && make -j100 && make test) 2>&1 | grep -P 'will use|ok$' -A1; done

1. -Llibgfortran.a -L/usr/lib -lgfortran -lm -lopenblaso
2. -Llibgfortran.a -L/usr/lib -L/usr/lib64/atlas/ -lgfortran -lm -ltatlas
3. -Llibgfortran.a -L/usr/lib -lgfortran -lm -lopenblasp
4. -Llibgfortran.a -L/usr/lib -lgfortran -lm -lopenblas
5. -Llibgfortran.a -L/usr/lib -L/usr/lib64/atlas/ -lgfortran -lm -lsatlas
6. -Llibgfortran.a -L/usr/lib -L/usr/lib64/atlas/ -lgfortran -lm -lsatlas
7. -Llibgfortran.a -L/usr/lib -lgfortran -lm -llapack
8. -Llibgfortran.a -L/usr/lib -lgfortran -lm -llapack -lblas
9. -Llibgfortran.a -L/usr/lib -L/usr/lib64/atlas/ -lgfortran -lm -llapack -lblas -lsatlas

	t/1.t ....... ok
	t/cgtsv.t ... ok
	t/gtsv.t .... ok
	t/legacy.t .. ok
	All tests successful.

Ubuntu / Debian

In addition, the following Ubuntu/Debian distributions have been tested, all
you need to install is libopenblas-dev to build LAPACK+BLAS successfully:

    grep -B4  'All tests' */root/pdl*/build.log

bionic-i386/root/pdl-linearalgebra/build.log-t/1.t ....... ok
bionic-i386/root/pdl-linearalgebra/build.log-t/cgtsv.t ... ok
bionic-i386/root/pdl-linearalgebra/build.log-t/gtsv.t .... ok
bionic-i386/root/pdl-linearalgebra/build.log-t/legacy.t .. ok
bionic-i386/root/pdl-linearalgebra/build.log:All tests successful.
--
bionic/root/pdl-linearalgebra/build.log-t/1.t ....... ok
bionic/root/pdl-linearalgebra/build.log-t/cgtsv.t ... ok
bionic/root/pdl-linearalgebra/build.log-t/gtsv.t .... ok
bionic/root/pdl-linearalgebra/build.log-t/legacy.t .. ok
bionic/root/pdl-linearalgebra/build.log:All tests successful.
--
bullseye-i386/root/pdl-linearalgebra/build.log-t/1.t ....... ok
bullseye-i386/root/pdl-linearalgebra/build.log-t/cgtsv.t ... ok
bullseye-i386/root/pdl-linearalgebra/build.log-t/gtsv.t .... ok
bullseye-i386/root/pdl-linearalgebra/build.log-t/legacy.t .. ok
bullseye-i386/root/pdl-linearalgebra/build.log:All tests successful.
--
bullseye/root/pdl-linearalgebra/build.log-t/1.t ....... ok
bullseye/root/pdl-linearalgebra/build.log-t/cgtsv.t ... ok
bullseye/root/pdl-linearalgebra/build.log-t/gtsv.t .... ok
bullseye/root/pdl-linearalgebra/build.log-t/legacy.t .. ok
bullseye/root/pdl-linearalgebra/build.log:All tests successful.
--
buster-i386/root/pdl-linearalgebra/build.log-t/1.t ....... ok
buster-i386/root/pdl-linearalgebra/build.log-t/cgtsv.t ... ok
buster-i386/root/pdl-linearalgebra/build.log-t/gtsv.t .... ok
buster-i386/root/pdl-linearalgebra/build.log-t/legacy.t .. ok
buster-i386/root/pdl-linearalgebra/build.log:All tests successful.
--
buster/root/pdl-linearalgebra/build.log-t/1.t ....... ok
buster/root/pdl-linearalgebra/build.log-t/cgtsv.t ... ok
buster/root/pdl-linearalgebra/build.log-t/gtsv.t .... ok
buster/root/pdl-linearalgebra/build.log-t/legacy.t .. ok
buster/root/pdl-linearalgebra/build.log:All tests successful.
--
focal/root/pdl-linearalgebra/build.log-t/1.t ....... ok
focal/root/pdl-linearalgebra/build.log-t/cgtsv.t ... ok
focal/root/pdl-linearalgebra/build.log-t/gtsv.t .... ok
focal/root/pdl-linearalgebra/build.log-t/legacy.t .. ok
focal/root/pdl-linearalgebra/build.log:All tests successful.
--
xenial-i386/root/pdl-linearalgebra/build.log-t/1.t ....... ok
xenial-i386/root/pdl-linearalgebra/build.log-t/cgtsv.t ... ok
xenial-i386/root/pdl-linearalgebra/build.log-t/gtsv.t .... ok
xenial-i386/root/pdl-linearalgebra/build.log-t/legacy.t .. ok
xenial-i386/root/pdl-linearalgebra/build.log:All tests successful.
--
xenial/root/pdl-linearalgebra/build.log-t/1.t ....... ok
xenial/root/pdl-linearalgebra/build.log-t/cgtsv.t ... ok
xenial/root/pdl-linearalgebra/build.log-t/gtsv.t .... ok
xenial/root/pdl-linearalgebra/build.log-t/legacy.t .. ok
xenial/root/pdl-linearalgebra/build.log:All tests successful.

[KJ7LNW]

Please review and send it through CI. Its a big change but thats what is needed for broad compatibility. Suggestions welcome...

-Eric

[mohawk2]

Thank you for what is obviously a lot of work! I believe there are some issues (as shown below), but they can be fixed (probably by reducing the scope of this change).

As you can see, the CI has already run. If you add a configure dep, you need to add it to the CI as noted on #15. If you have minor improvements (like updating the needed version of Devel::CheckLib (DCL) for argc) that should be in a separate, more-easily-reviewable PR, to reduce the scope of this one. I will need persuading that going through the libs flags list is a good idea.

Regarding this one, the fact that you are adding 400 lines is a red flag. Copy-pasting code out of DCL is a sign that a mistake has been made. I am not yet convinced that the partitioning of the lib lines is needed at all. The way you are doing produces (copied from your message above):

-Llibgfortran.a [...]

which is another red flag, because that is nonsensical. Also, I do not consider it valuable to allow control for this module to pick its LAPACK implementation. I'm also not at all a fan of loud configuration, so that needs removing. These days most users won't even see the output of the configuration as they'll use e.g. cpanm. The env var to debug is a great idea, and should be in the actual docs, probably with an early =head1 MODULE CONFIGURATION.

Using a regex to detect whether something is an absolute path is an error and instead File::Spec should be used (however, I have yet to be convinced that the approach that requires this is even a good idea).

Finally, you've shown results for Debian derivatives, but not BSD, nor RedHat derivatives - that will need rectifying (VirtualBox or VMware can help). http://matrix.cpantesters.org/?dist=PDL-LinearAlgebra+0.35 shows pass:fail of 60:44 for the latest, and 59:46 for 0.34. That ratio is shown on https://www.cpantesters.org/distro/P/PDL-LinearAlgebra.html?oncpan=1&distmat=1&version=0.35&grade=2 (on the left) to have been there for a number of versions, so it's not a catastrophe, but I am sure we can do better.

[coveralls]

Coverage remained the same at 53.638% when pulling 7ba193b on KJ7LNW:lapack-detect into 61c04d4 on PDLPorters:master.

[KJ7LNW]

At this point I need to be done working on this to focus on other projects.

However, I'll comment to help others who might pick it up, and I can push minor changes based on your findings if someone else can drive additional testing:

Thank you for what is obviously a lot of work!

You're welcome :)

I believe there are some issues (as shown below), but they can be fixed (probably by reducing the scope of this change).

As you'll see below, there isn't much that can be trimmed unless you want to loose support for something. This was quite the interesting and unexpectedly complex investigation:

As you can see, the CI has already run. If you add a configure dep, you need to add it to the CI as noted on #15. If you have minor improvements (like updating the needed version of Devel::CheckLib (DCL) for argc) that should be in a separate, more-easily-reviewable PR, to reduce the scope of this one. I will need persuading that going through the libs flags list is a good idea.

Thanks for the note. Pushed. Looks like OSX still has a problem and I would need to see the output of PDL_LA_DEBUG=1 perl Makefile.PL to debug it, but I don't see how to do that in the CI output. If you can tell me how then I'll push another CI if the fix is minor.

Regarding this one, the fact that you are adding 400 lines is a red flag.

Maybe. Its only as big as I found that it needed to be. Just be glad I didn't use Term::ANSIColor !

Copy-pasting code out of DCL is a sign that a mistake has been made.

Hmm? I didn't copy-paste anything from another package! How un-perl that would be, indeed. This is original code... Devel::CheckLib is used as-is, I just made a wrapper to work around its decade-old bugs. See the comment above new_check_lib() and also RT#60176, RT#75803, and RT#109028.

I am not yet convinced that the partitioning of the lib lines is needed at all.

Ultimately it is because of RT#60176 and RT#75803 (and debian library naming issues). If those were fixed then maybe we wouldn't need to. See comments in the patch.

The way you are doing produces (copied from your message above): -Llibgfortran.a [...]
which is another red flag, because that is nonsensical.

I thought that too, but shrugged because it was out of my control. It is produced by ExtUtils::F77 as you can see here:

]$ perl -MExtUtils::F77 -le 'print ExtUtils::F77->runtime'
Loaded ExtUtils::F77 version 1.26
"-Llibgfortran.a" -L/usr/lib -lgfortran  -lm

Also, I do not consider it valuable to allow control for this module to pick its LAPACK implementation.

Well, humbly, and for the benefit of PDL's crown jewel, I disagree. Users wanting P::LA are probably interested in performance. If they are, then choice is valuable for a number of reasons that I learned while adding LAPACK to xnec2c: OpenBLAS, ATLAS, and Intel MKL each have different threading models and distributions package them very differently. In addition, different threading models perform differently on different workloads---and that is a per-user decision. (We were able to get all threading models in all 3 of those LAPACK implementations working for xnec2c on Ubuntu, Debian, and Red Hat.)

Here is what I learned:

There are wide differences between threading models even in the same LAPACK implementation across distributions. For example, Debian/Ubuntu uses alternatives whereas RHEL splits out threading models by name: openblaso for OpenMP, openblasp for pthreads, etc, but defaults to just serial "openblas" which may not be ideal from a performance point of view. The only way to get OpenBLAS+OpenMP in RHEL to select it somehow. (Intel MKL is even more weird, don't get me started on that! ... but Intel MKL is probably the fastest LAPACK implementation out there!)

These days most users won't even see the output of the configuration as they'll use e.g. cpanm. The env var to debug is a great idea, and should be in the actual docs, probably with an early =head1 MODULE CONFIGURATION.

Good ideas.

I'm also not at all a fan of loud configuration, so that needs removing.

As you wish. Though it might be useful for debugging different deployments. I was hoping to see that output in CI, but not sure where it went?

Using a regex to detect whether something is an absolute path is an error and instead File::Spec should be used

Maybe so.

(however, I have yet to be convinced that the approach that requires this is even a good idea).

As above, splitting out -l<lib> and -L<dir> and /other/stuff was necesary to work around Devel::CheckLib bugs. In addition, Ubuntu/Debian don't always create a <lib>.so file in path so -l<lib> doesn't even work. Thus the -L<dir>'s need to be searched for <lib>.so.<ver> and the /path/to/<lib>.so.<version> is placed in LDFLAGS so it will build.

Most of the time was spent fighting with Devel::CheckLib. Believe me, if there is a better way that actually works with a broad set of distributions, then I'm all for it---but after what I've found here I doubt it.

Finally, you've shown results for Debian derivatives,

yep

but not BSD,

True, and I won't. Don't have the means, time, nor interest to poke at BSD. However, if others want to provide build logs then I'll make minor changes to get it in working order.

nor RedHat derivatives

Actually that is not so. We're an almost 100% redhat shop and use Oracle/CentOS for pretty much everything: See the Oracle Linux 8 section under the validation section. Its the same as RHEL 8 and has the best wide-spread LAPACK support. Every LAPACK implementation and threading model is detected and selectable, and all of them pass make test.

• that will need rectifying (VirtualBox or VMware can help).

I used nspawn for the ubu/debian tests above

http://matrix.cpantesters.org/?dist=PDL-LinearAlgebra+0.35 shows pass:fail of 60:44 for the latest, and 59:46 for 0.34. That ratio is shown on https://www.cpantesters.org/distro/P/PDL-LinearAlgebra.html?oncpan=1&distmat=1&version=0.35&grade=2 (on the left) to have been there for a number of versions, so it's not a catastrophe, but I am sure we can do better.

Maybe so. #15 is a serious bug that will take some noteworthy engineering to get it working across so many distributions.

[KJ7LNW]

There is a possibility that adding libnvblas.so and libnvblas10.so to the library search in this PR could enable NVBLAS GPU support. However, I don't have the hardware to test that...would be interesting if someone has nVidia hardware to test NVBLAS in PDL::LA.

[KJ7LNW]

...also, if someone can get me PDL_LA_DEBUG=1 perl Makefile.PL output for OSX then maybe that can be fixed.