Opened 3 years ago
Closed 3 years 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: |
Description (last modified by )
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)
Change History (70)
comment:1 Changed 3 years ago by
- Keywords spkg-configure added
comment:2 Changed 3 years ago by
comment:3 follow-up: ↓ 4 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
mpfrcx's spkg-install hardcodes the location of mpfr to SAGE_LOCAL. So this will need to be fixed.
comment:6 Changed 3 years ago by
- Branch set to u/dimpase/packages/mpfr-config
- Commit set to 5e1a30bbbcc673061515fbc839690ae19aa7d1a8
- Description modified (diff)
- Status changed from new to needs_review
New commits:
c4e42e6 | spkg-configure for GMP
|
b070088 | Move all the --with-mp logic into the spkg-configure.m4 for MPIR so that it
|
3b6eebd | Replace various --with-gmp=$SAGE_LOCAL flags in spkg-installs with a
|
b80bf72 | Reworked this a bit more
|
0ca3f56 | fix typo
|
06798ca | added a bit more explanation
|
b592d77 | correct logic for SAGE_GMP_PREFIX etc
|
5057680 | add the AX_ABSOLUTE_HEADER macro
|
101537b | iml in particular is very picky about being given an absolute path to the
|
5e1a30b | spkg-configure for mpfr, adjustments for its dependees
|
comment:7 Changed 3 years ago by
only the last commit is new, the rest are from #27212.
comment:8 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
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 3 years ago by
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 3 years ago by
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 3 years ago by
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 3 years ago by
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 3 years ago by
I've also tested this on Gentoo, on Debian.
comment:16 Changed 3 years ago by
- 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 3 years ago by
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 3 years ago by
- Commit changed from 5e1a30bbbcc673061515fbc839690ae19aa7d1a8 to 18fbf7b4bfa726dd7b8ff6a763a0ca44b7ce60f1
Branch pushed to git repo; I updated commit sha1. New commits:
18fbf7b | add SAGE_CONFIGURE_MPFR, fix sage-env-config.in
|
comment:19 Changed 3 years ago by
- Commit changed from 18fbf7b4bfa726dd7b8ff6a763a0ca44b7ce60f1 to 50aebab7b824b81166ce4652f928d15b72556e94
Branch pushed to git repo; I updated commit sha1. New commits:
50aebab | implement --with-mpfr=system/install
|
comment:20 Changed 3 years ago by
- Commit changed from 50aebab7b824b81166ce4652f928d15b72556e94 to 0fc433fe48e870db409d0ea8035ff064b25bcf61
comment:21 Changed 3 years ago by
- Status changed from needs_work to needs_review
merged with the latest #27212 and Sage 8.7.rc0
comment:22 Changed 3 years ago by
- 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 3 years ago by
- Commit changed from 0fc433fe48e870db409d0ea8035ff064b25bcf61 to 549d3aabec8766b63e7cc0ba99c425831242a959
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
8482e23 | Reworked this a bit more
|
63e54a4 | fix typo
|
41456a0 | added a bit more explanation
|
e572bb9 | correct logic for SAGE_GMP_PREFIX etc
|
f91e239 | add the AX_ABSOLUTE_HEADER macro
|
170143e | iml in particular is very picky about being given an absolute path to the
|
50c3070 | spkg-configure for mpfr, adjustments for its dependees
|
d4cdcfb | add SAGE_CONFIGURE_MPFR, fix sage-env-config.in
|
76d87a4 | implement --with-mpfr=system/install
|
549d3aa | build mpir if gmp/mpir is built
|
comment:24 Changed 3 years ago by
- Commit changed from 549d3aabec8766b63e7cc0ba99c425831242a959 to df8e63dc573a23225243cd48f7dd46c6004ce873
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0b72a68 | spkg-configure for mpfr, adjustments for its dependees
|
d690c9f | add SAGE_CONFIGURE_MPFR, fix sage-env-config.in
|
a833f93 | implement --with-mpfr=system/install
|
62feae3 | build mpir if gmp/mpir is built
|
df8e63d | bumped up configure
|
comment:25 Changed 3 years ago by
- Cc fbissey added
comment:26 Changed 3 years ago by
- Description modified (diff)
comment:27 Changed 3 years ago by
I don't understand why the addition of the --with-mpfr
flag. This should already be covered by #27567.
comment:28 follow-up: ↓ 30 Changed 3 years ago by
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 3 years ago by
- Commit changed from df8e63dc573a23225243cd48f7dd46c6004ce873 to 5c0fd282f63613768d2e120b35ec674932b3bd17
Branch pushed to git repo; I updated commit sha1. New commits:
5c0fd28 | do not need to implement --with-, just use #27567
|
comment:30 in reply to: ↑ 28 Changed 3 years ago by
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 assage-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 3 years ago by
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: ↓ 46 Changed 3 years ago by
Please remove the configure bump from this branch, while you're at it.
comment:33 Changed 3 years ago by
- 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:
2dca125 | Add support for unconditional pre- and post-check actions in spkg-configure.m4
|
334e4ae | Refactor 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
|
570cab3 | spkg-configure for mpfr, adjustments for its dependees
|
b824dad | add SAGE_CONFIGURE_MPFR, fix sage-env-config.in
|
7b0caa9 | implement --with-mpfr=system/install
|
1eb6c6c | build mpir if gmp/mpir is built
|
abf4235 | bumped up configure
|
602eadf | do not need to implement --with-, just use #27567
|
c79875b | Initial attempt at further cleanup.
|
15ba794 | I assume this was supposed to say --with-mpfr
|
comment:34 follow-up: ↓ 36 Changed 3 years ago by
Fixed this by the way. I wonder if this was causing you any grief...
comment:35 Changed 3 years ago by
- Commit changed from 15ba794c6397871ff308fdd5eb213087fb45f36c to 1aa131aa76e319f95f6020c58e1d8b8789cfa4bf
Branch pushed to git repo; I updated commit sha1. New commits:
1aa131a | just a little stylistic consistency in the messages
|
comment:36 in reply to: ↑ 34 Changed 3 years ago by
comment:37 Changed 3 years ago by
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.
comment:38 Changed 3 years ago by
- 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.
comment:39 Changed 3 years ago by
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 3 years ago by
- Commit changed from 1aa131aa76e319f95f6020c58e1d8b8789cfa4bf to 7e6ae72ee7cf898966e824b82d37a751c85dbc11
Branch pushed to git repo; I updated commit sha1. New commits:
7e6ae72 | add the change to sage_spkg_configure from #27642
|
comment:41 Changed 3 years ago by
- Dependencies set to #27642
comment:42 Changed 3 years ago by
- 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 3 years ago by
- 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 3 years ago by
oops, sorry, I didn't realise that #27642 has an old base :-(
comment:45 Changed 3 years ago by
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: ↓ 47 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
Ok, I'll tidy this up a bit more and then see if I can incorporate mpc as well.
comment:49 Changed 3 years ago by
- Commit changed from 1aa131aa76e319f95f6020c58e1d8b8789cfa4bf to d5986af21a000da5d5238b5f00ff1fa6da4404dd
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d9bbdd4 | Add support for unconditional pre- and post-check actions in spkg-configure.m4
|
a360518 | Refactor 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
|
d5986af | spkg-configure for mpfr, adjustments for its dependees
|
comment:50 Changed 3 years ago by
- 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 3 years ago by
- 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 3 years ago by
- 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: ↓ 55 Changed 3 years ago by
- Commit changed from d5986af21a000da5d5238b5f00ff1fa6da4404dd to 1948e335c3b5c447c674c8ec27c49f6472556ecf
Branch pushed to git repo; I updated commit sha1. New commits:
1948e33 | Add one missing SAGE_CONFIGURE_MPFR, for building gcc
|
comment:54 Changed 3 years ago by
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 3 years ago by
comment:56 Changed 3 years ago by
Yeah, I'm adding that in the branch for mpc now. I agree it makes sense to bundle them together.
comment:57 Changed 3 years ago by
- 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:59 Changed 3 years ago by
- Status changed from positive_review to needs_work
Merge conflict, please fix
comment:60 Changed 3 years ago by
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 3 years ago by
- Commit changed from 1948e335c3b5c447c674c8ec27c49f6472556ecf to 0329fd951a75cdf6f35c4681615b3aa914897bcf
comment:62 Changed 3 years ago by
- 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 3 years ago by
This ticket should probably not be merged by itself. Instead please merge #27822 which incorporates this one.
comment:64 Changed 3 years ago by
- Branch changed from public/packages/mpfr-config to 0329fd951a75cdf6f35c4681615b3aa914897bcf
- Resolution set to fixed
- Status changed from positive_review to closed
comment:65 Changed 3 years ago by
- Commit 0329fd951a75cdf6f35c4681615b3aa914897bcf deleted
- Resolution fixed deleted
- Status changed from closed to new
comment:66 Changed 3 years ago by
The comment:63 did say this should not be merged on its own, didn't?
comment:67 Changed 3 years ago by
- Description modified (diff)
- Milestone changed from sage-8.8 to sage-duplicate/invalid/wontfix
- Status changed from new to needs_review
comment:68 Changed 3 years ago by
- Status changed from needs_review to positive_review
comment:69 Changed 3 years ago by
- Resolution set to duplicate
- Status changed from positive_review to closed
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):
AC_SEARCH_LIBS
rather thatAC_CHECK_LIB
here?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.