Opened 2 years ago

Closed 22 months ago

Last modified 15 months ago

#27270 closed enhancement (fixed)

spkg-configure.m4 for arb

Reported by: dimpase Owned by:
Priority: major Milestone: sage-8.9
Component: build Keywords: spkg-configure
Cc: embray, jdemeyer, thansen, fbissey Merged in:
Authors: Dima Pasechnik, Isuru Fernando Reviewers: François Bissey, Isuru Fernando
Report Upstream: Reported upstream. No feedback yet. Work issues:
Branch: 29d4ce2 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by 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

Attachments (2)

configure-320.tar.gz (109.4 KB) - added by dimpase 2 years ago.
configure-323.tar.gz (113.1 KB) - added by dimpase 2 years ago.

Download all attachments as: .zip

Change History (80)

comment:1 Changed 2 years ago by dimpase

  • Cc embray added
  • Description modified (diff)

comment:2 Changed 2 years ago by dimpase

  • Dependencies set to #27264

comment:3 Changed 2 years ago by dimpase

  • Branch set to u/dimpase/packages/arbconfig
  • Commit set to 46b529e44617eaa4873c32f77254018cf54f5f96
  • Status changed from new to needs_review

this does not implement --with-arb=..., as I hope it's not needed. And no other packages have --with-arb=, too.


Last 10 new commits:

50aebabimplement --with-mpfr=system/install
0fc433fupdate for Sage 8.7.rc0, a typo fix
38427e1spkg-configure for mpc, adjustments for its dependees
ec32471Merge branch 'u/dimpase/packages/mpc-config' of trac.sagemath.org:sage into mpf
91f423frebase, deal with --with-mpc
33edfa8Merge branch 'mpf' into flintconf - no spkg-configure yet
82772d7spkg-configure for flint
24f17d5testi for GC enabled
c1ac4f2proper nesting of tests
46b529espkg-configure for arb

comment:4 follow-up: Changed 2 years ago by dimpase

Tests pass, apart from this amusing doctest failure:

File "src/sage/misc/package.py", line 282, in sage.misc.package.installed_packages
Failed example:
    installed_packages()
Expected:
    {...'arb': ...'pynac': ...}
Got:
    {'.dummy': '',
     'alabaster': '0.7.12',
     'appnope': '0.1.0.p0',
...

Less amusing is that with arb manually installed to /usr/local on Gentoo Linux, I get a import error upon Sage startup, from deep inside sagelib.

I have to start Sage as LD_LIBRARY_PATH=/usr/local/lib64 ./sage, otherwise I get

ImportError: libarb.so.2: cannot open shared object file: No such file or directory

I do have /usr/local/lib64 in /etc/ld.so.conf, by the way. Does this mean we need do jump through some linking hoops here?

I certainly can add a proper runtime test in spkg-configure.m4, but why?

comment:5 in reply to: ↑ 4 Changed 2 years ago by dimpase

Replying to dimpase:

Less amusing is that with arb manually installed to /usr/local on Gentoo Linux, I get a import error upon Sage startup, from deep inside sagelib.

oops, my fault. Forgot to run ldconfig after the installation. (maybe arb should do it for you, cf. https://github.com/fredrik-johansson/arb/issues/273).

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

comment:6 Changed 2 years ago by git

  • Commit changed from 46b529e44617eaa4873c32f77254018cf54f5f96 to 961886d503232cdfe1a99472b72529dbdd00fb27

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

961886darb can be dummy, don't mention it in packages doctest

comment:7 Changed 2 years ago by dimpase

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

upstream report not directly relevant to this ticket, bit OK.

comment:8 Changed 2 years ago by embray

  • Milestone changed from sage-8.7 to sage-8.8

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

comment:9 Changed 2 years ago by git

  • Commit changed from 961886d503232cdfe1a99472b72529dbdd00fb27 to 7a9c76847329f746f1b784a2b660679be81f1575

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

7a9c768typo in parameter

comment:10 Changed 2 years ago by git

  • Commit changed from 7a9c76847329f746f1b784a2b660679be81f1575 to 1ed5dbbe84f0daa203d220e69c0af10662b8810c

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

1357befimplement --with-mpfr=system/install
e7d04e7spkg-configure for mpc, adjustments for its dependees
fb3aec7rebase, deal with --with-mpc
30b85f7spkg-configure for flint
e17167etesti for GC enabled
d7f3747proper nesting of tests
88209e0typo in parameter
329b835rebase, deal with --with-mpc
447ab2dspkg-configure for arb
1ed5dbbarb can be dummy, don't mention it in packages doctest

comment:11 Changed 2 years ago by dimpase

OK, so this branch has mpfr (#27258) mpc (#27259) ntl (#27265) flint (#27264) and, naturally, arb. Would be great to have it all reviewed in one go...

comment:12 Changed 2 years ago by git

  • Commit changed from 1ed5dbbe84f0daa203d220e69c0af10662b8810c to c09667d92605952c0976eab66bff0c8e29def116

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

c09667dconfigure bump

Changed 2 years ago by dimpase

comment:13 Changed 2 years ago by git

  • Commit changed from c09667d92605952c0976eab66bff0c8e29def116 to a247437766304ca3932ee779db4ae22c4a843cb5

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

a867838typo
c4e7778rebase, deal with --with-mpc
cd15ac1spkg-configure for arb
07276c8arb can be dummy, don't mention it in packages doctest
a247437configure bump

comment:14 Changed 2 years ago by git

  • Commit changed from a247437766304ca3932ee779db4ae22c4a843cb5 to 9de7419573417e1074b419fd966754a40e072503

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

0392e76rebase, deal with --with-mpc
73ce2a9spkg-configure for flint
37fa1a1testi for GC enabled
4f6434cproper nesting of tests
a672706typo in parameter
013dfbetypo
e698ffccorrected rebase
8fc4fb6add dependency checks, remove unneeded `-with` handling
1ad182dSAGE_CONFIGURE_MPFR, fix sage-env-config.in, arb can be dummy
9de7419added dependency check

comment:15 Changed 2 years ago by dimpase

rebased over #27264, added dependency check

comment:16 Changed 2 years ago by git

  • Commit changed from 9de7419573417e1074b419fd966754a40e072503 to 483233b26149276d13a3b6149bec98a5a007756c

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

d23d9dbadd SAGE_CONFIGURE_MPFR, fix sage-env-config.in
4222018testi for GC enabled
02dd624proper nesting of tests
4fb6a73SAGE_CONFIGURE_MPFR, fix sage-env-config.in, arb can be dummy
483233badded dependency check

Changed 2 years ago by dimpase

comment:17 Changed 2 years ago by dimpase

  • Description modified (diff)

this is now bundled with flint ticket. New configure tarball attached. I also squashed some commits, and rebased over 8.8.beta7

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

comment:18 Changed 2 years ago by git

  • Commit changed from 483233b26149276d13a3b6149bec98a5a007756c to fc876ed1d0d34b1ae206b0afc22a223f7a223bad

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

fc876edconfigure bump

comment:19 Changed 2 years ago by chapoton

How come that 8.8.rc0 needs configure-323, which is here, not yet merged ??

EDIT:

And how can this depend on a duplicate/invalid/wontfix ticket ??

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

comment:20 Changed 2 years ago by dimpase

version numbers of these tarballs are autogenerated. they are otherwise pretty much meaningless.

it is merely a bug in our release process that is needed to be workedaround, as builbots don’t have autotools and cannot generate the stuff on the fly...

comment:21 follow-up: Changed 2 years ago by vbraun

Needs a new confball. I'd suggest to merge this in 8.9.beta0 so hold the horses until 8.8 is out.

You should remove the dependency on tickets that cannot be merged, the release script will wait until the dependencies are merged which, right now, is never.

comment:22 Changed 2 years ago by vbraun

  • Status changed from needs_review to needs_work

comment:23 Changed 2 years ago by dimpase

  • Dependencies #27264 deleted

the branch here bundles the work done on #27264.

comment:24 Changed 2 years ago by git

  • Commit changed from fc876ed1d0d34b1ae206b0afc22a223f7a223bad to bd14dd2a4552c1afa0a09b6858a7e0cd504ade17

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

848279cadd SAGE_CONFIGURE_MPFR, fix sage-env-config.in
2bea28btesti for GC enabled
b5ac981proper nesting of tests
f81225aSAGE_CONFIGURE_MPFR, fix sage-env-config.in, arb can be dummy
bd14dd2added dependency check

comment:25 Changed 2 years ago by dimpase

rebased over 8.8.rc0 (without configure bump, for the time being)

comment:26 Changed 2 years ago by embray

  • Milestone changed from sage-8.8 to sage-8.9

Tickets still needing working or clarification should be moved to the next release milestone at the soonest (please feel free to revert if you think the ticket is close to being resolved).

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

Replying to vbraun:

Needs a new confball. I'd suggest to merge this in 8.9.beta0 so hold the horses until 8.8 is out.

Would you like to get #27978 first?

comment:28 Changed 23 months ago by git

  • Commit changed from bd14dd2a4552c1afa0a09b6858a7e0cd504ade17 to 8d1a64795b1e69d521250c7dd4901214772372e0

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

d91e84aadd SAGE_CONFIGURE_MPFR, fix sage-env-config.in
f7b77dftesti for GC enabled
22100a4proper nesting of tests
f7826c4SAGE_CONFIGURE_MPFR, fix sage-env-config.in, arb can be dummy
5cd4aacadded dependency check
8d1a647configure bump

comment:29 Changed 23 months ago by dimpase

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:30 Changed 23 months ago by git

  • Commit changed from 8d1a64795b1e69d521250c7dd4901214772372e0 to 45cb1e46fe6929a6760e1ed96277301a36ffa084

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

d0cdca7add SAGE_CONFIGURE_MPFR, fix sage-env-config.in
370e481testi for GC enabled
032620dproper nesting of tests
e6b3d53SAGE_CONFIGURE_MPFR, fix sage-env-config.in, arb can be dummy
13235f9added dependency check
45cb1e4configure version bump

comment:31 Changed 23 months ago by dimpase

  • Description modified (diff)

now rebased over 8.9.beta1, using sha1-based configure tarball

Please test!

comment:32 Changed 22 months ago by git

  • Commit changed from 45cb1e46fe6929a6760e1ed96277301a36ffa084 to 605260c4c5000d3fd793787f114abdc8e59ad870

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

46f2ba9add SAGE_CONFIGURE_MPFR, fix sage-env-config.in
456ad04testi for GC enabled
52c3ae4proper nesting of tests
37d2d12SAGE_CONFIGURE_MPFR, fix sage-env-config.in, arb can be dummy
1f53bebadded dependency check
32f294bsage should be called with ./ in bootstrap
c758851Debian names the arb library differently
54d45bamention flint and arb (and one more forgotten package) for debian
605260cconfigure version bump

comment:33 Changed 22 months ago by dimpase

  • Description modified (diff)

rebased, config bump, added another name for the arb library (in Debian), mention it and flint in docs

comment:34 Changed 22 months ago by dimpase

  • Cc jdemeyer added
  • Status changed from needs_review to needs_info

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

comment:35 Changed 22 months ago by dimpase

I can apply

  • src/module_list.py

    a b library_order_list = aliases["SINGULAR_LIBRARIES"] + [ 
    7979    "ec", "ecm",
    8080] + aliases["LINBOX_LIBRARIES"] + aliases["FFLASFFPACK_LIBRARIES"] + aliases["GSL_LIBRARIES"] + [
    8181    "pari", "flint", "ratpoints", "ecl", "glpk", "ppl",
    82     "arb", "mpfi", "mpfr", "mpc", "gmp", "gmpxx",
     82    "flint-arb", "mpfi", "mpfr", "mpc", "gmp", "gmpxx",
    8383    "brial",
    8484    "brial_groebner",
    8585    "m4rie",
    ext_modules = [ 
    765765
    766766    Extension("sage.matrix.matrix_complex_ball_dense",
    767767              ["sage/matrix/matrix_complex_ball_dense.pyx"],
    768               libraries=['arb']),
     768              libraries=['flint-arb']),
    769769
    770770    Extension('sage.matrix.matrix_complex_double_dense',
    771771              sources = ['sage/matrix/matrix_complex_double_dense.pyx']),

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

comment:36 Changed 22 months ago by dimpase

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

comment:37 Changed 22 months ago by dimpase

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

comment:38 Changed 22 months ago by git

  • Commit changed from 605260c4c5000d3fd793787f114abdc8e59ad870 to 89d227c31890df899fc4531c2787fd2419759e3d

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

89d227cadd the name of arb dylib to cython_aliases

comment:39 Changed 22 months ago by git

  • Commit changed from 89d227c31890df899fc4531c2787fd2419759e3d to 4c245ced57975592747387cd0c1ee16543b8bb47

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

fd96a38use AC_CHECK_LIB() calls, as they can be nested
4c245ceconfigure version bump

comment:40 Changed 22 months ago by dimpase

  • Cc thansen added
  • Description modified (diff)
  • Status changed from needs_info to needs_review

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

comment:41 Changed 22 months ago by dimpase

  • Cc fbissey added
  • Description modified (diff)

comment:42 follow-up: Changed 22 months ago by fbissey

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.

comment:43 follow-up: Changed 22 months ago by fbissey

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.

comment:44 in reply to: ↑ 42 ; follow-up: Changed 22 months ago by dimpase

Replying to fbissey:

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.

comment:45 in reply to: ↑ 43 Changed 22 months ago by dimpase

Replying to fbissey:

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

comment:46 in reply to: ↑ 44 ; follow-up: Changed 22 months ago by fbissey

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.

comment:47 in reply to: ↑ 46 Changed 22 months ago by dimpase

Replying to fbissey:

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

comment:48 follow-up: Changed 22 months ago by fbissey

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.

comment:49 in reply to: ↑ 48 ; follow-up: Changed 22 months ago by dimpase

Replying to fbissey:

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.

comment:50 in reply to: ↑ 49 Changed 22 months ago by fbissey

Replying to dimpase:

Replying to fbissey:

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.

comment:51 follow-up: Changed 22 months ago by 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.

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.

Last edited 22 months ago by dimpase (previous) (diff)

comment:52 Changed 22 months ago by fbissey

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

@thansen 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.

comment:53 follow-up: Changed 22 months ago by 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.

comment:54 in reply to: ↑ 53 Changed 22 months ago by arojas

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.

comment:55 Changed 22 months ago by fbissey

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

comment:56 Changed 22 months ago by dimpase

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.

comment:57 Changed 22 months ago by fbissey

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.

comment:58 Changed 22 months ago by git

  • Commit changed from 4c245ced57975592747387cd0c1ee16543b8bb47 to f463852fe18b6974396b442f2ab1b2300cf5547e

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

cf8501cpass the name of arb dylib via an environment variable
f463852new configure version

comment:59 Changed 22 months ago by dimpase

  • Description modified (diff)

comment:60 Changed 22 months ago by dimpase

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

comment:61 Changed 22 months ago by arojas

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

comment:62 in reply to: ↑ 51 ; follow-up: Changed 22 months ago by gh-timokau

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.

comment:63 in reply to: ↑ 62 Changed 22 months ago by dimpase

Replying to gh-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.

comment:64 Changed 22 months ago by fbissey

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.

comment:65 Changed 22 months ago by dimpase

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

comment:66 Changed 22 months ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

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.

comment:67 Changed 22 months ago by git

  • Commit changed from f463852fe18b6974396b442f2ab1b2300cf5547e to 166e00c3652a3d1c43fe4616b2d8dbb888f6f879
  • 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. New commits:

2c4d350set SAGE_ARB_LIBRARY correctly in every scenario
166e00cbump up configure

comment:68 Changed 22 months ago by dimpase

  • Description modified (diff)

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

Last edited 22 months ago by dimpase (previous) (diff)

comment:69 Changed 22 months ago by arojas

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

comment:70 Changed 22 months ago by git

  • Commit changed from 166e00c3652a3d1c43fe4616b2d8dbb888f6f879 to 411d4a77fc8a691ed09cd14b9605d59dde292850

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

73dd667fallback for ARB_LIBRARY in src/sage/env.py
411d4a7bump up configure

comment:71 Changed 22 months ago by dimpase

  • Description modified (diff)

Done the fallback.

comment:72 Changed 22 months ago by isuruf

  • Branch changed from u/dimpase/packages/arbconfig to u/isuruf/arbconfig
  • Commit changed from 411d4a77fc8a691ed09cd14b9605d59dde292850 to 9f79388b5a7ba7dac32884f45cd792496baed4db

comment:73 Changed 22 months ago by isuruf

  • Authors changed from Dima Pasechnik to Dima Pasechnik, Isuru Fernando
  • Reviewers changed from François Bissey to François Bissey, Isuru Fernando
  • Status changed from needs_review to positive_review

comment:74 Changed 22 months ago by dimpase

  • Description modified (diff)

comment:75 Changed 22 months ago by dimpase

  • Branch changed from u/isuruf/arbconfig to u/dimpase/packages/arbconfig
  • Commit changed from 9f79388b5a7ba7dac32884f45cd792496baed4db to 29d4ce2b95a7a721afc22db4633995c4a93d7f31

Last 10 new commits:

5aad412sage should be called with ./ in bootstrap
085f518Debian names the arb library differently
d24ae0fmention flint and arb (and one more forgotten package) for debian
2e04b25add the name of arb dylib to cython_aliases
0e4e71ause AC_CHECK_LIB() calls, as they can be nested
3e8bd98pass the name of arb dylib via an environment variable
ce2b9e0set SAGE_ARB_LIBRARY correctly in every scenario
12f5129fallback for ARB_LIBRARY in src/sage/env.py
b5de899make code simpler
29d4ce2bump up configure

comment:76 Changed 22 months ago by dimpase

merged u/isuruf/arbconfig (which erroneously contained just 1 commit), rebased over 8.9.beta3, bumpled up configure

comment:77 Changed 22 months ago by vbraun

  • Branch changed from u/dimpase/packages/arbconfig to 29d4ce2b95a7a721afc22db4633995c4a93d7f31
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:78 Changed 15 months ago by mkoeppe

  • Commit 29d4ce2b95a7a721afc22db4633995c4a93d7f31 deleted

Recognizing flint-arb here broke the optional package e-antic (used by normaliz). Follow-up = #27952.

Note: See TracTickets for help on using tickets.