Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cython_aliases: Do not fail if one of the listed libraries is not known to pkgconfig #31384

Closed
mkoeppe opened this issue Feb 11, 2021 · 31 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 11, 2021

(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

@mkoeppe mkoeppe added this to the sage-9.3 milestone Feb 11, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 11, 2021

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 11, 2021

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

dace125sage.env.cython_aliases: Do not fail if one of the listed libraries is not known to pkgconfig

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 11, 2021

Commit: dace125

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 11, 2021

Reviewer: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 11, 2021

comment:4

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.

@tobiasdiez
Copy link
Contributor

comment:6

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 12, 2021

comment:7

Thanks! Then I will rework this ticket for the purposes of modularization, making the list of needed libraries an input.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 15, 2021

comment:9

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Mar 15, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 22, 2021

comment:10

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.

@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.3 Mar 22, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 22, 2021

comment:11

(Related previous ticket: #30706)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 22, 2021

Changed commit from dace125 to 4f42440

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 22, 2021

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

4f42440sage.env.cython_aliases: Do not fail if one of the listed libraries is not known to pkgconfig

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 22, 2021

Changed commit from 4f42440 to 3cb8f90

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 22, 2021

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

3cb8f90sage.env.cython_aliases: Make module lapack optional; generalize the interface

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 22, 2021

comment:14

If any changes in cython_aliases would eliminate downstream patching, now would be the time...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 22, 2021

Changed author from Tobias Diez to Tobias Diez, Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 22, 2021

Changed reviewer from Matthias Koeppe to Matthias Koeppe, ...

@kiwifb
Copy link
Member

kiwifb commented Mar 22, 2021

comment:16

I don't know about other distros but Gentoo is good. This particular section hasn't needed patching in some time here.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 23, 2021

comment:17

Debian has a relevant patch: https://sources.debian.org/patches/sagemath/9.2-2/d0-gsl-cblas.patch/

@kiwifb
Copy link
Member

kiwifb commented Mar 23, 2021

comment:18

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.

@tobihan
Copy link

tobihan commented Mar 23, 2021

comment:20

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?

@antonio-rojas
Copy link
Contributor

comment:21

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

https://salsa.debian.org/science-team/lapack/-/blob/master/debian/README.if-you-look-for-libcblas.so.3

The unpatched Netlib reference blas implementation does install a libcblas.so library.

@tobihan
Copy link

tobihan commented Mar 23, 2021

comment:22

Ok, then the fallback for Debian should be to use blas.pc and link against libblas.

@kiwifb
Copy link
Member

kiwifb commented Mar 23, 2021

comment:23

Sounds fair. If cblas.pc is not found, look for blas.pc instead. It should be doable.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 23, 2021

comment:24

You can do this already without patching sagelib by setting CBLAS_PC_MODULES = "cblas:blas" in sage_conf.py.

@kiwifb
Copy link
Member

kiwifb commented Mar 23, 2021

comment:25

Yes, indeed the mechanics to do that is already there. So many good changes recently :)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 23, 2021

comment:26

Sounds like no further changes are needed on this ticket. Let's get this into 9.3 please

@kliem
Copy link
Contributor

kliem commented Mar 25, 2021

comment:27

LGTM.

@kliem
Copy link
Contributor

kliem commented Mar 25, 2021

Changed reviewer from Matthias Koeppe, ... to Matthias Koeppe, Jonathan Kliem

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 26, 2021

comment:28

Thanks!

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Apr 18, 2021
@vbraun
Copy link
Member

vbraun commented May 27, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants