Opened 3 years ago
Closed 3 years ago
#27265 closed enhancement (duplicate)
spkg-configure.m4 for NTL
Reported by: | dimpase | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | build: configure | Keywords: | spkg-configure ntl |
Cc: | embray, fbissey, jdemeyer | Merged in: | |
Authors: | Dima Pasechnik, Erik Bray | Reviewers: | Erik Bray, Dima Pasechnik |
Report Upstream: | N/A | Work issues: | |
Branch: | public/packages/ntlconf (Commits, GitHub, GitLab) | Commit: | 892143a2d89591044dc20a3c87ecccaffdd46dd7 |
Dependencies: | #27212, #27641, #27751, #27259 | Stopgaps: |
Description (last modified by )
Attachments (1)
Change History (84)
comment:1 Changed 3 years ago by
- Description modified (diff)
comment:2 Changed 3 years ago by
- Dependencies set to #27212
comment:3 Changed 3 years ago by
- Dependencies changed from #27212 to #27212, #27238
comment:4 Changed 3 years ago by
comment:5 Changed 3 years ago by
- Branch set to u/dimpase/packages/ntlconf
- Commit set to af9ab9027f999907cc420d1ddc79950d2b8064e1
- Description modified (diff)
- Status changed from new to needs_review
Last 10 new commits:
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
|
862ca6a | Merge remote-tracking branch 'trac/develop' into HEAD
|
03dc987 | Merged trac #27215 in
|
c9873ae | Merge remote-tracking branch 'trac/develop' into mpf
|
af9ab90 | deal with --with-ntl, proper version check
|
comment:6 Changed 3 years ago by
- Dependencies changed from #27212, #27238 to #27212
comment:7 Changed 3 years ago by
nesting of AC_...
tests is wrong.
comment:8 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:9 Changed 3 years ago by
also needs
+++ b/build/pkgs/flint/spkg-install @@ -17,7 +17,7 @@ echo "Configuring FLINT." --disable-static \ --prefix="$SAGE_LOCAL" \ $SAGE_CONFIGURE_GMP \ - $SAGE_CONFIGURE_NTL \ + --with-ntl="$SAGE_NTL_PREFIX" \ $SAGE_CONFIGURE_MPFR \ $FLINT_CONFIGURE || sdh_die "Error: Failed to configure FLINT."
as completely omitting --with-ntl
results in no NTL linked into flint.
comment:10 Changed 3 years ago by
- Commit changed from af9ab9027f999907cc420d1ddc79950d2b8064e1 to d4378cbfd543cd10f2716a2c918b2c01ee265bbe
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
299e367 | Replace various --with-gmp=$SAGE_LOCAL flags in spkg-installs with a
|
058d515 | Reworked this a bit more
|
7beadaa | fix typo
|
b930874 | added a bit more explanation
|
6d2b496 | correct logic for SAGE_GMP_PREFIX etc
|
43be546 | add the AX_ABSOLUTE_HEADER macro
|
e343418 | iml in particular is very picky about being given an absolute path to the
|
e735b4b | configure version bump
|
cefb350 | deal with --with-ntl, proper version check
|
d4378cb | flint needs --with-ntl= always
|
comment:11 Changed 3 years ago by
comment:12 Changed 3 years ago by
- Commit changed from d4378cbfd543cd10f2716a2c918b2c01ee265bbe to 8fe9503a580b506aec1abc0de2fafaf0d2c5501d
Branch pushed to git repo; I updated commit sha1. New commits:
8fe9503 | check for deps of ntl
|
comment:13 Changed 3 years ago by
This is ready for review now. Note that often external NTL packages are built without gf2x
, but gf2x
is merely an optimisation.
Perhaps, to be completely on the safe side, spkg-configure
should check whether gf2x
is in, and if it is, only use system's NTL if gf2x
will come from the system, too.
(But I don't know how to check whether gf2x
is used in NTL, at least not in an OS-independent way).
comment:14 Changed 3 years ago by
- Cc embray fbissey added
comment:15 follow-up: ↓ 16 Changed 3 years ago by
Minor remark (it all looks ok) why keep --with-ntl="$SAGE_NTL_PREFIX"
instead of using $SAGE_CONFIGURE_NTL
like everywhere else in flint
?
comment:16 in reply to: ↑ 15 ; follow-up: ↓ 17 Changed 3 years ago by
Replying to fbissey:
Minor remark (it all looks ok) why keep
--with-ntl="$SAGE_NTL_PREFIX"
instead of using$SAGE_CONFIGURE_NTL
like everywhere else inflint
?
Because $SAGE_CONFIGURE_NTL
is empty if NTL comes from the system, but flint
must have --with-ntl
to build NTL interface.
comment:17 in reply to: ↑ 16 Changed 3 years ago by
Replying to dimpase:
Replying to fbissey:
Minor remark (it all looks ok) why keep
--with-ntl="$SAGE_NTL_PREFIX"
instead of using$SAGE_CONFIGURE_NTL
like everywhere else inflint
?Because
$SAGE_CONFIGURE_NTL
is empty if NTL comes from the system, butflint
must have--with-ntl
to build NTL interface.
Thank you for your answer. Once we get some bots reports I think we can move to a positive review.
comment:18 Changed 3 years ago by
bots will need need a configure tarball. I'll add it.
comment:19 Changed 3 years ago by
- Commit changed from 8fe9503a580b506aec1abc0de2fafaf0d2c5501d to 8473c7f9bbb37ab337bd32e2756106664c42c56b
Branch pushed to git repo; I updated commit sha1. New commits:
8473c7f | bumped up configure spkg
|
comment:20 Changed 3 years ago by
arrgh, and why is this cannot be merged?
comment:21 Changed 3 years ago by
- Commit changed from 8473c7f9bbb37ab337bd32e2756106664c42c56b to bc2f2778cb064293278d89f2c6ec7e3faddb0c75
comment:22 Changed 3 years ago by
- Description modified (diff)
comment:23 Changed 3 years ago by
- Status changed from needs_review to needs_work
as noted on #27751, build/pkgs/barvinok/spkg-install has missing then(s) in if(s)
comment:24 Changed 3 years ago by
- Commit changed from bc2f2778cb064293278d89f2c6ec7e3faddb0c75 to 1353705a47090fe937e34696f8bfb2f0f48ebdc4
comment:25 Changed 3 years ago by
- Dependencies changed from #27212 to #27212, #27751
- Status changed from needs_work to needs_review
comment:26 Changed 3 years ago by
- Cc jdemeyer added
this is a followup to #27212, and related to barvinok
, please review...
comment:27 Changed 3 years ago by
- Component changed from build to build: configure
- Keywords spkg-configure ntl added
comment:28 Changed 3 years ago by
Trying to get this to work now. I rebased it on current develop and reworked it similarly to #27259. I also have it check if we are installing the gf2x SPKG or not (we really need a utility macro for dependency handling).
But I get this:
configure: === checking whether to install the ntl SPKG === checking installing gmp/mpir? ... no checking installing gf2x? ... no checking NTL/ZZ.h usability... yes checking NTL/ZZ.h presence... yes checking for NTL/ZZ.h... yes
but then nothing. I don't see any output related to the AC_LINK_IFELSE
and AC_RUN_IFELSE
commands. I guess one of them is failing (I will check config.log next).
I guess these macros just don't have any default output? If not, we should add some.
comment:29 Changed 3 years ago by
gf2x is not compulsory for NTL. It works without it, too. So just don't do it.
comment:30 Changed 3 years ago by
In fact, it's why it's not possible to check whether gf2x is used with NTL in any "normal" way, (one does not want to run ldd on the library, right?)
comment:31 Changed 3 years ago by
I see the problem; I messed up the NTL version check stuff you did.
I don't even know what gf2x is so I'll take your word for it and remove that part.
comment:32 Changed 3 years ago by
gf2x does fast arithmetic for finite fields of even order. So without it NTL is a bit slower on these very special problems, that's all.
comment:33 Changed 3 years ago by
Out of curiosity, where did you come up with 3.10 as the minimum version of NTL? The version in Sage is 11.3.2, and the system package I have has 10.5.0.
comment:34 Changed 3 years ago by
Fedora 26 has NTL 10.3 (I guess you didn't mean 3.10...).
comment:35 Changed 3 years ago by
- Branch changed from u/dimpase/packages/ntlconf to public/packages/ntlconf
- Commit changed from 1353705a47090fe937e34696f8bfb2f0f48ebdc4 to 27f7e5859d2fed6b946170662874895e44561282
- Dependencies changed from #27212, #27751 to #27212, #27641, #27751
Rebased and incorporated #27641. Also fixed the version check, which didn't have correct logic.
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
|
d09da57 | deal with --with-ntl, proper version check
|
2ebf5b3 | flint needs --with-ntl= always
|
bc10f2e | check for deps of ntl
|
f16e43e | Clean up ntl/spkg-configure.m4
|
58cb21f | cleanup tabs
|
3a900fc | Remove unnecessary check for gf2x
|
27f7e58 | Fix NTL version check logic.
|
comment:36 Changed 3 years ago by
Right, you had 10.3. I guess I was having a dyslexic moment. I'll fix that.
comment:37 Changed 3 years ago by
- Commit changed from 27f7e5859d2fed6b946170662874895e44561282 to 2f2d3e1a2606b57ae813b6d28d8c99952fef9bfd
Branch pushed to git repo; I updated commit sha1. New commits:
2f2d3e1 | Accidentally swapped major/minor versions for minimum NTL version
|
comment:38 Changed 3 years ago by
This builds for me, again on the Ubuntu 18.04 machine I'm using, with NTL 10.5.
comment:39 follow-up: ↓ 42 Changed 3 years ago by
I am wondering why one needs these m4_push/popdefs...
comment:40 Changed 3 years ago by
- Reviewers set to Erik Bray, Dima Pasechnik
- Status changed from needs_review to positive_review
OK, this works. I've merged mpfr+mpc+ntl in the branch u/dimpase/build/mpfr-mpc-ntl
(which is not automatic) and tested it, as well.
comment:41 Changed 3 years ago by
Should we go ahead and set that as the branch for this ticket, or create a new one just for updating the configure version?
comment:42 in reply to: ↑ 39 Changed 3 years ago by
Replying to dimpase:
I am wondering why one needs these m4_push/popdefs...
I think it's not strictly necessary I'm just trying to avoid unnecessary cluttering of the macro namespace.
comment:43 Changed 3 years ago by
well, we need to sort out the unfortunate merging of #27642, which seems to lead to merge conflicts here. (and for mpfr+mpc alone, too). Only after this is done, we can proceed here, I think.
comment:44 Changed 3 years ago by
I'm still surprised it would lead to merge conflicts, much less any that are non-trivial. I'll have a look...
comment:45 Changed 3 years ago by
- Commit changed from 2f2d3e1a2606b57ae813b6d28d8c99952fef9bfd to d16db1cc2671fa816d40a84436268dfb5d36f003
- 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:
d16db1c | Trac #27265: Add spkg-configure.m4 for ntl
|
comment:46 Changed 3 years ago by
Rebased on 8.8.beta5 and squashed some commits.
comment:47 Changed 3 years ago by
- Dependencies changed from #27212, #27641, #27751 to #27212, #27641, #27751, #27259
- Status changed from needs_review to needs_work
This has sort of necessary (but trivial) conflicts with #27259 so I will base it on top of that ticket's branch.
comment:48 Changed 3 years ago by
- Commit changed from d16db1cc2671fa816d40a84436268dfb5d36f003 to 974c4831f775088bfd43f4efd5e5c453fcd44175
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d5e5e3d | spkg-configure for mpfr, adjustments for its dependees
|
0329fd9 | Add one missing SAGE_CONFIGURE_MPFR, for building gcc
|
d15ead5 | Trac #27259: spkg-configure for mpc, adjustments for its dependents
|
974c483 | Trac #27265: Add spkg-configure.m4 for ntl
|
comment:49 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:50 Changed 3 years ago by
This ticket should probably not be merged by itself. Instead please merge #27822 which incorporates this one.
comment:52 Changed 3 years ago by
- Status changed from positive_review to needs_work
You for some reason skipped 2ebf5b367c62def637fdc2955a80134a56439e0b - which is crucial for building Flint right with external NTL, see comment:9
comment:53 Changed 3 years ago by
- Commit changed from 974c4831f775088bfd43f4efd5e5c453fcd44175 to e025f888872035a04bcdbd047409c0e99a92dd14
Branch pushed to git repo; I updated commit sha1. New commits:
e025f88 | flint needs --with-ntl= always
|
comment:54 Changed 3 years ago by
- Status changed from needs_work to positive_review
comment:55 Changed 3 years ago by
- Commit changed from e025f888872035a04bcdbd047409c0e99a92dd14 to deaa7e0c9efb9df512c7d4f58a87df5759a3fbb2
- Status changed from positive_review to needs_review
comment:56 Changed 3 years ago by
Caught a bug - the tests were sometimes passing due to more libs in LIBS, it seems. Now it should be OK. Please review.
comment:57 follow-up: ↓ 60 Changed 3 years ago by
Are you saying flint's configure actually breaks if you *don't* pass a --with-ntl
option, even if it's empty?
I had removed that while squashing because it looked like a mistake to me.
comment:58 Changed 3 years ago by
Also please avoid adding unnecessary merge commits. I'll just rebase this again...
comment:59 follow-up: ↓ 61 Changed 3 years ago by
I see. It doesn't even enable the NTL interface if you don't pass --with-ntl
. Also if you use --with-ntl
without an argument it assumes you want /usr/local
which is also not so good. It might be mostly harmless but I want to double-check because that could be confusing.
There are several things that are not so good about flint's hand-written configure script. I've offered to redo it with autoconf but Bill Hart complained that he didn't want to have to learn autoconf. And while I can completely understand the sentiment I think the fact is it would be better and much simpler if it did use autoconf. There's nothing much fancy it needs to do...
comment:60 in reply to: ↑ 57 Changed 3 years ago by
Replying to embray:
Are you saying flint's configure actually breaks if you *don't* pass a
--with-ntl
option, even if it's empty?
yes, it won’t link ntl unless ‘—with-ntl’ is present.
I had removed that while squashing because it looked like a mistake to me.
the joys of dealing dealings with hand-written configure scripts.
comment:61 in reply to: ↑ 59 Changed 3 years ago by
Replying to embray:
There are several things that are not so good about flint's hand-written configure script. I've offered to redo it with autoconf but Bill Hart complained that he didn't want to have to learn autoconf. And while I can completely understand the sentiment I think the fact is it would be better and much simpler if it did use autoconf. There's nothing much fancy it needs to do...
Welcome! You are not the first one to offer. The worst thing about it, is that it infects "descendant" projects such as arb
. arb
's configure script is just a tweaked version of the flint
script.
comment:62 Changed 3 years ago by
- Commit changed from deaa7e0c9efb9df512c7d4f58a87df5759a3fbb2 to 17af3bad767bfe0ccc5f63750da5e82ad3533d3e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
17af3ba | flint needs --with-ntl= always
|
comment:63 Changed 3 years ago by
rebased/squashed to remove the last merge commit.
comment:64 Changed 3 years ago by
Is it OK now? Also, could you please update #27822 in this respect?
comment:65 Changed 3 years ago by
Yes, I think this is ok now. I'm still a little perturbed by the situation with flint but I think this will do for now. I'll rebase #27822.
comment:66 Changed 3 years ago by
- Status changed from needs_review to positive_review
comment:67 Changed 3 years ago by
- Commit changed from 17af3bad767bfe0ccc5f63750da5e82ad3533d3e to 74b1451ee15263868c44386b03f272ea6da966ae
- 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. Last 10 new commits:
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
|
d09da57 | deal with --with-ntl, proper version check
|
2ebf5b3 | flint needs --with-ntl= always
|
bc10f2e | check for deps of ntl
|
f16e43e | Clean up ntl/spkg-configure.m4
|
58cb21f | cleanup tabs
|
3a900fc | Remove unnecessary check for gf2x
|
27f7e58 | Fix NTL version check logic.
|
d98a05e | correct use of AC_LINK_IFELSE
|
74b1451 | flint needs --with-ntl= always
|
comment:68 Changed 3 years ago by
Oops, rebased an older version of the branch.
comment:69 Changed 3 years ago by
- Commit changed from 74b1451ee15263868c44386b03f272ea6da966ae to 4b3caa7b35b0f0effb442feb18e4cc2c574cbb8e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d5e5e3d | spkg-configure for mpfr, adjustments for its dependees
|
0329fd9 | Add one missing SAGE_CONFIGURE_MPFR, for building gcc
|
d15ead5 | Trac #27259: spkg-configure for mpc, adjustments for its dependents
|
974c483 | Trac #27265: Add spkg-configure.m4 for ntl
|
96a7d77 | correct use of AC_LINK_IFELSE
|
4b3caa7 | flint needs --with-ntl= always
|
comment:70 Changed 3 years ago by
- Status changed from needs_review to positive_review
comment:71 Changed 3 years ago by
- Description modified (diff)
comment:72 Changed 3 years ago by
Missing quotes here:
-
build/pkgs/ntl/spkg-configure.m4
a b SAGE_SPKG_CONFIGURE([ntl], [ 19 19 AC_LINK_IFELSE([ 20 20 AC_LANG_PROGRAM([[#include <NTL/ZZ.h>]], 21 21 [[NTL::ZZ a;]] 22 )], [LIBS= $LIBS -lntl]22 )], [LIBS="$LIBS -lntl"] 23 23 [AC_MSG_RESULT([yes])], [ 24 24 AC_MSG_RESULT([no]); sage_spkg_install_ntl=yes 25 25 ])
so it works if LIBS was empty, but breaks if it's already not...
comment:73 Changed 3 years ago by
- Commit changed from 4b3caa7b35b0f0effb442feb18e4cc2c574cbb8e to 892143a2d89591044dc20a3c87ecccaffdd46dd7
- Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
892143a | add missing quotes
|
comment:74 Changed 3 years ago by
- Status changed from needs_review to positive_review
stumble upon this while trying this with more spkg-configs merged.
comment:75 Changed 3 years ago by
- Status changed from positive_review to needs_work
Please only set this ticket to positive review after you have personally verified that a clean sage build works
comment:76 Changed 3 years ago by
what makes you think I did not do this?
comment:77 Changed 3 years ago by
A clean build worked great, it's a "dirty" build that revealed the problem of comment:72
comment:78 Changed 3 years ago by
- Description modified (diff)
- Milestone changed from sage-8.8 to sage-duplicate/invalid/wontfix
- Status changed from needs_work to positive_review
comment:79 Changed 3 years ago by
with clang on OSX, I just saw a failure to recognise NTL, due to c++ in the test called without -std=gnu++11
.
Adding AC_REQUIRE([SAGE_SPKG_CONFIGURE_GCC])
just above AC_REQUIRE([SAGE_SPKG_CONFIGURE_GMP])
in NTL's spkg-configure.m4 fixes this.
But this seems to be a not very consistent way to do thing, as SAGE_SPKG_CONFIGURE_GCC
ought to be called before the any other library tests. Do we have a way to force this?
comment:80 follow-up: ↓ 81 Changed 3 years ago by
It might work to just manually call AC_REQUIRE([SAGE_SPKG_CONFIGURE_GCC])
right at the beginning of SAGE_SPKG_COLLECT
.
comment:81 in reply to: ↑ 80 ; follow-up: ↓ 82 Changed 3 years ago by
Replying to embray:
It might work to just manually call
AC_REQUIRE([SAGE_SPKG_CONFIGURE_GCC])
right at the beginning ofSAGE_SPKG_COLLECT
.
hmm, no, this does not work, as at this moment SAGE_SPKG_CONFIGURE_GCC
is not defined.
I am thinking of putting it into SAGE_SPKG_CONFIGURE
, but this means it's a recursive call, so the only way I can think of would be avoid the recursion:
to define essentially a copy of
SAGE_SPKG_CONFIGURE
, without this call, and then define SAGE_SPKG_CONFIGURE_GCC
using this copy. After this AC_REQUIRE(...)
would not involve a recursion.
comment:82 in reply to: ↑ 81 Changed 3 years ago by
Replying to dimpase:
Replying to embray:
It might work to just manually call
AC_REQUIRE([SAGE_SPKG_CONFIGURE_GCC])
right at the beginning ofSAGE_SPKG_COLLECT
.hmm, no, this does not work, as at this moment
SAGE_SPKG_CONFIGURE_GCC
is not defined.I am thinking of putting it into
SAGE_SPKG_CONFIGURE
, but this means it's a recursive call, so the only way I can think of would be avoid the recursion: to define essentially a copy ofSAGE_SPKG_CONFIGURE
, without this call, and then defineSAGE_SPKG_CONFIGURE_GCC
using this copy. After thisAC_REQUIRE(...)
would not involve a recursion.
I have opened #28005 to deal with this.
comment:83 Changed 3 years ago by
- Resolution set to duplicate
- Status changed from positive_review to closed
here we might want to check that NTL is built with gf2x. However, it appears that it's only question of better performance, not an exta API, if it gf2x is linked.