spkg-configure.m4 for NTL

[dimpase]

implemented a direct version check using AC_RUN_IFELSE. The rest is standard, as in #27212, basically.

to be merged as a part of #27822

[dimpase]

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.

[dimpase]

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

[dimpase]

nesting of AC_... tests is wrong.

[embray]

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

[dimpase]

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.

[git]

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

[dimpase]

rebased over the latest #27212 and fixed comment:9 issue.

[git]

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

​8fe9503

check for deps of ntl

[dimpase]

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

[fbissey]

Minor remark (it all looks ok) why keep --with-ntl="$SAGE_NTL_PREFIX" instead of using $SAGE_CONFIGURE_NTL like everywhere else in flint?

[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 in flint?

Because $SAGE_CONFIGURE_NTL is empty if NTL comes from the system, but flint must have --with-ntl to build NTL interface.

[fbissey]

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 in flint?

Because $SAGE_CONFIGURE_NTL is empty if NTL comes from the system, but flint 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.

[dimpase]

bots will need need a configure tarball. I'll add it.

[dimpase]

updated configure spkg

[git]

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

​8473c7f

bumped up configure spkg

[dimpase]

arrgh, and why is this cannot be merged?

[git]

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

​1d165a8	deal with --with-ntl, proper version check
​5817a99	flint needs --with-ntl= always
​6979e43	check for deps of ntl
​bc2f277	bumped up configure spkg

[dimpase]

as noted on #27751, build/pkgs/barvinok/spkg-install has missing then(s) in if(s)

[git]

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

​e758967	adding missing then in if
​1c7c182	Merge branch 'barvinokfix1' into ntlconf
​1353705	another missing then

[dimpase]

this is a followup to #27212, and related to barvinok, please review...

[embray]

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.

[dimpase]

gf2x is not compulsory for NTL. It works without it, too. So just don't do it.

[dimpase]

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

[embray]

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.

[dimpase]

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.

[embray]

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.

[dimpase]

Fedora 26 has NTL 10.3 (I guess you didn't mean 3.10...).

[embray]

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.

[embray]

Right, you had 10.3. I guess I was having a dyslexic moment. I'll fix that.

[git]

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

​2f2d3e1

Accidentally swapped major/minor versions for minimum NTL version

[embray]

This builds for me, again on the Ubuntu 18.04 machine I'm using, with NTL 10.5.

[dimpase]

I am wondering why one needs these m4_push/popdefs...

[dimpase]

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.

[embray]

Should we go ahead and set that as the branch for this ticket, or create a new one just for updating the configure version?

[embray]

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.

[dimpase]

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.

[embray]

I'm still surprised it would lead to merge conflicts, much less any that are non-trivial. I'll have a look...

[git]

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

[embray]

Rebased on 8.8.beta5 and squashed some commits.

[embray]

This has sort of necessary (but trivial) conflicts with #27259 so I will base it on top of that ticket's branch.

[git]

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

[embray]

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

[dimpase]

lgtm

[dimpase]

You for some reason skipped 2ebf5b367c62def637fdc2955a80134a56439e0b - which is crucial for building Flint right with external NTL, see comment:9

[git]

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

​e025f88

flint needs --with-ntl= always

[git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

​c40427a	correct use of AC_LINK_IFELSE
​deaa7e0	Merge branch 'public/packages/ntlconf' of trac.sagemath.org:sage into wip

[dimpase]

Caught a bug - the tests were sometimes passing due to more libs in LIBS, it seems. Now it should be OK. Please review.

[embray]

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.

[embray]

Also please avoid adding unnecessary merge commits. I'll just rebase this again...

[embray]

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

[dimpase]

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.

[fbissey]

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.

[git]

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

​17af3ba

flint needs --with-ntl= always

[dimpase]

rebased/squashed to remove the last merge commit.

[dimpase]

Is it OK now? Also, could you please update #27822 in this respect?

[embray]

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.

[git]

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

[embray]

Oops, rebased an older version of the branch.

[git]

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

[dimpase]

Missing quotes here:

build/pkgs/ntl/spkg-configure.m4

@@ -19 +19 @@
         AC_LINK_IFELSE([
             AC_LANG_PROGRAM([[#include <NTL/ZZ.h>]],
                             [[NTL::ZZ a;]]
-            )], [LIBS=$LIBS -lntl]
+            )], [LIBS="$LIBS -lntl"]
                 [AC_MSG_RESULT([yes])], [
             AC_MSG_RESULT([no]); sage_spkg_install_ntl=yes
         ])

so it works if LIBS was empty, but breaks if it's already not...

[git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

​892143a

add missing quotes

[dimpase]

stumble upon this while trying this with more spkg-configs merged.

[vbraun]

Please only set this ticket to positive review after you have personally verified that a clean sage build works

[dimpase]

what makes you think I did not do this?

[dimpase]

A clean build worked great, it's a "dirty" build that revealed the problem of comment:72

[dimpase]

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?

[embray]

It might work to just manually call AC_REQUIRE([SAGE_SPKG_CONFIGURE_GCC]) right at the beginning of SAGE_SPKG_COLLECT.

[dimpase]

Replying to embray:

It might work to just manually call AC_REQUIRE([SAGE_SPKG_CONFIGURE_GCC]) right at the beginning of SAGE_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.

[dimpase]

Replying to dimpase:

Replying to embray:

It might work to just manually call AC_REQUIRE([SAGE_SPKG_CONFIGURE_GCC]) right at the beginning of SAGE_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.

I have opened #28005 to deal with this.