Opened 2 years ago

Closed 21 months 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:

Status badges

Description (last modified by 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

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 (84)

comment:1 Changed 2 years ago by dimpase

  • Description modified (diff)

comment:2 Changed 2 years ago by dimpase

  • Dependencies set to #27212

comment:3 Changed 2 years ago by dimpase

  • Dependencies changed from #27212 to #27212, #27238

comment:4 Changed 2 years ago by 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.

comment:5 Changed 2 years ago by dimpase

  • Authors set to Dima Pasechnik
  • 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:

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
862ca6aMerge remote-tracking branch 'trac/develop' into HEAD
03dc987Merged trac #27215 in
c9873aeMerge remote-tracking branch 'trac/develop' into mpf
af9ab90deal with --with-ntl, proper version check

comment:6 Changed 2 years ago by dimpase

  • Dependencies changed from #27212, #27238 to #27212

comment:7 Changed 2 years ago by dimpase

nesting of AC_... tests is wrong.

comment:8 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:9 Changed 2 years ago by 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.

comment:10 Changed 2 years ago by git

  • Commit changed from af9ab9027f999907cc420d1ddc79950d2b8064e1 to d4378cbfd543cd10f2716a2c918b2c01ee265bbe

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

299e367Replace various --with-gmp=$SAGE_LOCAL flags in spkg-installs with a
058d515Reworked this a bit more
7beadaafix typo
b930874added a bit more explanation
6d2b496correct logic for SAGE_GMP_PREFIX etc
43be546add the AX_ABSOLUTE_HEADER macro
e343418iml in particular is very picky about being given an absolute path to the
e735b4bconfigure version bump
cefb350deal with --with-ntl, proper version check
d4378cbflint needs --with-ntl= always

comment:11 Changed 2 years ago by dimpase

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

comment:12 Changed 2 years ago by git

  • Commit changed from d4378cbfd543cd10f2716a2c918b2c01ee265bbe to 8fe9503a580b506aec1abc0de2fafaf0d2c5501d

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

8fe9503check for deps of ntl

comment:13 Changed 2 years ago by 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).

comment:14 Changed 2 years ago by dimpase

  • Cc embray fbissey added

comment:15 follow-up: Changed 2 years ago by 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?

comment:16 in reply to: ↑ 15 ; follow-up: Changed 2 years ago by 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.

comment:17 in reply to: ↑ 16 Changed 2 years ago by 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.

comment:18 Changed 2 years ago by dimpase

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

Changed 2 years ago by dimpase

updated configure spkg

comment:19 Changed 2 years ago by git

  • Commit changed from 8fe9503a580b506aec1abc0de2fafaf0d2c5501d to 8473c7f9bbb37ab337bd32e2756106664c42c56b

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

8473c7fbumped up configure spkg

comment:20 Changed 2 years ago by dimpase

arrgh, and why is this cannot be merged?

comment:21 Changed 2 years ago by git

  • Commit changed from 8473c7f9bbb37ab337bd32e2756106664c42c56b to bc2f2778cb064293278d89f2c6ec7e3faddb0c75

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

1d165a8deal with --with-ntl, proper version check
5817a99flint needs --with-ntl= always
6979e43check for deps of ntl
bc2f277bumped up configure spkg

comment:22 Changed 2 years ago by dimpase

  • Description modified (diff)

comment:23 Changed 2 years ago by dimpase

  • 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 2 years ago by git

  • Commit changed from bc2f2778cb064293278d89f2c6ec7e3faddb0c75 to 1353705a47090fe937e34696f8bfb2f0f48ebdc4

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

e758967adding missing then in if
1c7c182Merge branch 'barvinokfix1' into ntlconf
1353705another missing then

comment:25 Changed 2 years ago by dimpase

  • Dependencies changed from #27212 to #27212, #27751
  • Status changed from needs_work to needs_review

comment:26 Changed 2 years ago by dimpase

  • Cc jdemeyer added

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

comment:27 Changed 2 years ago by embray

  • Component changed from build to build: configure
  • Keywords spkg-configure ntl added

comment:28 Changed 2 years ago by 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.

comment:29 Changed 2 years ago by dimpase

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

comment:30 Changed 2 years ago by 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?)

comment:31 Changed 2 years ago by 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.

comment:32 Changed 2 years ago by 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.

comment:33 Changed 2 years ago by 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.

comment:34 Changed 2 years ago by dimpase

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

comment:35 Changed 2 years ago by embray

  • 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:

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
d09da57deal with --with-ntl, proper version check
2ebf5b3flint needs --with-ntl= always
bc10f2echeck for deps of ntl
f16e43eClean up ntl/spkg-configure.m4
58cb21fcleanup tabs
3a900fcRemove unnecessary check for gf2x
27f7e58Fix NTL version check logic.

comment:36 Changed 2 years ago by embray

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

comment:37 Changed 2 years ago by git

  • Commit changed from 27f7e5859d2fed6b946170662874895e44561282 to 2f2d3e1a2606b57ae813b6d28d8c99952fef9bfd

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

2f2d3e1Accidentally swapped major/minor versions for minimum NTL version

comment:38 Changed 2 years ago by embray

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

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

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

comment:40 Changed 2 years ago by dimpase

  • Authors changed from Dima Pasechnik to Dima Pasechnik, Erik Bray
  • 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 2 years ago by 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?

comment:42 in reply to: ↑ 39 Changed 2 years ago by 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.

comment:43 Changed 2 years ago by 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.

comment:44 Changed 2 years ago by embray

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 2 years ago by git

  • 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:

d16db1cTrac #27265: Add spkg-configure.m4 for ntl

comment:46 Changed 2 years ago by embray

Rebased on 8.8.beta5 and squashed some commits.

comment:47 Changed 2 years ago by embray

  • 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 git

  • Commit changed from d16db1cc2671fa816d40a84436268dfb5d36f003 to 974c4831f775088bfd43f4efd5e5c453fcd44175

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
d15ead5Trac #27259: spkg-configure for mpc, adjustments for its dependents
974c483Trac #27265: Add spkg-configure.m4 for ntl

comment:49 Changed 2 years ago by embray

  • Status changed from needs_work to needs_review

comment:50 Changed 2 years ago by embray

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

comment:51 Changed 2 years ago by dimpase

  • Status changed from needs_review to positive_review

lgtm

comment:52 Changed 2 years ago by dimpase

  • 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 git

  • Commit changed from 974c4831f775088bfd43f4efd5e5c453fcd44175 to e025f888872035a04bcdbd047409c0e99a92dd14

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

e025f88flint needs --with-ntl= always

comment:54 Changed 2 years ago by dimpase

  • Status changed from needs_work to positive_review

comment:55 Changed 2 years ago by git

  • Commit changed from e025f888872035a04bcdbd047409c0e99a92dd14 to deaa7e0c9efb9df512c7d4f58a87df5759a3fbb2
  • 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:

c40427acorrect use of AC_LINK_IFELSE
deaa7e0Merge branch 'public/packages/ntlconf' of trac.sagemath.org:sage into wip

comment:56 Changed 2 years ago by dimpase

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: Changed 2 years ago by 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.

comment:58 Changed 2 years ago by embray

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

comment:59 follow-up: Changed 2 years ago by 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...

comment:60 in reply to: ↑ 57 Changed 2 years ago by 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.

comment:61 in reply to: ↑ 59 Changed 2 years ago by 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.

comment:62 Changed 2 years ago by git

  • Commit changed from deaa7e0c9efb9df512c7d4f58a87df5759a3fbb2 to 17af3bad767bfe0ccc5f63750da5e82ad3533d3e

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

17af3baflint needs --with-ntl= always

comment:63 Changed 2 years ago by dimpase

rebased/squashed to remove the last merge commit.

comment:64 Changed 2 years ago by dimpase

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

comment:65 Changed 2 years ago by 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.

comment:66 Changed 2 years ago by embray

  • Status changed from needs_review to positive_review

comment:67 Changed 2 years ago by git

  • 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:

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
d09da57deal with --with-ntl, proper version check
2ebf5b3flint needs --with-ntl= always
bc10f2echeck for deps of ntl
f16e43eClean up ntl/spkg-configure.m4
58cb21fcleanup tabs
3a900fcRemove unnecessary check for gf2x
27f7e58Fix NTL version check logic.
d98a05ecorrect use of AC_LINK_IFELSE
74b1451flint needs --with-ntl= always

comment:68 Changed 2 years ago by embray

Oops, rebased an older version of the branch.

comment:69 Changed 2 years ago by git

  • Commit changed from 74b1451ee15263868c44386b03f272ea6da966ae to 4b3caa7b35b0f0effb442feb18e4cc2c574cbb8e

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
d15ead5Trac #27259: spkg-configure for mpc, adjustments for its dependents
974c483Trac #27265: Add spkg-configure.m4 for ntl
96a7d77correct use of AC_LINK_IFELSE
4b3caa7flint needs --with-ntl= always

comment:70 Changed 2 years ago by embray

  • Status changed from needs_review to positive_review

comment:71 Changed 2 years ago by embray

  • Description modified (diff)

comment:72 Changed 2 years ago by dimpase

Missing quotes here:

  • build/pkgs/ntl/spkg-configure.m4

    a b SAGE_SPKG_CONFIGURE([ntl], [ 
    1919        AC_LINK_IFELSE([
    2020            AC_LANG_PROGRAM([[#include <NTL/ZZ.h>]],
    2121                            [[NTL::ZZ a;]]
    22             )], [LIBS=$LIBS -lntl]
     22            )], [LIBS="$LIBS -lntl"]
    2323                [AC_MSG_RESULT([yes])], [
    2424            AC_MSG_RESULT([no]); sage_spkg_install_ntl=yes
    2525        ])

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

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

comment:73 Changed 2 years ago by git

  • 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:

892143aadd missing quotes

comment:74 Changed 2 years ago by dimpase

  • Status changed from needs_review to positive_review

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

comment:75 Changed 23 months ago by vbraun

  • 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 dimpase

what makes you think I did not do this?

comment:77 Changed 23 months ago by dimpase

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

comment:78 Changed 23 months ago by dimpase

  • 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 22 months ago by 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?

comment:80 follow-up: Changed 22 months ago by embray

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: Changed 22 months ago by 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.

comment:82 in reply to: ↑ 81 Changed 22 months ago by 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.

comment:83 Changed 21 months ago by chapoton

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