Opened 2 years ago

Closed 22 months ago

#27258 closed enhancement (duplicate)

spkg-configure.m4 for mpfr

Reported by: dimpase Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: build: configure Keywords: spkg-configure mpfr
Cc: embray, fbissey Merged in:
Authors: Dima Pasechnik, Erik Bray Reviewers: Erik Bray, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 0329fd9 (Commits, GitHub, GitLab) Commit:
Dependencies: #27641 Stopgaps:

Status badges

Description (last modified by dimpase)

recognise system's mpfr version via mpfr_free_pool presence, generate SAGE_MPFR_PREFIX to be used with --with-mpfr= options.

the branch to be merged as a part of #27822

Attachments (1)

configure-318.tar.gz (106.7 KB) - added by dimpase 2 years ago.
updated configure spkg

Download all attachments as: .zip

Change History (70)

comment:1 Changed 2 years ago by dimpase

  • Keywords spkg-configure added

comment:2 Changed 2 years ago by mmezzarobba

Two stupid questions (I don't think I'm good enough with autoconf to review this ticket, but I'd like to understand the mechanism):

  • Is there a particular reason for using AC_SEARCH_LIBS rather that AC_CHECK_LIB here?
  • What is the break instruction for? Will the generated code be part of a loop that tries several options or something like that? I couldn't locate the corresponding logic.

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

from autoconf docs:

AC_CHECK_LIB requires some care in usage, and should be avoided in some common cases. Many standard functions like gethostbyname appear in the standard C library on some hosts, and in special libraries like nsl on other hosts. On some hosts the special libraries contain variant implementations that you may not want to use. These days it is normally better to use AC_SEARCH_LIBS([gethostbyname], [nsl]) instead of AC_CHECK_LIB([nsl], [gethostbyname]).

I guess Erik can comment better than me about the use of break.

comment:4 in reply to: ↑ 3 Changed 2 years ago by mmezzarobba

Replying to dimpase:

from autoconf docs:

AC_CHECK_LIB requires some care in usage, and should be avoided in some common cases. Many standard functions like gethostbyname appear in the standard C library on some hosts, and in special libraries like nsl on other hosts. On some hosts the special libraries contain variant implementations that you may not want to use. These days it is normally better to use AC_SEARCH_LIBS([gethostbyname], [nsl]) instead of AC_CHECK_LIB([nsl], [gethostbyname]).

Thanks. But I think this is purely about the case where the same function can be found in several different libraries.

comment:5 Changed 2 years ago by dimpase

mpfrcx's spkg-install hardcodes the location of mpfr to SAGE_LOCAL. So this will need to be fixed.

comment:6 Changed 2 years ago by dimpase

  • Branch set to u/dimpase/packages/mpfr-config
  • Commit set to 5e1a30bbbcc673061515fbc839690ae19aa7d1a8
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

c4e42e6spkg-configure for GMP
b070088Move all the --with-mp logic into the spkg-configure.m4 for MPIR so that it
3b6eebdReplace various --with-gmp=$SAGE_LOCAL flags in spkg-installs with a
b80bf72Reworked this a bit more
0ca3f56fix typo
06798caadded a bit more explanation
b592d77correct logic for SAGE_GMP_PREFIX etc
5057680add the AX_ABSOLUTE_HEADER macro
101537biml in particular is very picky about being given an absolute path to the
5e1a30bspkg-configure for mpfr, adjustments for its dependees

comment:7 Changed 2 years ago by dimpase

only the last commit is new, the rest are from #27212.

comment:8 Changed 2 years ago by embray

Are you sure in all these cases that it's safe to pass --with-mpfr="$SAGE_MPFR_PREFIX" when $SAGE_MPFR_PREFIX is empty?

I found that a number of packages do not have sensible defaults for --with-gmp= passed an empty value, which is why I had to define

if [ -n "$SAGE_GMP_PREFIX" ]; then
    export SAGE_CONFIGURE_GMP="--with-gmp=$SAGE_GMP_PREFIX"
fi

And then pass various configure scripts $SAGE_CONFIGURE_GMP instead of --with-gmp=$SAGE_GMP_PREFIX.

Not saying that's always necessary though; might depend on the package and how it handles an empty value for the flag :(

comment:9 Changed 2 years ago by embray

On Ubuntu 16.04 I tried ./bootstrap && ./configure. On first attempt mpfr.h was not detected. Ok, apt-get install libmpfr-dev and tried ./configure again and it found the headers but failed to find mpfr_free_pool. Apparently the system package (despite the package being named "libmpfr4") is version 3.1.4 of MPFR, so maybe it doesn't have mpfr_free_pool?

Why was mpfr_free_pool chosen? Do we have a specific dependency on it? Or is it possible in principle to use version 3.1.x? If not then that's fine; if the system package is too old it's too old, so we don't use it. I'm just curious.

comment:10 Changed 2 years ago by embray

In Ubuntu 18.04 (bionic) it now has "libmpfr6" which is v4.0.1 so I might try testing against that next.

comment:11 Changed 2 years ago by embray

Got all the way through building sagelib with MPFR 3.1.4, but then at runtime I get

ImportError: /home/sage/sage/local/lib/python2.7/site-packages/sage/rings/real_mpfr.so: undefined symbol: mpfr_rootn_ui

where mpfr_rootn_ui is a function introduced in 4.0.0, as is mpfr_free_pool. ISTM that specific requirement could be worked around, but I'm happy to just say "ok 4.0.0 is the minimum required MPFR" for now.

I'll make an Ubuntu 18.04 container and try it in there.

comment:12 Changed 2 years ago by dimpase

Are you saying that the test for mpfr_free_pool actually succeeds on MPFR 3.1.4? OK, let's switch to testing for mpfr_rootn_ui then.

comment:13 Changed 2 years ago by embray

Oops, no, I just changed it to mpfr_init for the sake of testing against 3.1.4. mpfr_free_pool is fine (though mpfr_rootn_ui has the advantage of explicitly being used by sage).

comment:14 Changed 2 years ago by embray

Completed a successful build from scratch and make ptestlong on Ubuntu 18.04 using the system mpfr. So maybe this is OK as-is.

comment:15 Changed 2 years ago by dimpase

I've also tested this on Gentoo, on Debian.

comment:16 Changed 2 years ago by dimpase

  • Status changed from needs_review to needs_work

Oops, this does break (optional) mpfrcx, one of 4 packages that depend on mpfr. So indeed most likely it needs akin to $SAGE_CONFIGURE_GMP.

comment:17 Changed 2 years ago by dimpase

and I forgot about the need to SAGE_MPRF_* things in sage-env-config.in. This means that the tests ran with these being empty - mostly OK for system libs, but should be deadly for the case where things should come from SAGE_LOCAL...

comment:18 Changed 2 years ago by git

  • Commit changed from 5e1a30bbbcc673061515fbc839690ae19aa7d1a8 to 18fbf7b4bfa726dd7b8ff6a763a0ca44b7ce60f1

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

18fbf7badd SAGE_CONFIGURE_MPFR, fix sage-env-config.in

comment:19 Changed 2 years ago by git

  • Commit changed from 18fbf7b4bfa726dd7b8ff6a763a0ca44b7ce60f1 to 50aebab7b824b81166ce4652f928d15b72556e94

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

50aebabimplement --with-mpfr=system/install

comment:20 Changed 2 years ago by git

  • Commit changed from 50aebab7b824b81166ce4652f928d15b72556e94 to 0fc433fe48e870db409d0ea8035ff064b25bcf61

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

862ca6aMerge remote-tracking branch 'trac/develop' into HEAD
03dc987Merged trac #27215 in
c9873aeMerge remote-tracking branch 'trac/develop' into mpf
0fc433fupdate for Sage 8.7.rc0, a typo fix

comment:21 Changed 2 years ago by dimpase

  • Status changed from needs_work to needs_review

merged with the latest #27212 and Sage 8.7.rc0

comment:22 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:23 Changed 2 years ago by git

  • Commit changed from 0fc433fe48e870db409d0ea8035ff064b25bcf61 to 549d3aabec8766b63e7cc0ba99c425831242a959

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

8482e23Reworked this a bit more
63e54a4fix typo
41456a0added a bit more explanation
e572bb9correct logic for SAGE_GMP_PREFIX etc
f91e239add the AX_ABSOLUTE_HEADER macro
170143eiml in particular is very picky about being given an absolute path to the
50c3070spkg-configure for mpfr, adjustments for its dependees
d4cdcfbadd SAGE_CONFIGURE_MPFR, fix sage-env-config.in
76d87a4implement --with-mpfr=system/install
549d3aabuild mpir if gmp/mpir is built

comment:24 Changed 2 years ago by git

  • Commit changed from 549d3aabec8766b63e7cc0ba99c425831242a959 to df8e63dc573a23225243cd48f7dd46c6004ce873

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

0b72a68spkg-configure for mpfr, adjustments for its dependees
d690c9fadd SAGE_CONFIGURE_MPFR, fix sage-env-config.in
a833f93implement --with-mpfr=system/install
62feae3build mpir if gmp/mpir is built
df8e63dbumped up configure

Changed 2 years ago by dimpase

updated configure spkg

comment:25 Changed 2 years ago by dimpase

  • Cc fbissey added

comment:26 Changed 2 years ago by dimpase

  • Description modified (diff)

comment:27 Changed 2 years ago by embray

I don't understand why the addition of the --with-mpfr flag. This should already be covered by #27567.

comment:28 follow-up: Changed 2 years ago by embray

It seems like this would also be able to benefit from #27641. In this case you would want to add the bits that set

AC_SUBST(SAGE_MPFR_INCLUDE, ...)
AC_SUBST(SAGE_MPFR_PREFIX, ...)

in the "POST" argument, since it's something that should always be done (the values of those variables would of course still depend on checks performed earlier in the macro).

This would be similar then to the POST argument added for MPIR in #27641.

That's a strong part of what it's for: If we have some AC_SUBST variables that will be fed into another template file, such as sage-env-config.in, we want to ensure that those substitutions are always made by mpfr/spkg-configure.m4 regardless what else was determined w.r.t. MPFR.

comment:29 Changed 2 years ago by git

  • Commit changed from df8e63dc573a23225243cd48f7dd46c6004ce873 to 5c0fd282f63613768d2e120b35ec674932b3bd17

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

5c0fd28do not need to implement --with-, just use #27567

comment:30 in reply to: ↑ 28 Changed 2 years ago by dimpase

Replying to embray:

It seems like this would also be able to benefit from #27641. In this case you would want to add the bits that set

AC_SUBST(SAGE_MPFR_INCLUDE, ...)
AC_SUBST(SAGE_MPFR_PREFIX, ...)

in the "POST" argument, since it's something that should always be done (the values of those variables would of course still depend on checks performed earlier in the macro).

This would be similar then to the POST argument added for MPIR in #27641.

That's a strong part of what it's for: If we have some AC_SUBST variables that will be fed into another template file, such as sage-env-config.in, we want to ensure that those substitutions are always made by mpfr/spkg-configure.m4 regardless what else was determined w.r.t. MPFR.

I don't quite understand this. At present AC_SUBSTs are done if $with_mpfr is either system or install. Can it have any other values? If not, the code is already covered everything. If yes, then it's totally unclear how and where to get meaningful values to use in AC_SUBSTs.

A nice syntactic sugar might be to be able to put the system case in one [...] block, and [...] in another (instead of writing an explicit case statement).

comment:31 Changed 2 years ago by embray

Again, I would just compare with mpir. In the argument 5 ("POST") it checks $sage_spkg_install_mpir (as well as $sage_spkg_install_gmp) and sets the values that go in sage-env-config accordingly.

I'll take a try at it.

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

Please remove the configure bump from this branch, while you're at it.

comment:33 Changed 2 years ago by embray

  • Branch changed from u/dimpase/packages/mpfr-config to public/packages/mpfr-config
  • Commit changed from 5c0fd282f63613768d2e120b35ec674932b3bd17 to 15ba794c6397871ff308fdd5eb213087fb45f36c

This worked for me at least insofar as detecting my system mpfr properly. Now let's see if it builds...


New commits:

2dca125Add support for unconditional pre- and post-check actions in spkg-configure.m4
334e4aeRefactor the mpir/spkg-configure.m4 (and to a lesser extent the one for gmp) to use the new pre and post arguments to SAGE_SPKG_CONFIGURE
570cab3spkg-configure for mpfr, adjustments for its dependees
b824dadadd SAGE_CONFIGURE_MPFR, fix sage-env-config.in
7b0caa9implement --with-mpfr=system/install
1eb6c6cbuild mpir if gmp/mpir is built
abf4235bumped up configure
602eadfdo not need to implement --with-, just use #27567
c79875bInitial attempt at further cleanup.
15ba794I assume this was supposed to say --with-mpfr

comment:34 follow-up: Changed 2 years ago by embray

Fixed this by the way. I wonder if this was causing you any grief...

comment:35 Changed 2 years ago by git

  • Commit changed from 15ba794c6397871ff308fdd5eb213087fb45f36c to 1aa131aa76e319f95f6020c58e1d8b8789cfa4bf

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

1aa131ajust a little stylistic consistency in the messages

comment:36 in reply to: ↑ 34 Changed 2 years ago by dimpase

Replying to embray:

Fixed this by the way. I wonder if this was causing you any grief...

I didn't see any fallout from it, and I recall fixing some copy/paste oopsies like this one in sage-env-config.in, perhaps it's a result of too many rebases that this one stayed :-)

comment:37 Changed 2 years ago by embray

Build (from scratch) succeeded with no problem with system gmp+mpfr on Ubuntu 18.04, at least. I'll run the tests next just to be sure.

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

comment:38 Changed 2 years ago by dimpase

  • Authors changed from Dima Pasechnik to Dima Pasechnik, Erik Bray
  • Dependencies #27212 deleted
  • Reviewers set to Erik Bray, Dima Pasechnik

it seems to conflict with #27642 (in your m4/sage_spkg_configure.m4)- can you rebase it? Otherwise, all good.

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

comment:39 Changed 2 years ago by dimpase

namely, here you only add $4 and $5 to SAGE_SPKG_CONFIGURE macro, whereas on #27642 you not only add them, but also do

+
+AS_VAR_SET([sage_spkg_name], SPKG_NAME)
+
+$4
+
+AS_IF([test -n "`ls "${SAGE_SPKG_INST}/${sage_spkg_name}"-* 2>/dev/null`"],
+SPKG_INSTALL_VAR[=yes;] SPKG_USE_SYSTEM[=no], [
 m4_ifval(
 [$2],
 [AS_VAR_SET_IF(SPKG_INSTALL_VAR, [], SPKG_INSTALL_VAR[=no])],
 [AS_VAR_SET_IF(SPKG_INSTALL_VAR, [], SPKG_INSTALL_VAR[=yes])])
+])
+

I presume the latter should take the precedence. I'll rebase accordingly.

comment:40 Changed 2 years ago by git

  • Commit changed from 1aa131aa76e319f95f6020c58e1d8b8789cfa4bf to 7e6ae72ee7cf898966e824b82d37a751c85dbc11

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

7e6ae72add the change to sage_spkg_configure from #27642

comment:41 Changed 2 years ago by dimpase

  • Dependencies set to #27642

comment:42 Changed 2 years ago by embray

  • Status changed from needs_review to needs_work

I don't know why you pushed that last commit. I need to rebase #27642, and in turn rebase this on top of that. Please don't add spurious new commits that duplicate changes from other tickets as it will only make a mess of things later (e.g. become superfluous empty commits).

comment:43 Changed 2 years ago by git

  • Commit changed from 7e6ae72ee7cf898966e824b82d37a751c85dbc11 to 1aa131aa76e319f95f6020c58e1d8b8789cfa4bf

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

comment:44 Changed 2 years ago by dimpase

oops, sorry, I didn't realise that #27642 has an old base :-(

comment:45 Changed 2 years ago by embray

I deleted your last commit; it doesn't really belong on this branch, and it's not strictly related by any means.

That change is specifically related to #27642 and does not conflict with anything here.

comment:46 in reply to: ↑ 32 ; follow-up: Changed 2 years ago by embray

Replying to dimpase:

Please remove the configure bump from this branch, while you're at it.

To be clear, is this because we plan to consolidate this ticket with others?

comment:47 in reply to: ↑ 46 Changed 2 years ago by dimpase

Replying to embray:

Replying to dimpase:

Please remove the configure bump from this branch, while you're at it.

To be clear, is this because we plan to consolidate this ticket with others?

yes, indeed, otherwise it's too hard to get into the release. It's fine as long as it's just new spkg-configure.m4 files, but if it's more then the hell breaks loose.

Perhaps ./bootstrap should have a remote service to run autoconf ;-)

comment:48 Changed 2 years ago by embray

Ok, I'll tidy this up a bit more and then see if I can incorporate mpc as well.

comment:49 Changed 2 years ago by git

  • Commit changed from 1aa131aa76e319f95f6020c58e1d8b8789cfa4bf to d5986af21a000da5d5238b5f00ff1fa6da4404dd

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

d9bbdd4Add support for unconditional pre- and post-check actions in spkg-configure.m4
a360518Refactor the mpir/spkg-configure.m4 (and to a lesser extent the one for gmp) to use the new pre and post arguments to SAGE_SPKG_CONFIGURE
d5986afspkg-configure for mpfr, adjustments for its dependees

comment:50 Changed 2 years ago by embray

  • Dependencies changed from #27642 to #27641
  • Status changed from needs_work to needs_review

Okay, I removed the confball update and squashed a bunch of intermediate commits away. I think this is pretty clean now. Now let's take a look at mpc...

mpfr and mpc should probably be bundled together--do you think anything else should go with them? ntl maybe?

comment:51 Changed 2 years ago by dimpase

  • Dependencies changed from #27641 to #27642

perhaps not only mpc, but also ntl, arb, and flint. They all call each other and modify src/bin/sage-env-config.in...

comment:52 Changed 2 years ago by dimpase

  • Dependencies changed from #27642 to #27641

I didn't mean to undo your change of dependency without asking, but I don't understand, aren't you using the branch of #27642 here?

comment:53 follow-up: Changed 2 years ago by git

  • Commit changed from d5986af21a000da5d5238b5f00ff1fa6da4404dd to 1948e335c3b5c447c674c8ec27c49f6472556ecf

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

1948e33Add one missing SAGE_CONFIGURE_MPFR, for building gcc

comment:54 Changed 2 years ago by embray

No, I decided not to--they're not strictly related at all.

I think #27642 will merge cleanly with these other tickets, and still needs more work anyways.

comment:55 in reply to: ↑ 53 Changed 2 years ago by dimpase

Replying to git:

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

1948e33Add one missing SAGE_CONFIGURE_MPFR, for building gcc

and once mpc is added to the picture, one would also need to fix --with-mpc="$SAGE_LOCAL" - that's really why it's best to do them all together...

comment:56 Changed 2 years ago by embray

Yeah, I'm adding that in the branch for mpc now. I agree it makes sense to bundle them together.

comment:57 Changed 2 years ago by embray

  • Component changed from build to build: configure
  • Keywords mpfr added

(BTW I'm trying to organize all these spkg-configure tickets under the "build: configure" component)

comment:58 Changed 2 years ago by dimpase

  • Status changed from needs_review to positive_review

all clear

comment:59 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict, please fix

comment:60 Changed 2 years ago by dimpase

Volker, we have u/dimpase/build/mpfr-mpc-ntl, which is the merged mpfr+mpc - the branch of #27259, also includes this one (#27258), and ntl from #27265. They are modifying many common files, and should be merged together. How do we go about it? We can add a configure bump to u/dimpase/build/mpfr-mpc-ntl, after rebasing it appropriately, so that #27258, #27259, and #27265 are all merged together in one go.

We can open a special ticket for it if you prefer, or deal with it here.

comment:61 Changed 2 years ago by git

  • Commit changed from 1948e335c3b5c447c674c8ec27c49f6472556ecf to 0329fd951a75cdf6f35c4681615b3aa914897bcf

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

d5e5e3dspkg-configure for mpfr, adjustments for its dependees
0329fd9Add one missing SAGE_CONFIGURE_MPFR, for building gcc

comment:62 Changed 2 years ago by embray

  • Status changed from needs_work to positive_review

Rebased. Not sure why it had a merge conflict, as there were no conflicts found during rebasing.

comment:63 Changed 2 years ago by embray

This ticket should probably not be merged by itself. Instead please merge #27822 which incorporates this one.

comment:64 Changed 2 years ago by vbraun

  • Branch changed from public/packages/mpfr-config to 0329fd951a75cdf6f35c4681615b3aa914897bcf
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:65 Changed 2 years ago by vbraun

  • Commit 0329fd951a75cdf6f35c4681615b3aa914897bcf deleted
  • Resolution fixed deleted
  • Status changed from closed to new

comment:66 Changed 2 years ago by dimpase

The comment:63 did say this should not be merged on its own, didn't?

comment:67 Changed 2 years ago by dimpase

  • Description modified (diff)
  • Milestone changed from sage-8.8 to sage-duplicate/invalid/wontfix
  • Status changed from new to needs_review

comment:68 Changed 2 years ago by dimpase

  • Status changed from needs_review to positive_review

comment:69 Changed 22 months ago by chapoton

  • Resolution set to duplicate
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.