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

spkg-configure for GSL #28879

Closed
dimpase opened this issue Dec 13, 2019 · 59 comments
Closed

spkg-configure for GSL #28879

dimpase opened this issue Dec 13, 2019 · 59 comments

Comments

@dimpase
Copy link
Member

dimpase commented Dec 13, 2019

Depends on #27870

CC: @embray @kiwifb @isuruf @mkoeppe

Component: build: configure

Work Issues: install modified gsl.pc

Author: Dima Pasechnik

Branch/Commit: a180599

Reviewer: Isuru Fernando

Issue created by migration from https://trac.sagemath.org/ticket/28879

@dimpase dimpase added this to the sage-9.1 milestone Dec 13, 2019
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 14, 2019

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 14, 2019

Changed commit from f8833aa to 3a95431

@isuruf
Copy link
Member

isuruf commented Dec 14, 2019

comment:4

Did you check this with an underlinked GSL (A blas library is not linked in) or a fully linked one like Sage's GSL?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 14, 2019

Changed commit from 3a95431 to 137c972

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 14, 2019

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

137c972typo in variable name

@dimpase
Copy link
Member Author

dimpase commented Dec 15, 2019

comment:6

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

@kiwifb
Copy link
Member

kiwifb commented Dec 15, 2019

comment:7

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

@isuruf
Copy link
Member

isuruf commented Dec 15, 2019

comment:8

What happens when gslcblas and openblas are both linked in?

@kiwifb
Copy link
Member

kiwifb commented Dec 15, 2019

comment:9

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.

@isuruf
Copy link
Member

isuruf commented Dec 15, 2019

comment:10

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.

@kiwifb
Copy link
Member

kiwifb commented Dec 15, 2019

comment:11
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.

@isuruf
Copy link
Member

isuruf commented Dec 15, 2019

comment:12

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

@isuruf
Copy link
Member

isuruf commented Dec 15, 2019

comment:13

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)

@kiwifb
Copy link
Member

kiwifb commented Dec 15, 2019

comment:14

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.

@isuruf
Copy link
Member

isuruf commented Dec 15, 2019

comment:15

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)

@kiwifb
Copy link
Member

kiwifb commented Dec 15, 2019

comment:16

Remove GSL_CBLAS_LIB and add a line

requires: cblas

which will pull cblas.pc all by itself.

@dimpase
Copy link
Member Author

dimpase commented Dec 15, 2019

comment:17

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

@kiwifb
Copy link
Member

kiwifb commented Dec 15, 2019

comment:18

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.

@dimpase
Copy link
Member Author

dimpase commented Dec 15, 2019

Work Issues: install modified gsl.pc

@dimpase
Copy link
Member Author

dimpase commented Dec 15, 2019

comment:20

Replying to @kiwifb:

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

@dimpase
Copy link
Member Author

dimpase commented Dec 20, 2019

comment:21

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.

@isuruf
Copy link
Member

isuruf commented Dec 20, 2019

comment:22

(I suppose it's the same with Sage-supplied GSL and OpenBLAS)

No, see https://github.com/sagemath/sage-prod/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.

@dimpase
Copy link
Member Author

dimpase commented Dec 20, 2019

comment:23

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 21, 2019

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

f9d6966get rid of -i option of sed

@dimpase
Copy link
Member Author

dimpase commented Dec 21, 2019

comment:46

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

@dimpase
Copy link
Member Author

dimpase commented Dec 21, 2019

comment:47

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 21, 2019

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

9d9e046fix a crucial typo in edited gsl.pc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 21, 2019

Changed commit from f9d6966 to 9d9e046

@dimpase
Copy link
Member Author

dimpase commented Dec 21, 2019

comment:49

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

@dimpase
Copy link
Member Author

dimpase commented Dec 21, 2019

comment:50

now things work on Ubuntu, too.

@isuruf
Copy link
Member

isuruf commented Dec 21, 2019

comment:51

Tested that configure creates gsl.pc correctly in OSX.

@isuruf
Copy link
Member

isuruf commented Dec 21, 2019

Reviewer: Isuru Fernando

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 28, 2019

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 28, 2019

Changed commit from 9d9e046 to a180599

@dimpase
Copy link
Member Author

dimpase commented Dec 28, 2019

comment:54

trivial rebase over updated #27870

@dimpase dimpase modified the milestones: sage-9.1, sage-9.0 Dec 28, 2019
@dimpase dimpase modified the milestones: sage-9.0, sage-9.1 Jan 1, 2020
@vbraun
Copy link
Member

vbraun commented Jan 9, 2020

Changed branch from u/dimpase/packages/gslconf to a180599

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

4 participants