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:

Status badges

Description


Change History (56)

comment:1 Changed 2 years ago by git

  • Commit changed from f8833aa8b98fadbb8f060e112ba4ef3638a5421d to 3a95431674d81d946926d0553d0f70b0d77654a4

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

e5c91dbupdate pkgconfig to v1.5.1. See trac #28883
1720b38a bare minumim openblas config
925266buse openblas.pc for blas and lapack, don't require atlas is to be used
b89df80move blas handling to build/pkgs/
6341576create cblas.pc link, too, not only blas and lapack
3a95431spkg-configure for gsl

comment:2 Changed 2 years ago by dimpase

  • Cc fbissey added
  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to enhancement

comment:3 Changed 2 years ago by dimpase

  • Cc isuruf mkoeppe added

comment:4 follow-up: Changed 2 years ago by 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?

comment:5 Changed 2 years ago by git

  • Commit changed from 3a95431674d81d946926d0553d0f70b0d77654a4 to 137c97252c15455b1d0e8c8d81929b4a21e722b2

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

137c972typo in variable name

comment:6 in reply to: ↑ 4 ; follow-up: Changed 2 years ago by 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

comment:7 in reply to: ↑ 6 Changed 2 years ago by fbissey

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: Changed 2 years ago by isuruf

What happens when gslcblas and openblas are both linked in?

comment:9 Changed 2 years ago by fbissey

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 isuruf

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

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 isuruf

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 isuruf

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: Changed 2 years ago by 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.

comment:15 Changed 2 years ago by isuruf

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)

Last edited 2 years ago by isuruf (previous) (diff)

comment:16 Changed 2 years ago by fbissey

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 dimpase

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 fbissey

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 dimpase

  • 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 dimpase

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 dimpase

Replying to isuruf:

What happens when gslcblas and openblas 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.

Last edited 2 years ago by dimpase (previous) (diff)

comment:22 Changed 2 years ago by isuruf

(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.

Last edited 2 years ago by isuruf (previous) (diff)

comment:23 Changed 2 years ago by dimpase

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: Changed 2 years ago by 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

comment:25 in reply to: ↑ 24 Changed 2 years ago by dimpase

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: Changed 2 years ago by 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"

comment:27 in reply to: ↑ 26 Changed 2 years ago by dimpase

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: Changed 2 years ago by isuruf

ldd gives transitive deps. Try readelf -d local/lib/python3.7/site-packages/cvxopt/gsl.cpython-37m-x86_64-linux-gnu.so

Last edited 2 years ago by isuruf (previous) (diff)

comment:29 Changed 2 years ago by dimpase

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 isuruf

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 isuruf

Last edited 2 years ago by isuruf (previous) (diff)

comment:32 Changed 2 years ago by git

  • Commit changed from 137c97252c15455b1d0e8c8d81929b4a21e722b2 to 2bc86cd701014b1bdcc0c7d5055c5d323962fbd2

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

2bc86cdinstall and modify gsl.pc to use cblas

comment:33 Changed 2 years ago by dimpase

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 dimpase

  • Status changed from needs_work to needs_review

comment:35 follow-up: Changed 2 years ago by isuruf

  • 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 git

  • Commit changed from 2bc86cd701014b1bdcc0c7d5055c5d323962fbd2 to c13be388d0e6ce1dbd719d64a7a71ff2f87cc82e

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

c13be38add extension '' to -i in sed calls

comment:37 in reply to: ↑ 35 ; follow-up: Changed 2 years ago by fbissey

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 git

  • Commit changed from c13be388d0e6ce1dbd719d64a7a71ff2f87cc82e to 310ba06411ee413379c1ce3ca75b595ac83e8b0b

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

310ba06get SED into config.status

comment:39 Changed 2 years ago by dimpase

  • Status changed from needs_work to needs_review

OK, all done, also managed to get SED value into config.status.

comment:40 follow-up: Changed 2 years ago by 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 :-)

comment:41 in reply to: ↑ 40 Changed 2 years ago by fbissey

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 isuruf

Replying to fbissey:

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.

I'm pretty sure this doesn't work on OSX. See https://stackoverflow.com/a/5694430/4768820

comment:43 Changed 2 years ago by dimpase

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 isuruf

-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 git

  • Commit changed from 310ba06411ee413379c1ce3ca75b595ac83e8b0b to f9d6966f63a12991b81dce40ba69887cd9dacaa3

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

f9d6966get rid of -i option of sed

comment:46 Changed 2 years ago by dimpase

OK, done. Now it's POSIX, I think.

comment:47 Changed 2 years ago by dimpase

  • 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 git

  • Commit changed from f9d6966f63a12991b81dce40ba69887cd9dacaa3 to 9d9e0465310b67352105644e7e26eabb2684c84b

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

9d9e046fix a crucial typo in edited gsl.pc

comment:49 Changed 2 years ago by dimpase

the .pc syntax needs Requires rather than Require...

comment:50 Changed 2 years ago by dimpase

  • Status changed from needs_work to needs_review

now things work on Ubuntu, too.

comment:51 Changed 2 years ago by isuruf

  • Status changed from needs_review to positive_review

Tested that configure creates gsl.pc correctly in OSX.

comment:52 Changed 2 years ago by isuruf

  • Reviewers set to Isuru Fernando

comment:53 Changed 2 years ago by git

  • 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:

c031c9euse openblas.pc for blas and lapack, don't require atlas is to be used
b49f79emove blas handling to build/pkgs/
81bf522create cblas.pc link, too, not only blas and lapack
244f954spkg-configure for gsl
efd642ftypo in variable name
641e3fainstall and modify gsl.pc to use cblas
51bc622add extension '' to -i in sed calls
13a5434get SED into config.status
ac32b13get rid of -i option of sed
a180599fix a crucial typo in edited gsl.pc

comment:54 Changed 2 years ago by dimpase

  • 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 dimpase

  • Milestone changed from sage-9.0 to sage-9.1

comment:56 Changed 2 years ago by vbraun

  • Branch changed from u/dimpase/packages/gslconf to a18059933c6b6ecfe4e98a7c8e1ca6de3dc223c8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.