Opened 4 years ago
Closed 4 years ago
#18777 closed enhancement (fixed)
Remove unneeded BLAS linking for Cython modules
Reported by: | jdemeyer | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.8 |
Component: | build | Keywords: | |
Cc: | jason, fbissey, jpflori | Merged in: | |
Authors: | Jeroen Demeyer | Reviewers: | François Bissey |
Report Upstream: | N/A | Work issues: | |
Branch: | be582ec (Commits) | Commit: | be582ec8355415737606b208b102f895a9cfde57 |
Dependencies: | Stopgaps: |
Description (last modified by )
Various Cython modules implementing matrices and vectors list
libraries = [BLAS, BLAS2]
in src/module_list.py
.
However, nothing in Sage directly uses BLAS, it is only used through external libraries (linbox, numpy, ...). So it makes no sense to link to BLAS and nothing else, which means that this is a mistake.
I checked the commit history and these extensions listing only BLAS, BLAS2
as libraries go back to #3498 and #4206:
commit a8b66cc30d4aca925c1e3a1c0bb69d0f4f62fbb1 Author: Jason Grout <jason-sage@creativetrax.com> Date: Wed Jul 9 19:54:35 2008 -0500 Switch the RDF and CDF matrices to a numpy 1.2 backend; factor out common functionality to matrix_double_dense.pyx.
and
commit b07600d93461c717fc6702e99afd0f2cf2eeb1df Author: Jason Grout <jason-sage@creativetrax.com> Date: Tue Nov 11 03:30:38 2008 -0600 Change the RDF/CDF vector backend to numpy, and refactor the code to a common parent vector_double_dense. Also change names of the classes to vector_real_double_dense and vector_complex_double_dense.
It seems that the backends were changed from GSL to Numpy and I guess that the BLAS libraries (needed for GSL) were simply kept.
There might also be the mistaken assumption that BLAS was needed for numpy. It's true that numpy uses BLAS, but we use numpy using the standard Python import
mechanism, it's not the usual dynamic linking (there is no -lnumpy
argument needed either).
Change History (6)
comment:1 Changed 4 years ago by
- Description modified (diff)
comment:2 Changed 4 years ago by
- Branch set to u/jdemeyer/remove_unneeded_blas_linking_for_cython_modules
comment:3 Changed 4 years ago by
- Commit set to be582ec8355415737606b208b102f895a9cfde57
- Status changed from new to needs_review
comment:4 Changed 4 years ago by
OK I will check all of these with readelf
in sage-on-gentoo. Since It is all compiled with -as-needed
we'll definitely see if anything needs cblas directly. If the dependency is indirect will it break on cygwin?
comment:5 Changed 4 years ago by
- Reviewers set to François Bissey
- Status changed from needs_review to positive_review
All these only link to libc
and libpython
which I could have seen straight away from module_list.py
. We definitely want that simplification. I cannot see how we would introduce side effects by removing these linkings.
comment:6 Changed 4 years ago by
- Branch changed from u/jdemeyer/remove_unneeded_blas_linking_for_cython_modules to be582ec8355415737606b208b102f895a9cfde57
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Remove unneeded BLAS linking for Cython modules