Opened 2 years ago
Closed 2 years ago
#28879 closed enhancement (fixed)
spkg-configure for GSL
Reported by: | dimpase | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.1 |
Component: | build: configure | Keywords: | |
Cc: | embray, fbissey, isuruf, mkoeppe | Merged in: | |
Authors: | Dima Pasechnik | Reviewers: | Isuru Fernando |
Report Upstream: | N/A | Work issues: | install modified gsl.pc |
Branch: | a180599 (Commits, GitHub, GitLab) | Commit: | a18059933c6b6ecfe4e98a7c8e1ca6de3dc223c8 |
Dependencies: | #27870 | Stopgaps: |
Description
Change History (56)
comment:1 Changed 2 years ago by
- Commit changed from f8833aa8b98fadbb8f060e112ba4ef3638a5421d to 3a95431674d81d946926d0553d0f70b0d77654a4
comment:2 Changed 2 years ago by
- Cc fbissey added
- Status changed from new to needs_review
- Type changed from PLEASE CHANGE to enhancement
comment:3 Changed 2 years ago by
- Cc isuruf mkoeppe added
comment:4 follow-up: ↓ 6 Changed 2 years ago by
Did you check this with an underlinked GSL (A blas library is not linked in) or a fully linked one like Sage's GSL?
comment:5 Changed 2 years ago by
- Commit changed from 3a95431674d81d946926d0553d0f70b0d77654a4 to 137c97252c15455b1d0e8c8d81929b4a21e722b2
Branch pushed to git repo; I updated commit sha1. New commits:
137c972 | typo in variable name
|
comment:6 in reply to: ↑ 4 ; follow-up: ↓ 7 Changed 2 years ago by
Replying to isuruf:
Did you check this with an underlinked GSL (A blas library is not linked in) or a fully linked one like Sage's GSL?
Only on debian and gentoo, where it's linked to its own gslcblas
comment:7 in reply to: ↑ 6 Changed 2 years ago by
Replying to dimpase:
Replying to isuruf:
Did you check this with an underlinked GSL (A blas library is not linked in) or a fully linked one like Sage's GSL?
Only on debian and gentoo, where it's linked to its own gslcblas
Not on my gentoo box, but that would be the default. In any case neither debian or gentoo will ship it underlinked. I don't think any major distro ship underlinked libraries. OK as much as they can help it. Stuff that people build themselves is another story
comment:8 follow-up: ↓ 21 Changed 2 years ago by
What happens when gslcblas
and openblas
are both linked in?
comment:9 Changed 2 years ago by
A bit pathological, but basically nothing visible until you test something with a high precision where there can be divergences between the two. If you run GSL test suite with openblas rather than gslcblas you'll get failures of the numerical noise kind.
Honestly, I am not sure which one would be used. My guess would be the first one on the list of libraries linked.
comment:10 Changed 2 years ago by
Okay. Another concern is when system's gsl.pc
is used by downstream packages, they'll be linking to gslcblas
instead of openblas
, but sage's gsl.pc
would link the downstream package to openblas
.
comment:11 Changed 2 years ago by
fbissey@moonloop ~ $ cat /usr/lib64/pkgconfig/gsl.pc prefix=/usr exec_prefix=/usr libdir=/usr/lib64 includedir=/usr/include GSL_CBLAS_LIB=-lcblas Name: GSL Description: GNU Scientific Library Version: 2.5 Libs: -L/usr/lib64 -lgsl ${GSL_CBLAS_LIB} -lm -lm Cflags: -I/usr/include
distro will usually patch gsl.pc to match the linking they are doing. Supposing that libgsl is linked with cblas1 and sage tries to link extensions gslext.pyx using libgsl to cblas2. If there are direct call to cblas in glsext.pyx, cblas2 will be used for these, otherwise -lcblas2 will be ignored. If there is no direct calls to cblas in gslext.pyx, libgsl will use cblas1, since cblas2 will be discarded. If cblas2 is not discarded because there are calls to cblas in gslext.pyx what you get in libgsl is not clearly defined and may depend on what is loaded first.
comment:12 Changed 2 years ago by
On Ubuntu,
(base) isuru@isuru:~$ cat /usr/lib/x86_64-linux-gnu/pkgconfig/gsl.pc prefix=/usr exec_prefix=/usr libdir=/usr/lib/x86_64-linux-gnu includedir=/usr/include GSL_CBLAS_LIB=-lgslcblas Name: GSL Description: GNU Scientific Library Version: 2.4 Libs: -L/usr/lib/x86_64-linux-gnu -lgsl ${GSL_CBLAS_LIB} -lm -lm Cflags: -I/usr/include (base) isuru@isuru:~$ ls /usr/lib/x86_64-linux-gnu/libgslcblas.so /usr/lib/x86_64-linux-gnu/libgslcblas.so (base) isuru@isuru:~$ ls -al /usr/lib/x86_64-linux-gnu/libgslcblas.so lrwxrwxrwx 1 root root 20 Aug 9 2017 /usr/lib/x86_64-linux-gnu/libgslcblas.so -> libgslcblas.so.0.0.0 (base) isuru@isuru:~$ ls -al /usr/lib/x86_64-linux-gnu/libgslcblas.so.0.0.0 -rw-r--r-- 1 root root 255968 Aug 9 2017 /usr/lib/x86_64-linux-gnu/libgslcblas.so.0.0.0
comment:13 Changed 2 years ago by
It is underlinked as far as I can tell.
(base) isuru@isuru:~$ ldd /usr/lib/x86_64-linux-gnu/libgsl.so.23 linux-vdso.so.1 (0x00007fffb1ba6000) libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f8a703f4000) libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f8a70003000) /lib64/ld-linux-x86-64.so.2 (0x00007f8a70bf4000)
comment:14 follow-up: ↓ 20 Changed 2 years ago by
OK, I am surprised. Then cblas as to be provided with anything linking libgsl. And if we are taking it from gsl.pc that may be gslcblas. At worst we take performance hit.
An option would be to substitute gslcblas by the the default cblas we are using in the linking flags.
comment:15 Changed 2 years ago by
In the openblas spkg-configure
script, openblas.pc
is copied into $SAGE_LOCAL/lib/pkgconfig/cblas.pc
. How about copying the system gsl.pc
into $SAGE_LOCAL/lib/pkgconfig/gsl.pc
and replacing the gslcblas line with openblas? (or whatever BLAS is selected)
comment:16 Changed 2 years ago by
Remove GSL_CBLAS_LIB
and add a line
requires: cblas
which will pull cblas.pc
all by itself.
comment:17 Changed 2 years ago by
Just to make it clear, are you proposing to change gsl.pc
in the underlinked case only,
right?
Will the underlinked GSL work with "cblas", which is in the configuration of #27870 the same as openblas
? (I am ignorant about the purpose of gslcblas
, sorry).
comment:18 Changed 2 years ago by
Yes it will work. It would work if we did it in all cases. gslcblas
is just another complete cblas implementation just as atlas and openblas are.
comment:19 Changed 2 years ago by
- Status changed from needs_review to needs_work
- Work issues set to install modified gsl.pc
comment:20 in reply to: ↑ 14 Changed 2 years ago by
Replying to fbissey:
OK, I am surprised. Then cblas as to be provided with anything linking libgsl. And if we are taking it from gsl.pc that may be gslcblas. At worst we take performance hit.
An option would be to substitute gslcblas by the the default cblas we are using in the linking flags.
GSL is also underlinked on Fedora
comment:21 in reply to: ↑ 8 Changed 2 years ago by
Replying to isuruf:
What happens when
gslcblas
andopenblas
are both linked in?
this is what I get with this branch:
$ ldd local/lib/python3.7/site-packages/cvxopt/gsl.cpython-37m-x86_64-linux-gnu.so ... libgsl.so.23 => /usr/lib/x86_64-linux-gnu/libgsl.so.23 (0x00007d76b701a000) libopenblas.so.0 => /usr/lib/x86_64-linux-gnu/libopenblas.so.0 (0x00007d76b4e36000) ... libgslcblas.so.0 => /usr/lib/x86_64-linux-gnu/libgslcblas.so.0 (0x00007d76b48db000) ... libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007d76b462e000) ...
so both libopenblas.so
and libgslblas.so
are there.
(I suppose it's the same with Sage-supplied GSL and OpenBLAS)
Are you saying that modifyng gsl.pc
as suggested here would lead to skipping libgslblas.so
?
IMHO this would not lead to re-linking libgsl.so
with libopenblas.so
, if it was
already linked with libgslblas.so
. Woulndn't is need some LD_PRELOAD
platform-dependent ld
hacks --- I guess MacOS would need something quite different to achieve this.
comment:22 Changed 2 years ago by
(I suppose it's the same with Sage-supplied GSL and OpenBLAS)
No, see https://github.com/sagemath/sage/blob/f0ae571d17e685258c1de89c2a29953f4d8fb302/build/pkgs/gsl/patches/gsl-2.1-gslcblas.patch . gslcblas is removed sage-suplied GSL.
Are you saying that modifyng gsl.pc as suggested here would lead to skipping libgslblas.so ?
Yes
IMHO this would not lead to re-linking libgsl.so with libopenblas.so, if it was already linked with libgslblas.so
libgsl.so
is not linked to any blas in distros like Ubuntu. It is underlinked. In sage-the-distribution, libgsl.so
is linked to libopenblas
and the -lgslcblas
line is removed from gsl.pc
.
comment:23 Changed 2 years ago by
No, why, on Debian 10 I see
$ ldd /usr/lib/x86_64-linux-gnu/libgsl.so linux-vdso.so.1 (0x00007ffcf2bb5000) libgslcblas.so.0 => /usr/lib/x86_64-linux-gnu/libgslcblas.so.0 (0x00007a0da11b5000) libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007a0da1032000) libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007a0da0e71000) /lib64/ld-linux-x86-64.so.2 (0x00007a0da1499000)
So libgsl.so is linked to libopenblas.so
comment:24 follow-up: ↓ 25 Changed 2 years ago by
I guess it depends on the distro. I checked Ubuntu 18.04, and it's underlinked there. See https://trac.sagemath.org/ticket/28879#comment:13
comment:25 in reply to: ↑ 24 Changed 2 years ago by
Replying to isuruf:
I guess it depends on the distro. I checked Ubuntu 18.04, and it's underlinked there. See https://trac.sagemath.org/ticket/28879#comment:13
Yes, it's distro-dependent. So how is this gsl.pc hack supposed to help in such a case (of already linked gslcblas? Are we left with linking to both openblas and gslcblas then?
comment:26 follow-up: ↓ 27 Changed 2 years ago by
In the case that it is already linked to libgslcblas, then the library or the executable using gsl has the option of selecting the blas.
See https://wiki.debian.org/DebianScience/LinearAlgebraLibraries section "Using the GSL with an optimized BLAS"
comment:27 in reply to: ↑ 26 Changed 2 years ago by
Replying to isuruf:
In the case that it is already linked to libgslcblas, then the library or the executable using gsl has the option of selecting the blas.
See https://wiki.debian.org/DebianScience/LinearAlgebraLibraries section "Using the GSL with an optimized BLAS"
well, this does not quite wotk. After these changes to gsl.pc I see the correct linking command
gcc -pthread -shared -L/home/dimpase/sage/local/lib -Wl,-rpath,/home/dimpase/sage/local/lib -L. -L/home/dimpase/sage/local/lib -Wl,-rpath,/home/dimpase/sage/local/lib -L/home/dimpase/sage/local/lib -Wl,-rpath,/home/dimpase/sage/local/lib build/temp.linux-x86_64-3.7/src/C/gsl.o -L/home/dimpase/sage/local -L/home/dimpase/sage/local -L/home/dimpase/sage/local/lib -lm -lgsl -lopenblas -lpython3.7m -o build/lib.linux-x86_64-3.7/cvxopt/gsl.cpython-37m-x86_64-linux-gnu.so
but stil the same ldd output as in comment:21.
comment:28 follow-up: ↓ 31 Changed 2 years ago by
ldd
gives transitive deps. Try readelf -d local/lib/python3.7/site-packages/cvxopt/gsl.cpython-37m-x86_64-linux-gnu.so
comment:29 Changed 2 years ago by
OK, so readelf -d
indeed shows that the only linking to gslcblas comes from one of the linked libs
0x0000000000000001 (NEEDED) Shared library: [libm.so.6] 0x0000000000000001 (NEEDED) Shared library: [libgsl.so.23] 0x0000000000000001 (NEEDED) Shared library: [libopenblas.so.0] 0x0000000000000001 (NEEDED) Shared library: [libpython3.7m.so.1.0] 0x0000000000000001 (NEEDED) Shared library: [libpthread.so.0] 0x0000000000000001 (NEEDED) Shared library: [libc.so.6]
and it must be libgsl, for obvious reasons. So that's as good as it gets, we won't try to relink libgsl, right?
comment:30 Changed 2 years ago by
So that's as good as it gets, we won't try to relink libgsl, right?
Yes. Looks good. Can you push your changes to modify gsl.pc?
comment:31 in reply to: ↑ 28 Changed 2 years ago by
comment:32 Changed 2 years ago by
- Commit changed from 137c97252c15455b1d0e8c8d81929b4a21e722b2 to 2bc86cd701014b1bdcc0c7d5055c5d323962fbd2
Branch pushed to git repo; I updated commit sha1. New commits:
2bc86cd | install and modify gsl.pc to use cblas
|
comment:33 Changed 2 years ago by
This is not ideal, as one should use $SED
, not sed
.
However, calling sed
happens in config.status
, and I have no
idea how to make $SED
known there.
Probably some magic with "init commands" I don't know.
comment:34 Changed 2 years ago by
- Status changed from needs_work to needs_review
comment:35 follow-up: ↓ 37 Changed 2 years ago by
- Status changed from needs_review to needs_work
sed -i
without an extension is GNU sed specific and doesn't work with BSD sed
comment:36 Changed 2 years ago by
- Commit changed from 2bc86cd701014b1bdcc0c7d5055c5d323962fbd2 to c13be388d0e6ce1dbd719d64a7a71ff2f87cc82e
Branch pushed to git repo; I updated commit sha1. New commits:
c13be38 | add extension '' to -i in sed calls
|
comment:37 in reply to: ↑ 35 ; follow-up: ↓ 42 Changed 2 years ago by
Replying to isuruf:
sed -i
without an extension is GNU sed specific and doesn't work with BSD sed
It is not posix full stop. I am surprised it works as suggested on freeBSD. My experience with AIX sed
, which is posix, is that -i
isn't supported at all. Fortunately we don't have to support such dinosaur platforms.
@Dima one of the purpose of -e
is to be able to chain commands.
sed -i'' -e 's/\${GSL_CBLAS_LIB}\ //' -e 's/GSL_CBLAS_LIB.*/Require: cblas/' "$SAGE_LOCAL"/lib/pkgconfig/gsl.pc
is perfectly legal. Of course some line separation gain in clarity, but that's why I would break things into several line in a pure shell script
sed \ -e 's/\${GSL_CBLAS_LIB}\ //' \ -e 's/GSL_CBLAS_LIB.*/Require: cblas/' \ -i'' $SAGE_LOCAL"/lib/pkgconfig/gsl.pc
comment:38 Changed 2 years ago by
- Commit changed from c13be388d0e6ce1dbd719d64a7a71ff2f87cc82e to 310ba06411ee413379c1ce3ca75b595ac83e8b0b
Branch pushed to git repo; I updated commit sha1. New commits:
310ba06 | get SED into config.status
|
comment:39 Changed 2 years ago by
- Status changed from needs_work to needs_review
OK, all done, also managed to get SED value into config.status.
comment:40 follow-up: ↓ 41 Changed 2 years ago by
if you prefer chained -e
in one sed call, let me know. I don't see this comment while I fought value of SED thing :-)
comment:41 in reply to: ↑ 40 Changed 2 years ago by
Replying to dimpase:
if you prefer chained
-e
in one sed call, let me know. I don't see this comment while I fought value of SED thing :-)
That's just nitpicking and absolutely non-esential. My comment should be taken as informative rather than "take this action, please".
comment:42 in reply to: ↑ 37 Changed 2 years ago by
Replying to fbissey:
Replying to isuruf:
sed -i
without an extension is GNU sed specific and doesn't work with BSD sedIt is not posix full stop. I am surprised it works as suggested on freeBSD. My experience with AIX
sed
, which is posix, is that-i
isn't supported at all. Fortunately we don't have to support such dinosaur platforms.
I'm pretty sure this doesn't work on OSX. See https://stackoverflow.com/a/5694430/4768820
comment:43 Changed 2 years ago by
So it should be -i ''
, not -i''
? I don't have access to an OSX machine in the coming 2 weeks, sorry.
comment:44 Changed 2 years ago by
-i ''
works in BSD/OSX sed, but doesn't in GNU sed. -i''
works in GNU sed, but doesn't in BSD/OSX sed.
Solution is to use -i.bak
as in the StackOverflow? answer. This creates a backup file with suffix .bak
comment:45 Changed 2 years ago by
- Commit changed from 310ba06411ee413379c1ce3ca75b595ac83e8b0b to f9d6966f63a12991b81dce40ba69887cd9dacaa3
Branch pushed to git repo; I updated commit sha1. New commits:
f9d6966 | get rid of -i option of sed
|
comment:46 Changed 2 years ago by
OK, done. Now it's POSIX, I think.
comment:47 Changed 2 years ago by
- Status changed from needs_review to needs_work
testing this on Ubuntu (i.e. undelinked libgsl.so) gives a linking error during python import:
$ ./sage --python Python 3.7.3 (default, Dec 21 2019, 11:29:09) [GCC 7.4.0] on linux Type "help", "copyright", "credits" or "license" for more information. >>> from sage.rings.complex_double import CDF Traceback (most recent call last): File "<stdin>", line 1, in <module> ImportError: /usr/lib/i386-linux-gnu/libgsl.so.23: undefined symbol: cblas_ctrmv >>>
I suppose some sagelib modules don't know that they must also link with openblas.
comment:48 Changed 2 years ago by
- Commit changed from f9d6966f63a12991b81dce40ba69887cd9dacaa3 to 9d9e0465310b67352105644e7e26eabb2684c84b
Branch pushed to git repo; I updated commit sha1. New commits:
9d9e046 | fix a crucial typo in edited gsl.pc
|
comment:49 Changed 2 years ago by
the .pc syntax needs Requires
rather than Require
...
comment:50 Changed 2 years ago by
- Status changed from needs_work to needs_review
now things work on Ubuntu, too.
comment:51 Changed 2 years ago by
- Status changed from needs_review to positive_review
Tested that configure creates gsl.pc correctly in OSX.
comment:52 Changed 2 years ago by
- Reviewers set to Isuru Fernando
comment:53 Changed 2 years ago by
- Commit changed from 9d9e0465310b67352105644e7e26eabb2684c84b to a18059933c6b6ecfe4e98a7c8e1ca6de3dc223c8
- Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. Last 10 new commits:
c031c9e | use openblas.pc for blas and lapack, don't require atlas is to be used
|
b49f79e | move blas handling to build/pkgs/
|
81bf522 | create cblas.pc link, too, not only blas and lapack
|
244f954 | spkg-configure for gsl
|
efd642f | typo in variable name
|
641e3fa | install and modify gsl.pc to use cblas
|
51bc622 | add extension '' to -i in sed calls
|
13a5434 | get SED into config.status
|
ac32b13 | get rid of -i option of sed
|
a180599 | fix a crucial typo in edited gsl.pc
|
comment:54 Changed 2 years ago by
- Milestone changed from sage-9.1 to sage-9.0
- Status changed from needs_review to positive_review
trivial rebase over updated #27870
comment:55 Changed 2 years ago by
- Milestone changed from sage-9.0 to sage-9.1
comment:56 Changed 2 years ago by
- Branch changed from u/dimpase/packages/gslconf to a18059933c6b6ecfe4e98a7c8e1ca6de3dc223c8
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
update pkgconfig to v1.5.1. See trac #28883
a bare minumim openblas config
use openblas.pc for blas and lapack, don't require atlas is to be used
move blas handling to build/pkgs/
create cblas.pc link, too, not only blas and lapack
spkg-configure for gsl