Opened 2 years ago
Closed 21 months ago
#27265 closed enhancement (duplicate)
spkgconfigure.m4 for NTL
Reported by:  dimpase  Owned by:  

Priority:  major  Milestone:  sageduplicate/invalid/wontfix 
Component:  build: configure  Keywords:  spkgconfigure 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 2 years ago by
 Description modified (diff)
comment:2 Changed 2 years ago by
 Dependencies set to #27212
comment:3 Changed 2 years ago by
 Dependencies changed from #27212 to #27212, #27238
comment:4 Changed 2 years ago by
comment:5 Changed 2 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 remotetracking branch 'trac/develop' into HEAD

03dc987  Merged trac #27215 in

c9873ae  Merge remotetracking branch 'trac/develop' into mpf

af9ab90  deal with withntl, proper version check

comment:6 Changed 2 years ago by
 Dependencies changed from #27212, #27238 to #27212
comment:7 Changed 2 years ago by
nesting of AC_...
tests is wrong.
comment:8 Changed 2 years ago by
 Milestone changed from sage8.7 to sage8.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
also needs
+++ b/build/pkgs/flint/spkginstall @@ 17,7 +17,7 @@ echo "Configuring FLINT." disablestatic \ prefix="$SAGE_LOCAL" \ $SAGE_CONFIGURE_GMP \  $SAGE_CONFIGURE_NTL \ + withntl="$SAGE_NTL_PREFIX" \ $SAGE_CONFIGURE_MPFR \ $FLINT_CONFIGURE  sdh_die "Error: Failed to configure FLINT."
as completely omitting withntl
results in no NTL linked into flint.
comment:10 Changed 2 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 withgmp=$SAGE_LOCAL flags in spkginstalls 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 withntl, proper version check

d4378cb  flint needs withntl= always

comment:11 Changed 2 years ago by
comment:12 Changed 2 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 2 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, spkgconfigure
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 OSindependent way).
comment:14 Changed 2 years ago by
 Cc embray fbissey added
comment:15 followup: ↓ 16 Changed 2 years ago by
Minor remark (it all looks ok) why keep withntl="$SAGE_NTL_PREFIX"
instead of using $SAGE_CONFIGURE_NTL
like everywhere else in flint
?
comment:16 in reply to: ↑ 15 ; followup: ↓ 17 Changed 2 years ago by
Replying to fbissey:
Minor remark (it all looks ok) why keep
withntl="$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 withntl
to build NTL interface.
comment:17 in reply to: ↑ 16 Changed 2 years ago by
Replying to dimpase:
Replying to fbissey:
Minor remark (it all looks ok) why keep
withntl="$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 havewithntl
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 2 years ago by
bots will need need a configure tarball. I'll add it.
comment:19 Changed 2 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 2 years ago by
arrgh, and why is this cannot be merged?
comment:21 Changed 2 years ago by
 Commit changed from 8473c7f9bbb37ab337bd32e2756106664c42c56b to bc2f2778cb064293278d89f2c6ec7e3faddb0c75
comment:22 Changed 2 years ago by
 Description modified (diff)
comment:23 Changed 2 years ago by
 Status changed from needs_review to needs_work
as noted on #27751, build/pkgs/barvinok/spkginstall has missing then(s) in if(s)
comment:24 Changed 2 years ago by
 Commit changed from bc2f2778cb064293278d89f2c6ec7e3faddb0c75 to 1353705a47090fe937e34696f8bfb2f0f48ebdc4
comment:25 Changed 2 years ago by
 Dependencies changed from #27212 to #27212, #27751
 Status changed from needs_work to needs_review
comment:26 Changed 2 years ago by
 Cc jdemeyer added
this is a followup to #27212, and related to barvinok
, please review...
comment:27 Changed 2 years ago by
 Component changed from build to build: configure
 Keywords spkgconfigure ntl added
comment:28 Changed 2 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 2 years ago by
gf2x is not compulsory for NTL. It works without it, too. So just don't do it.
comment:30 Changed 2 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 2 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 2 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 2 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 2 years ago by
Fedora 26 has NTL 10.3 (I guess you didn't mean 3.10...).
comment:35 Changed 2 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 postcheck actions in spkgconfigure.m4

a360518  Refactor the mpir/spkgconfigure.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 withntl, proper version check

2ebf5b3  flint needs withntl= always

bc10f2e  check for deps of ntl

f16e43e  Clean up ntl/spkgconfigure.m4

58cb21f  cleanup tabs

3a900fc  Remove unnecessary check for gf2x

27f7e58  Fix NTL version check logic.

comment:36 Changed 2 years ago by
Right, you had 10.3. I guess I was having a dyslexic moment. I'll fix that.
comment:37 Changed 2 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 2 years ago by
This builds for me, again on the Ubuntu 18.04 machine I'm using, with NTL 10.5.
comment:39 followup: ↓ 42 Changed 2 years ago by
I am wondering why one needs these m4_push/popdefs...
comment:40 Changed 2 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/mpfrmpcntl
(which is not automatic) and tested it, as well.
comment:41 Changed 2 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 2 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 2 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 2 years ago by
I'm still surprised it would lead to merge conflicts, much less any that are nontrivial. I'll have a look...
comment:45 Changed 2 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 spkgconfigure.m4 for ntl

comment:46 Changed 2 years ago by
Rebased on 8.8.beta5 and squashed some commits.
comment:47 Changed 2 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 2 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  spkgconfigure for mpfr, adjustments for its dependees

0329fd9  Add one missing SAGE_CONFIGURE_MPFR, for building gcc

d15ead5  Trac #27259: spkgconfigure for mpc, adjustments for its dependents

974c483  Trac #27265: Add spkgconfigure.m4 for ntl

comment:49 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:50 Changed 2 years ago by
This ticket should probably not be merged by itself. Instead please merge #27822 which incorporates this one.
comment:52 Changed 2 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 2 years ago by
 Commit changed from 974c4831f775088bfd43f4efd5e5c453fcd44175 to e025f888872035a04bcdbd047409c0e99a92dd14
Branch pushed to git repo; I updated commit sha1. New commits:
e025f88  flint needs withntl= always

comment:54 Changed 2 years ago by
 Status changed from needs_work to positive_review
comment:55 Changed 2 years ago by
 Commit changed from e025f888872035a04bcdbd047409c0e99a92dd14 to deaa7e0c9efb9df512c7d4f58a87df5759a3fbb2
 Status changed from positive_review to needs_review
comment:56 Changed 2 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 followup: ↓ 60 Changed 2 years ago by
Are you saying flint's configure actually breaks if you *don't* pass a withntl
option, even if it's empty?
I had removed that while squashing because it looked like a mistake to me.
comment:58 Changed 2 years ago by
Also please avoid adding unnecessary merge commits. I'll just rebase this again...
comment:59 followup: ↓ 61 Changed 2 years ago by
I see. It doesn't even enable the NTL interface if you don't pass withntl
. Also if you use withntl
without an argument it assumes you want /usr/local
which is also not so good. It might be mostly harmless but I want to doublecheck because that could be confusing.
There are several things that are not so good about flint's handwritten 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 2 years ago by
Replying to embray:
Are you saying flint's configure actually breaks if you *don't* pass a
withntl
option, even if it's empty?
yes, it won’t link ntl unless ‘—withntl’ is present.
I had removed that while squashing because it looked like a mistake to me.
the joys of dealing dealings with handwritten configure scripts.
comment:61 in reply to: ↑ 59 Changed 2 years ago by
Replying to embray:
There are several things that are not so good about flint's handwritten 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 2 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 withntl= always

comment:63 Changed 2 years ago by
rebased/squashed to remove the last merge commit.
comment:64 Changed 2 years ago by
Is it OK now? Also, could you please update #27822 in this respect?
comment:65 Changed 2 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 2 years ago by
 Status changed from needs_review to positive_review
comment:67 Changed 2 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/spkgconfigure.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 withntl, proper version check

2ebf5b3  flint needs withntl= always

bc10f2e  check for deps of ntl

f16e43e  Clean up ntl/spkgconfigure.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 withntl= always

comment:68 Changed 2 years ago by
Oops, rebased an older version of the branch.
comment:69 Changed 2 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  spkgconfigure for mpfr, adjustments for its dependees

0329fd9  Add one missing SAGE_CONFIGURE_MPFR, for building gcc

d15ead5  Trac #27259: spkgconfigure for mpc, adjustments for its dependents

974c483  Trac #27265: Add spkgconfigure.m4 for ntl

96a7d77  correct use of AC_LINK_IFELSE

4b3caa7  flint needs withntl= always

comment:70 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:71 Changed 2 years ago by
 Description modified (diff)
comment:72 Changed 2 years ago by
Missing quotes here:

build/pkgs/ntl/spkgconfigure.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 2 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 2 years ago by
 Status changed from needs_review to positive_review
stumble upon this while trying this with more spkgconfigs merged.
comment:75 Changed 23 months 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 23 months ago by
what makes you think I did not do this?
comment:77 Changed 23 months ago by
A clean build worked great, it's a "dirty" build that revealed the problem of comment:72
comment:78 Changed 23 months ago by
 Description modified (diff)
 Milestone changed from sage8.8 to sageduplicate/invalid/wontfix
 Status changed from needs_work to positive_review
comment:79 Changed 22 months 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 spkgconfigure.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 followup: ↓ 81 Changed 22 months 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 ; followup: ↓ 82 Changed 22 months 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 22 months 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 21 months 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.