cython_aliases: Do not fail if one of the listed libraries is not known to pkgconfig
mkoeppe opened this issue · 31 comments
(split out from #30371)
This helps with the modularization effort.
CC: @isuruf @kiwifb @antonio-rojas @collares @tobihan @infinity0
Component: build: configure
Author: Tobias Diez, Matthias Koeppe
Branch/Commit: 3cb8f90
Reviewer: Matthias Koeppe, Jonathan Kliem
Issue created by migration from https://trac.sagemath.org/ticket/31384
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
dace125 | sage.env.cython_aliases: Do not fail if one of the listed libraries is not known to pkgconfig |
Commit: dace125
Reviewer: Matthias Koeppe
What did you need this for exactly in #30371?
For modularization we certainly need something like this; but I think when building all modules, with this patch the unresolved cython aliases will just stay as is and cause errors later.
I changed this during my experiments where I tried to have everything work without any previous built sage's packages. So it's not needed for #30371.
Thanks! Then I will rework this ticket for the purposes of modularization, making the list of needed libraries an input.
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
A version of this is needed for Sage 9.3: Since #29706, cython_aliases insists to find lapack.pc even though no LAPACK_... aliases are used anywhere in src/.
In the Sage distribution, of course, we have that; but it fails for example in conda when testing without build of the Sage distribution.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4f42440 | sage.env.cython_aliases: Do not fail if one of the listed libraries is not known to pkgconfig |
Branch pushed to git repo; I updated commit sha1. New commits:
3cb8f90 | sage.env.cython_aliases: Make module lapack optional; generalize the interface |
If any changes in cython_aliases would eliminate downstream patching, now would be the time...
Changed author from Tobias Diez to Tobias Diez, Matthias Koeppe
Changed reviewer from Matthias Koeppe to Matthias Koeppe, ...
I don't know about other distros but Gentoo is good. This particular section hasn't needed patching in some time here.
Debian has a relevant patch: https://sources.debian.org/patches/sagemath/9.2-2/d0-gsl-cblas.patch/
They are enforcing using gslcblas as their cblas library. That's a bit surprising and shocking. Their own comment is confusing, they seem to think gslcblas should be used which is not the case.
I think it could be nice to provide them with a fallback option that could be provided by sage_conf.py if pkg-config cannot find a cblas module. Someone knowing more about what .pc files debian provides should comment.
In Debian BLAS is managed with the alternatives system (update-alternatives), which allows to install multiple implementations at the same time and define one of them as default.
As a result, libblas.so.3, cblas.h and blas.pc are symlinks managed by the alternatives system, so these files do not show up in any package, but they are created as symlinks when any BLAS implementation is installed.
But no BLAS implementation installs a libcblas.so or cblas.pc. Could it be that this is not really needed and provided by libblas.so together with cblas.h?
Replying to @tobihan:
But no BLAS implementation installs a libcblas.so or cblas.pc. Could it be that this is not really needed and provided by libblas.so together with cblas.h?
That's a Debian specific thing
The unpatched Netlib reference blas implementation does install a libcblas.so library.
Ok, then the fallback for Debian should be to use blas.pc and link against libblas.
Sounds fair. If cblas.pc is not found, look for blas.pc instead. It should be doable.
You can do this already without patching sagelib by setting CBLAS_PC_MODULES = "cblas:blas" in sage_conf.py.
Yes, indeed the mechanics to do that is already there. So many good changes recently :)
Sounds like no further changes are needed on this ticket. Let's get this into 9.3 please
LGTM.
Changed reviewer from Matthias Koeppe, ... to Matthias Koeppe, Jonathan Kliem
Thanks!