#29398 closed enhancement (fixed)

Accept system (Open)BLAS

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.1
Component: build: configure Keywords:
Cc: dimpase, embray, isuruf, arojas, fbissey, vbraun Merged in:
Authors: Matthias Koeppe Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 0cda07c (Commits, GitHub, GitLab) Commit: 0cda07c33fe26f035377c57c8982f3e2ea8da85d
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

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:

2) No devel package is provided as liblapack-devel already provide the needed headers and import. Openblas is fully compatible with Netlib BLAS.

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) - see #29393 (Use system openblas on Fedora)

Change History (59)

comment:1 Changed 19 months ago by mkoeppe

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)?

comment:2 follow-up: Changed 19 months ago by dimpase

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.

comment:3 follow-up: Changed 19 months ago by mkoeppe

which of our packages depends specifically on openblas.pc?

comment:4 in reply to: ↑ 2 Changed 19 months ago by mkoeppe

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

comment:5 in reply to: ↑ 3 Changed 19 months ago by dimpase

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.

comment:6 Changed 19 months ago by mkoeppe

Yes, that's the problem.

comment:7 Changed 19 months ago by mkoeppe

  • Branch set to u/mkoeppe/cygwin__accept_system_blas

comment:8 Changed 19 months ago by dimpase

  • Commit set to ba97132a21bc05c5b05cc31ea86567751bcffca6

is this on top of a not yet merged ticket? Please list it in Dependencies.

Also, what are these OPENBLAS_* variables?


New commits:

bb89447build/pkgs/openblas/spkg-configure.m4: Use OPENBLAS_{LIBS,CFLAGS} while checking for cblas/lapack functions
71ac6a5build/pkgs/openblas/spkg-configure.m4: Do not use AC_FC_FUNC.
7188bddMerge branch 't/29361/openblas_spkg_configure_m4__fix_the_check_for_lapack_cblas_functions' into t/29398/cygwin__accept_system_blas
ba97132build/pkgs/openblas/spkg-configure.m4: Fall back to checking whether {blas,cblas,lapack}.pc correspond to openblas - for cygwin

comment:9 Changed 19 months ago by mkoeppe

  • Dependencies set to #29361

comment:10 Changed 19 months ago by mkoeppe

Not ready for review yet.

OPENBLAS_* is created by the pkgconfig macro

comment:11 Changed 19 months ago by mkoeppe

And "of course" it does not work at all as advertised on cygwin.

comment:12 Changed 19 months ago by mkoeppe

cblas.pc refers to a non-existent library "-lcblas"...

comment:13 Changed 19 months ago by dimpase

we can resort to shipping platform-dependent openblas.pc

or, indeed, generate it.

comment:14 Changed 19 months ago by git

  • Commit changed from ba97132a21bc05c5b05cc31ea86567751bcffca6 to 089057b1fa6a29b15883be508d20df14323ba632

Branch pushed to git repo; I updated commit sha1. New commits:

f0bd0ecbuild/pkgs/openblas/spkg-configure.m4: Try if blas.pc alone has everything
e08192bfixup
089057bUpdate comments

comment:15 Changed 19 months ago by mkoeppe

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.

comment:16 follow-up: Changed 19 months ago by mkoeppe

gsl's spkg-install needs updating too -- it uses pkgconfig cblas

comment:17 Changed 19 months ago by mkoeppe

  • Dependencies changed from #29361 to #29361, #29082

comment:18 Changed 19 months ago by git

  • Commit changed from 089057b1fa6a29b15883be508d20df14323ba632 to a34e7a301461c814ef0112e376f99736324e0cb9

Branch pushed to git repo; I updated commit sha1. New commits:

56a4f4cbuild/pkgs/glpk/distros/cygwin.txt: New
ba09bbabuild/pkgs/ncurses/distros/cygwin.txt: Fixup
391d224build/pkgs/openblas/distros/cygwin.txt: New
e9f831abuild/pkgs/ntl/distros/cygwin.txt: New
905148bbuild/pkgs/pcre/distros/cygwin.txt: New
12540acbuild/pkgs/r/distros/cygwin.txt: New
3fd30e3build/pkgs/libatomic_ops/distros/cygwin.txt: New
f29a168build/pkgs/cygwin.txt: python -> python37
0162b7eMerge branch 't/29397/add_more_cygwin_system_packages' into t/29398/cygwin__accept_system_blas
a34e7a3build/pkgs/r/distros/cygwin.txt: Add libtirpc-devel, needed for building rpy2

comment:19 Changed 19 months ago by git

  • Commit changed from a34e7a301461c814ef0112e376f99736324e0cb9 to 5df9ca3af1cca195c7f9ac2fe5fc0b0c6480d2f1

Branch pushed to git repo; I updated commit sha1. New commits:

705da6aLink/generate pc files in make, not configure
d86cd33fixup
f5b4f62Makefile [misc-clean]: No need to remove build/lib/pkgconfig
2a38a16build/pkgs/gsl/spkg-configure.m4: Quoting fix
329843bIgnore /src/lib/pkgconfig using top-level .gitignore
5df9ca3Merge branch 't/29082/move__pc_file_from_src__to_build___clean_generated___pc_files_at__make_distclean_' into t/29398/cygwin__accept_system_blas

comment:20 Changed 19 months ago by mkoeppe

  • Dependencies changed from #29361, #29082 to #29361, #29397, #29082

comment:21 in reply to: ↑ 16 Changed 19 months ago by mkoeppe

Replying to mkoeppe:

gsl's spkg-install needs updating too -- it uses pkgconfig cblas

Or, rather, we should be generating cblas.pc

comment:22 follow-up: Changed 19 months ago by mkoeppe

  • Description modified (diff)

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-standard openblas, which has no .pc files at all (#29393 Use system openblas on Fedora).

comment:23 Changed 19 months ago by dimpase

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)

comment:24 Changed 19 months ago by mkoeppe

I don't think one needs an absolute path at all in this situation.

comment:25 Changed 19 months ago by dimpase

well, that's what you have in *.pc files, absolute paths, I don't really know why.

comment:26 Changed 19 months ago by mkoeppe

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

comment:27 Changed 19 months ago by git

  • Commit changed from 5df9ca3af1cca195c7f9ac2fe5fc0b0c6480d2f1 to b57d690b5eb19a2fbfabd39d3f74101e2bbe24a9

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

db7ffdbbuild/pkgs/openblas/spkg-configure.m4: Fall back to checking whether {blas,cblas,lapack}.pc correspond to openblas - for cygwin
20d2f9abuild/pkgs/openblas/spkg-configure.m4: Try if blas.pc alone has everything
f2c53d8fixup
28d0055Update comments
b57d690build/pkgs/r/distros/cygwin.txt: Add libtirpc-devel, needed for building rpy2

comment:28 Changed 19 months ago by mkoeppe

Rebased on 9.1.beta9

comment:29 in reply to: ↑ 22 Changed 19 months ago by mkoeppe

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-standard openblas, which has no .pc files at all (#29393 Use system openblas on Fedora).

Help with this would be welcome...

comment:30 Changed 19 months ago by mkoeppe

  • Dependencies #29361, #29397, #29082 deleted

comment:31 Changed 19 months ago by git

  • Commit changed from b57d690b5eb19a2fbfabd39d3f74101e2bbe24a9 to 950ba671f078caeaa964db7292212a0450d30178

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b9cc4b4build/pkgs/r/distros/cygwin.txt: Add libtirpc-devel, needed for building rpy2
950ba67build/pkgs/openblas/spkg-configure.m4: Fall back to building openblas.pc from AC_SEARCH_LIBS

comment:32 Changed 19 months ago by mkoeppe

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.

comment:33 Changed 19 months ago by dimpase

would it also work on Fedora (which doesn't install openblas.pc)?

comment:34 Changed 19 months ago by mkoeppe

I would hope so, but haven't tested yet.

comment:36 Changed 19 months ago by dimpase

  • Description modified (diff)
  • Summary changed from cygwin: Accept system BLAS to Accept system (Open)BLAS

comment:37 Changed 19 months ago by mkoeppe

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.

Last edited 19 months ago by mkoeppe (previous) (diff)

comment:38 Changed 19 months ago by mkoeppe

  • Description modified (diff)

comment:39 Changed 19 months ago by dimpase

Do you mean that on Cygwin they don't have openblas_get_config even for "real" openblas? This is a bug...

comment:40 Changed 19 months ago by mkoeppe

They only use openblas as a drop-in binary-compatible replacement for blas/cblas.

comment:41 Changed 19 months ago by dimpase

if it's cygwin-specific, maybe it's easier to call cygcheck -c openblas ?

comment:42 Changed 19 months ago by mkoeppe

That wouldn't give the correct answer when users change the PATH from the default one, so that the generic blas dll wins

comment:43 Changed 19 months ago by mkoeppe

comment:44 Changed 19 months ago by git

  • Commit changed from 950ba671f078caeaa964db7292212a0450d30178 to 2873d86a6e39d76320250d1b8509e301ff00f868

Branch pushed to git repo; I updated commit sha1. New commits:

2873d86build/pkgs/openblas/spkg-configure.m4: Check version also when no pc file is available

comment:45 Changed 19 months ago by mkoeppe

  • Authors set to Matthias Koeppe

comment:46 Changed 19 months ago by mkoeppe

  • Status changed from new to needs_review

comment:47 Changed 19 months ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

lgtm

comment:48 Changed 19 months ago by mkoeppe

Thanks!

comment:49 Changed 19 months ago by mkoeppe

  • Status changed from positive_review to needs_work

comment:50 Changed 19 months ago by mkoeppe

I guess I should remove my debugging code that disables pkgconfig detection from this!

comment:51 Changed 19 months ago by mkoeppe

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_

comment:52 follow-up: Changed 19 months ago by git

  • Commit changed from 2873d86a6e39d76320250d1b8509e301ff00f868 to 0cda07c33fe26f035377c57c8982f3e2ea8da85d

Branch pushed to git repo; I updated commit sha1. New commits:

0cda07cbuild/pkgs/openblas/spkg-configure.m4: Make pkgconfig-based detection work again

comment:53 Changed 19 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:54 in reply to: ↑ 52 Changed 19 months ago by dimpase

Replying to git:

Branch pushed to git repo; I updated commit sha1. New commits:

0cda07cbuild/pkgs/openblas/spkg-configure.m4: Make pkgconfig-based detection work again

oops, I missed this.

comment:55 Changed 19 months ago by mkoeppe

comment:56 Changed 19 months ago by dimpase

  • Status changed from needs_review to positive_review

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?

comment:57 follow-up: Changed 19 months ago by mkoeppe

$ cat build/pkgs/sqlite/distros/debian.txt 
libsqlite3-dev

We can add the package with the executable sqlite3

comment:58 in reply to: ↑ 57 Changed 19 months ago by mkoeppe

Replying to mkoeppe:

We can add the package with the executable sqlite3

This is now #29487

comment:59 Changed 19 months ago by vbraun

  • Branch changed from u/mkoeppe/cygwin__accept_system_blas to 0cda07c33fe26f035377c57c8982f3e2ea8da85d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.