spkg-configure.m4 for arb

[dimpase]

SAGE_SPKG_CONFIGURE([arb], [
    AC_CHECK_HEADER(arb.h, [], [sage_spkg_install_arb=yes])
dnl below function added in version 2.16 of arb 
    AC_SEARCH_LIBS([acb_mat_eig_simple], [arb], [break], [sage_spkg_install_arb=yes])

we also take care of non-standard naming of arb's dylib here.

configure tarball http://users.ox.ac.uk/~coml0531/sage/configure-b5de8991395bbf58e663e69d6250de656897c4e2.tar.gz

Upstream: Reported upstream. No feedback yet.

CC: @embray @jdemeyer @tobihan @kiwifb

Component: build

Author: Dima Pasechnik, Isuru Fernando

Branch: 29d4ce2

Reviewer: François Bissey, Isuru Fernando

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

[dimpase]

Description changed:

--- 
+++ 
@@ -7,4 +7,4 @@

configure tarball -http://users.ox.ac.uk/~coml0531/sage/configure-13235f9997462c2349a452f2dfb6b2097deb7d56.tar.gz +http://users.ox.ac.uk/~coml0531/sage/configure-54d45ba31e6d669ec33a80fefc6be20ad40d9f21.tar.gz

[dimpase]

comment:34

with the latest branch on Debian buster, building sage/libs/arb/arith.so against system's arb (so it needs -lflint-arb and not -larb breaks with a linking error (as there is no libarb)

[sagelib-8.9.beta2] gcc -pthread -shared -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-2.7/build/cythonized/sage/libs/arb/arith.o -L/home/dimpase/sage/local/lib -L/home/dimpase/sage/local/lib -lflint -larb -lgmp -lpython2.7 -o build/lib.linux-x86_64-2.7/sage/libs/arb/arith.so
[sagelib-8.9.beta2] /usr/bin/ld: cannot find -larb

It apparently derives -larb somewhere in Cython, but where, and is there a way to override it?

Note that -lflint-arb is known to the global makefile:

build/make/Makefile-auto:LIBS = -lrw -lreadline -lmpc -lgf2x -lcurl -lbz2 -lflint-arb -lflint -lmpfr -lgmp -lm  -lntl

[dimpase]

comment:35

I can apply

--- a/src/module_list.py
+++ b/src/module_list.py
@@ -79,7 +79,7 @@ library_order_list = aliases["SINGULAR_LIBRARIES"] + [
     "ec", "ecm",
 ] + aliases["LINBOX_LIBRARIES"] + aliases["FFLASFFPACK_LIBRARIES"] + aliases["GSL_LIBRARIES"] + [
     "pari", "flint", "ratpoints", "ecl", "glpk", "ppl",
-    "arb", "mpfi", "mpfr", "mpc", "gmp", "gmpxx",
+    "flint-arb", "mpfi", "mpfr", "mpc", "gmp", "gmpxx",
     "brial",
     "brial_groebner",
     "m4rie",
@@ -765,7 +765,7 @@ ext_modules = [

     Extension("sage.matrix.matrix_complex_ball_dense",
               ["sage/matrix/matrix_complex_ball_dense.pyx"],
-              libraries=['arb']),
+              libraries=['flint-arb']),

     Extension('sage.matrix.matrix_complex_double_dense',
               sources = ['sage/matrix/matrix_complex_double_dense.pyx']),

and then it builds, but is this the correct way?

[dimpase]

comment:36

oops, scratch this, this change does not help, it appears there is some divining of -larb by Cython(?)

[dimpase]

comment:37

OK, so also some #distutils entries need to be modified. Debian applies https://salsa.debian.org/science-team/sagemath/blob/master/debian/patches/d0-arb.patch

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

89d227c

add the name of arb dylib to cython_aliases

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 605260c to 89d227c

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 89d227c to 4c245ce

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

fd96a38	use AC_CHECK_LIB() calls, as they can be nested
4c245ce	configure version bump

[dimpase]

comment:40

please review - should also work on Debian now...

[dimpase]

Description changed:

--- 
+++ 
@@ -6,5 +6,5 @@
     AC_SEARCH_LIBS([acb_mat_eig_simple], [arb], [break], [sage_spkg_install_arb=yes])

-configure tarball -http://users.ox.ac.uk/~coml0531/sage/configure-54d45ba31e6d669ec33a80fefc6be20ad40d9f21.tar.gz +configure tarball +http://users.ox.ac.uk/~coml0531/sage/configure-fd96a3818dc2ceb473983b2881d11da075e8d119.tar.gz

[dimpase]

Description changed:

--- 
+++ 
@@ -6,5 +6,7 @@
     AC_SEARCH_LIBS([acb_mat_eig_simple], [arb], [break], [sage_spkg_install_arb=yes])

+we also take care of non-standard naming of arb's dylib here. + configure tarball http://users.ox.ac.uk/~coml0531/sage/configure-fd96a3818dc2ceb473983b2881d11da075e8d119.tar.gz

[kiwifb]

comment:42

Quick impressions on the arb part. I would test for flint-arb first, otherwise you could get the other arb on a debian system. I would use AC_SEARCH_LIBS see https://autotools.io/autoconf/finding.html. When you use AC_CHECK_LIBS or AC_SEARCH_LIBS the library found will be added to LIBS and that may be used for all linkings independently of other settings - of course that's what would happen in a fully autotooled package. The question is whether all those SAGE_${PKG}_LIBRARY variables are needed at all.

[kiwifb]

comment:43

I have now read the stuff about src/sage/env.py.in and this is a no for me. I don't run configure to build the sage python library and don't intend to if I can help it. I certainly can prepare the sources if you force me too but I'd rather not have to do that.

[dimpase]

comment:44

Replying to @kiwifb:

Quick impressions on the arb part. I would test for flint-arb first, otherwise you could get the other arb on a debian system.

not really - it tests for a specific function from the "right" arb, I cannot imagine the other arb having it.

I would use AC_SEARCH_LIBS see https://autotools.io/autoconf/finding.html.

I tried that, but then you have to analyse the result of its run, and this leads to arcane code (one needs to strip -l, etc...)

When you use AC_CHECK_LIBS or AC_SEARCH_LIBS the library found will be added to LIBS and that may be used for all linkings independently of other settings - of course that's what would happen in a fully autotooled package. The question is whether all those SAGE_${PKG}_LIBRARY variables are needed at all.

The problem is that sagelib isn't autotooled, and so one needes to get arb/flint-arb strings into # distutils libraries= directives in Cython header files.

[dimpase]

comment:45

Replying to @kiwifb:

I have now read the stuff about src/sage/env.py.in and this is a no for me. I don't run configure to build the sage python library and don't intend to if I can help it. I certainly can prepare the sources if you force me too but I'd rather not have to do that.

well, I don't see a point of not running ./configure. Perhaps it was fashionable in Python world 10-15 years ago :-)

[kiwifb]

comment:46

OK you are right about the library order. Because of the check you make it doesn't matter.

It is an interesting comment about configure. Unfortunately, as far as I know running configure for a python project is still not a done thing. It doesn't mesh with the other elements of the python toolchain. There is certainly room for some configurability option in sage in regards to the optional libraries. But autotool's configure is definitely not the python way. Apart from cypari I cannot think of another python package using configure.

Let's be honest, a small configure script for sage's optional stuff and the possibly unusual stuff would be fine to run. The full configure script for the whole of the distribution is inappropriate. It all goes back, once again, to "sagelib" being treated special instead of as a normal package. So many things would have to click in place once you do that. Other superbuild systems like the one for paraview don't treat the main target in a special way like sage's does.

[dimpase]

comment:47

Replying to @kiwifb:

It is an interesting comment about configure. Unfortunately, as far as I know running configure for a python project is still not a done thing. It doesn't mesh with the other elements of the python toolchain. There is certainly room for some configurability option in sage in regards to the optional libraries. But autotool's configure is definitely not the python way. Apart from cypari I cannot think of another python package using configure.

Have you ever heard of cpython? :-)

Let's be honest, a small configure script for sage's optional stuff and the possibly unusual stuff would be fine to run. The full configure script for the whole of the distribution is inappropriate. It all goes back, once again, to "sagelib" being treated special instead of as a normal package. So many things would have to click in place once you do that.

well, writing thousands of lines of Python to do configuration and building is what is (mal)practiced by many Python packages for reasons I don't really get.

Other superbuild systems like the one for paraview don't treat the main target in a special way like sage's does.

I suggest you might look at the build system of Tensorflow (it uses Bazel ;-))

[kiwifb]

comment:48

cpython is the language. While it would be interesting to see if python could bootstrap itself, I am considering that you are stretching your credibility there.

bazel and the way it is used in tensorflow [heck the whole way tensorflow is build] is a whole private hell of its own. Let's not go there. Invoking a worst project does not make yours any better. That reminds of my daughter pretending her room isn't messy because she saw a bigger mess at one of her friend's house.

[dimpase]

comment:49

Replying to @kiwifb:

cpython is the language. While it would be interesting to see if python could bootstrap itself, I am considering that you are stretching your credibility there.

I don't claim any credibility, but merely observe that Python 3 is moving quite a bit of its build system from setup.py to autotools.

I don't get it why pre-build manipulation of Python/Cython code must be done in Python or shell, and not with more suitable tools. Sounds like religion rather than common sense to me.

[kiwifb]

comment:50

Replying to @dimpase:

Replying to @kiwifb:

cpython is the language. While it would be interesting to see if python could bootstrap itself, I am considering that you are stretching your credibility there.

I don't claim any credibility, but merely observe that Python 3 is moving quite a bit of its build system from setup.py to autotools.

I don't get it why pre-build manipulation of Python/Cython code must be done in Python or shell, and not with more suitable tools. Sounds like religion rather than common sense to me.

I haven't seen that. But that is welcome in some way.

But the big issue we have in distro if I can pinpoint it better is not necessarily that you want to run configure. It is that you run configure for sage the distro and use it for sage the package. "sagelib" should be its own self contained package, and if it had its own configure why not. But leveraging your monster build system in the way you do, is not fine. We don't want it.

[dimpase]

comment:51

Unvendoring everything that was sheepishly vendored into Sage over the years is an essential step towards having a self contained package for sagelib alone.

This ticket is a part of the unvendoring process. One way or another, the problem here is that arb library does not have pkg-config support, and yes, we want to support !Debian/Ubuntu arb package with non-standard naming.

[kiwifb]

comment:52

I understand perfectly. Since the controversial changes are for the sake of debian, I think a debian developer should give their opinion.

@tobihan if you would like to oblige or pass it to someone else in debian for review.

As I said I won't review this any further but won't oppose it further either. I have my own contingency plans.

[dimpase]

comment:53

alternatively I can store an environment variable in src/bin/sage-env-config (which, as you know, is also created by ./configure :-)) and then access it from src/sage/env.py to populate ARB_LIBRARY.

This would avoid creating src/sage/env.py.in.

If this is more acceptable (after all, distributions already deal with rewriting src/bin/sage-env-config.in) let me know, I can do this change.

[antonio-rojas]

comment:54

Replying to @dimpase:

alternatively I can store an environment variable in src/bin/sage-env-config (which, as you know, is also created by ./configure :-)) and then access it from src/sage/env.py to populate ARB_LIBRARY.

This would avoid creating src/sage/env.py.in.

If this is more acceptable (after all, distributions already deal with rewriting src/bin/sage-env-config.in) let me know, I can do this change.

+1 to this alternative. Setting env variables at build time is much better than having to tamper with source files.

[kiwifb]

comment:55

While it feels better to deal with an environment variable I wonder if it is better to elect to touch the source code here.

People can compile cython code from sage against sage libraries. If we don't touch the code minimally and someone wants to compile some cython code from sage involving arb, they will have to define the variable unless we put it permanently in the environment in some way (which is also doable of course but I stopped doing that around sage-5.0 I think).

[dimpase]

comment:56

Well, you tell me what you prefer. I think that to compile cython code from sage against sage libraries requires pkg-config knowing about SAGE_LOCAL/lib/pkgconfig directory to pick up settings for quite a number of Sage libraries, otherwise it's doomed to fail. In effect, this means either using Sage's pkg-config or providing a properly set PKG_CONFIG_PATH. That is you cannot get away from setting some environment variables.

In fact I plan to work on spkg-configure.m4 for pkgconf Sage package (cf. #27827, a part of #27330) and to install PKG_CONFIG_PATH into src/bin/sage-env-config in case a system has pkg-config installed.

[kiwifb]

comment:57

If it currently wouldn't work for cython code, then environment is best - it doesn't break something currently working. Thank you for pulling through my painful exhortations with something palatable.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

cf8501c	pass the name of arb dylib via an environment variable
f463852	new configure version

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 4c245ce to f463852

[dimpase]

Description changed:

--- 
+++ 
@@ -9,4 +9,5 @@
 we also take care of non-standard naming of arb's dylib here.

 configure tarball
-http://users.ox.ac.uk/~coml0531/sage/configure-fd96a3818dc2ceb473983b2881d11da075e8d119.tar.gz
+http://users.ox.ac.uk/~coml0531/sage/configure-cf8501c4fe6a1b2b7435c00957765230e0cc0f5a.tar.gz
+

[dimpase]

comment:60

OK, it's changed as discussed. Please review.

[antonio-rojas]

comment:61

FWIW sagelib builds fine against system packages now by just setting SAGE_ARB_LIBRARY (and not running configure)

[timokau]

comment:62

For what its worth, I'm fine with setting an environment variable. There are already lots of env vars to set, one more or less won't make a difference. And I'm also glad we can get around configure. Thanks François for taking the distro standpoint here :)

Replying to @dimpase:

Unvendoring everything that was sheepishly vendored into Sage over the years is an essential step towards having a self contained package for sagelib alone.

I feel like you're arguing slightly different things here. Treating the sagelib package as just another package could in principle be done without removing any vendoring. You keep the current build system, add a sagelib package under build, add basically everything as a dependency for it. sagelib can then have its own ./configure, which configure just sagelib and not everything else. Distros would have not problem running that.

[dimpase]

comment:63

Replying to @timokau:

Replying to @dimpase:

Unvendoring everything that was sheepishly vendored into Sage over the years is an essential step towards having a self contained package for sagelib alone.

I feel like you're arguing slightly different things here. Treating the sagelib package as just another package could in principle be done without removing any vendoring.

the main configure configures "just" sagelib. What one has in build/pkgs/ are vendored dependencies, which ought to disappear. These little (or not so little) spkg-configure.m4 scripts are part of the sagelib configure, they check that the deps are satisfied. They have nothing to do with building the relevant deps (they deal with NOT building them).

You keep the current build system, add a sagelib package under build, add basically everything as a dependency for it. sagelib can then have its own ./configure, which configure just sagelib and not everything else. Distros would have not problem running that.

[kiwifb]

comment:64

I'll be waiting to see if the patchbot turns green, but in principle I am OK with this and will give it a positive review if no problems turn up.

[dimpase]

comment:65

not sure whether patchbots are able to deal well with packages changing configure; basically, for a proper test one needs to have appropriate versions of arb etc installed (so e.g. Debian 10 and Gentoo should be OK - and these are what I tested them on, as well as on other platforms, such as OSX with Homebrew).

[kiwifb]

Reviewer: François Bissey

[kiwifb]

comment:66

I was hoping the changes in the tarball handling would have helped with that. But obviously not. It is good enough, in my opinion, to send to the bots to figure if corner cases have been missed.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from f463852 to 166e00c

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

2c4d350	set SAGE_ARB_LIBRARY correctly in every scenario
166e00c	bump up configure

[dimpase]

comment:68

fixing a "minor" m4 error in the latest change.

[dimpase]

Description changed:

--- 
+++ 
@@ -9,5 +9,4 @@
 we also take care of non-standard naming of arb's dylib here.

 configure tarball
-http://users.ox.ac.uk/~coml0531/sage/configure-cf8501c4fe6a1b2b7435c00957765230e0cc0f5a.tar.gz
-
+http://users.ox.ac.uk/~coml0531/sage/configure-2c4d3506fbb122678f9be24139209e8ae2165728.tar.gz

[antonio-rojas]

comment:69

Could you add a fallback for SAGE_ARB_LIBRARY in env.py? This wouldn't make any difference for users of the Sage build system (since it is set in sage-env-config) or distros that rename the arb library (which need to set it anyway), and would save distros that use the standard arb library name from having to worry about it

--- src/sage/env.py.orig        2019-07-19 14:05:32.947459540 +0200
+++ src/sage/env.py     2019-07-19 14:15:14.193540393 +0200
@@ -406,6 +406,8 @@
     # This is not a problem in practice since LinBox depends on
     # fflas-ffpack and fflas-ffpack does add such a C++11 flag.
     aliases["LINBOX_CFLAGS"].append("-std=gnu++11")
-    aliases["ARB_LIBRARY"] = os.environ['SAGE_ARB_LIBRARY']
-
+    if os.environ.get('SAGE_ARB_LIBRARY'):
+        aliases["ARB_LIBRARY"] = os.environ['SAGE_ARB_LIBRARY']
+    else:
+        aliases["ARB_LIBRARY"] = 'arb'
     return aliases

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 166e00c to 411d4a7

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

73dd667	fallback for ARB_LIBRARY in src/sage/env.py
411d4a7	bump up configure

[dimpase]

comment:71

Done the fallback.

[dimpase]

Description changed:

--- 
+++ 
@@ -9,4 +9,4 @@
 we also take care of non-standard naming of arb's dylib here.

 configure tarball
-http://users.ox.ac.uk/~coml0531/sage/configure-2c4d3506fbb122678f9be24139209e8ae2165728.tar.gz
+http://users.ox.ac.uk/~coml0531/sage/configure-73dd6671794c7bf5e408f5cba884df00ff341c9d.tar.gz

[isuruf]

Changed branch from u/dimpase/packages/arbconfig to u/isuruf/arbconfig

[isuruf]

Changed commit from 411d4a7 to 9f79388