Accept system (Open)BLAS
mkoeppe opened this issue · 66 comments
As noted in #29397, Cygwin has openblas but does not provide openblas.pc; instead blas.pc, cblas.pc, lapack.pc are provided (but cblas.pc is broken).
From http://cygwin.1069669.n5.nabble.com/Updated-openblas-0-3-3-1-td142613.html:
-
No devel package is provided as liblapack-devel already provide the needed headers and import. Openblas is fully compatible with Netlib BLAS.
-
libopenblas consist of a single file /usr/bin/cygblas-0.dll that will precede in PATH the liblapack0 /usr/lib/lapack/cygblas-0.dll and used instead. Removing libopenblas will restore the usage of Netlib BLAS
Fedora has a similar omission - it does not install openblas.pc (while its openblas has cblas and lapack capabilities) - see #29393 (Use system openblas on Fedora)
CC: @dimpase @embray @isuruf @antonio-rojas @kiwifb @vbraun
Component: build: configure
Author: Matthias Koeppe
Branch/Commit: 0cda07c
Reviewer: Dima Pasechnik
Issue created by migration from https://trac.sagemath.org/ticket/29398
Should we accept any system BLAS (blas+cblas+lapack) on cygwin, or test whether it's provided by openblas, for example by testing for openblas_get_config (https://github.com/xianyi/OpenBLAS/wiki/OpenBLAS-Extensions)?
if we use openblas we would need to generate missing openblas.pc
(same problem on fedora)
build/pkgs/openblas has a python script for this purpose, by the way.
we should ask cygwin maintainers to fix the install and provide pc file.
which of our packages depends specifically on openblas.pc?
Replying to @dimpase:
build/pkgs/openblas has a python script for this purpose, by the way.
write_pc_file.py seems to do the opposite direction -- it writes blas.pc, cblas.pc, lapack.pc. Cygwin already has those
Replying to @mkoeppe:
which of our packages depends specifically on openblas.pc?
I meant, openblas.pc may be used by our spkg-configure for openblas. Otherwise it is not needed.
Yes, that's the problem.
is this on top of a not yet merged ticket?
Please list it in Dependencies.
Also, what are these OPENBLAS_* variables?
New commits:
bb89447 | build/pkgs/openblas/spkg-configure.m4: Use OPENBLAS_{LIBS,CFLAGS} while checking for cblas/lapack functions |
71ac6a5 | build/pkgs/openblas/spkg-configure.m4: Do not use AC_FC_FUNC. |
7188bdd | Merge branch 't/29361/openblas_spkg_configure_m4__fix_the_check_for_lapack_cblas_functions' into t/29398/cygwin__accept_system_blas |
ba97132 | build/pkgs/openblas/spkg-configure.m4: Fall back to checking whether {blas,cblas,lapack}.pc correspond to openblas - for cygwin |
Not ready for review yet.
OPENBLAS_* is created by the pkgconfig macro
And "of course" it does not work at all as advertised on cygwin.
cblas.pc refers to a non-existent library "-lcblas"...
we can resort to shipping platform-dependent openblas.pc
or, indeed, generate it.
This is a version that works for cygwin (at least configure finds blas now).
But it is not really checking that the implementation is provided by openblas. It seems that functions such as openblas_get_config are not accessible.
gsl's spkg-install needs updating too -- it uses pkgconfig cblas
Branch pushed to git repo; I updated commit sha1. New commits:
56a4f4c | build/pkgs/glpk/distros/cygwin.txt: New |
ba09bba | build/pkgs/ncurses/distros/cygwin.txt: Fixup |
391d224 | build/pkgs/openblas/distros/cygwin.txt: New |
e9f831a | build/pkgs/ntl/distros/cygwin.txt: New |
905148b | build/pkgs/pcre/distros/cygwin.txt: New |
12540ac | build/pkgs/r/distros/cygwin.txt: New |
3fd30e3 | build/pkgs/libatomic_ops/distros/cygwin.txt: New |
f29a168 | build/pkgs/cygwin.txt: python -> python37 |
0162b7e | Merge branch 't/29397/add_more_cygwin_system_packages' into t/29398/cygwin__accept_system_blas |
a34e7a3 | build/pkgs/r/distros/cygwin.txt: Add libtirpc-devel, needed for building rpy2 |
Branch pushed to git repo; I updated commit sha1. New commits:
705da6a | Link/generate pc files in make, not configure |
d86cd33 | fixup |
f5b4f62 | Makefile [misc-clean]: No need to remove build/lib/pkgconfig |
2a38a16 | build/pkgs/gsl/spkg-configure.m4: Quoting fix |
329843b | Ignore /src/lib/pkgconfig using top-level .gitignore |
5df9ca3 | Merge branch 't/29082/move__pc_file_from_src__to_build___clean_generated___pc_files_at__make_distclean_' into t/29398/cygwin__accept_system_blas |
Replying to @mkoeppe:
gsl'sspkg-installneeds updating too -- it usespkgconfig cblas
Or, rather, we should be generating cblas.pc
Description changed:
---
+++
@@ -1,4 +1,4 @@
-As noted in #29397, Cygwin has openblas but does not provide openblas.pc; instead blas.pc, cblas.pc, lapack.pc are provided
+As noted in #29397, Cygwin has openblas but does not provide openblas.pc; instead blas.pc, cblas.pc, lapack.pc are provided (but cblas.pc is broken).
From http://cygwin.1069669.n5.nabble.com/Updated-openblas-0-3-3-1-td142613.html:
generating *.pc files is a bit unpleasant, I did it on #28906 - I suppose one can borrow the relevant macros there (finind the absolute patch to the library was the most tricky part)
I don't think one needs an absolute path at all in this situation.
well, that's what you have in *.pc files, absolute paths, I don't really know why.
To my understanding, the variables at the top of the .pc files such as prefix, includedir are there only by convention and to make relocation easier. The only thing that matters is Libs: and Cflags
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
db7ffdb | build/pkgs/openblas/spkg-configure.m4: Fall back to checking whether {blas,cblas,lapack}.pc correspond to openblas - for cygwin |
20d2f9a | build/pkgs/openblas/spkg-configure.m4: Try if blas.pc alone has everything |
f2c53d8 | fixup |
28d0055 | Update comments |
b57d690 | build/pkgs/r/distros/cygwin.txt: Add libtirpc-devel, needed for building rpy2 |
Rebased on 9.1.beta9
Replying to @mkoeppe:
Perhaps best to ignore the .pc files in this situation and just do AC_SEARCH_LIBS for the libraries, and then writing out facade .pc files as in #29082. This would also take care of the
fedora-32-standardopenblas, which has no .pc files at all (#29393 Use system openblas on Fedora).
Help with this would be welcome...
This seems to work. It falls back to using AC_SEARCH_LIBS, and then constructs a minimal openblas.pc.
But I think this needs some extra checks -- otherwise this would accept just about any BLAS in the system and pretend it's openblas.
would it also work on Fedora (which doesn't install openblas.pc)?
I would hope so, but haven't tested yet.
Description changed:
---
+++
@@ -6,4 +6,4 @@
3) libopenblas consist of a single file /usr/bin/cygblas-0.dll that will precede in PATH the liblapack0 /usr/lib/lapack/cygblas-0.dll and used instead. Removing libopenblas will restore the usage of Netlib BLAS
-
+Fedora has a similar omission - it does not install openblas.pc (while its openblas has cblas and lapack capabilities)For checking whether a system blas on Linux is actually openblas, we can use a function test for openblas_get_config.
On Cygwin, I don't know a better solution than to do strings $(which cygblas-0.dll) | grep openblas to distinguish the good openblas from the bad generic blas.
Description changed:
---
+++
@@ -6,4 +6,4 @@
3) libopenblas consist of a single file /usr/bin/cygblas-0.dll that will precede in PATH the liblapack0 /usr/lib/lapack/cygblas-0.dll and used instead. Removing libopenblas will restore the usage of Netlib BLAS
-Fedora has a similar omission - it does not install openblas.pc (while its openblas has cblas and lapack capabilities)
+Fedora has a similar omission - it does not install openblas.pc (while its openblas has cblas and lapack capabilities) - see #29393 (Use system openblas on Fedora)Do you mean that on Cygwin they don't have openblas_get_config even for "real" openblas? This is a bug...
They only use openblas as a drop-in binary-compatible replacement for blas/cblas.
if it's cygwin-specific, maybe it's easier to call cygcheck -c openblas ?
That wouldn't give the correct answer when users change the PATH from the default one, so that the generic blas dll wins
Fedora seems to work too - https://github.com/mkoeppe/sage/runs/550765309
Branch pushed to git repo; I updated commit sha1. New commits:
2873d86 | build/pkgs/openblas/spkg-configure.m4: Check version also when no pc file is available |
Author: Matthias Koeppe
Reviewer: Dima Pasechnik
lgtm
Thanks!
I guess I should remove my debugging code that disables pkgconfig detection from this!
With the pkgconfig-based detection disabled, on debian-buster-standard (https://github.com/mkoeppe/sage/runs/553984061) one sees the following:
self._backend.solve()
File "sage/numerical/backends/cvxopt_sdp_backend.pyx", line 369, in sage.numerical.backends.cvxopt_sdp_backend.CVXOPTSDPBackend.solve (build/cythonized/sage/numerical/backends/cvxopt_sdp_backend.c:4838)
from cvxopt import matrix as c_matrix, solvers
File "/sage/local/lib/python3.7/site-packages/cvxopt/__init__.py", line 279, in <module>
from cvxopt import solvers, blas, lapack
ImportError: /sage/local/lib/python3.7/site-packages/cvxopt/blas.cpython-37m-x86_64-linux-gnu.so: undefined symbol: dtrsm_
Branch pushed to git repo; I updated commit sha1. New commits:
0cda07c | build/pkgs/openblas/spkg-configure.m4: Make pkgconfig-based detection work again |
Replying to @sagetrac-git:
Branch pushed to git repo; I updated commit sha1. New commits:
0cda07c | build/pkgs/openblas/spkg-configure.m4: Make pkgconfig-based detection work again |
oops, I missed this.
Fixed debian-buster-standard: https://github.com/mkoeppe/sage/runs/554994789
OK, looks good.
By the way (offtopic), why do we see this
2020-04-02T16:07:07.4502918Z **********************************************************************
2020-04-02T16:07:07.4503012Z File "src/sage/tests/cmdline.py", line 600, in sage.tests.cmdline.test_executable
2020-04-02T16:07:07.4503087Z Failed example:
2020-04-02T16:07:07.4503167Z err
2020-04-02T16:07:07.4503245Z Expected:
2020-04-02T16:07:07.4503440Z ''
2020-04-02T16:07:07.4503502Z Got:
2020-04-02T16:07:07.4503715Z '/sage/src/bin/sage: line 564: exec: sqlite3: not found\n'
is it by design?
$ cat build/pkgs/sqlite/distros/debian.txt
libsqlite3-dev
We can add the package with the executable sqlite3
Changed branch from u/mkoeppe/cygwin__accept_system_blas to 0cda07c