Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#25532 closed enhancement (fixed)

Upgrade to NTL 11.3.2

Reported by: slelievre Owned by: embray
Priority: major Milestone: sage-8.8
Component: packages: standard Keywords: upgrade, ntl
Cc: fbissey, gh-timokau, infinity0, jdemeyer, jpflori, leif, saraedum, slelievre, thansen, tscrim, vbraun Merged in:
Authors: Timo Kaufmann, François Bissey Reviewers: Dima Pasechnik, Volker Braun, Travis Scrimshaw, Erik Bray
Report Upstream: N/A Work issues:
Branch: 9aa1288 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by slelievre)

The last upgrade in Sage was to NTL 10.3.0 in ticket #22869.

Change History (95)

comment:1 Changed 4 years ago by gh-antonio-rojas

  • Cc gh-antonio-rojas added

comment:2 Changed 4 years ago by arojas

  • Cc arojas added; gh-antonio-rojas removed

comment:3 Changed 4 years ago by gh-timokau

  • Cc gh-timokau added

comment:4 Changed 4 years ago by gh-timokau

  • Description modified (diff)
  • Summary changed from Upgrade to NTL 11.1.0 to Upgrade to NTL 11.2.0

comment:5 Changed 4 years ago by gh-timokau

  • Branch set to public/25532
  • Commit set to 7f8e3807d0ffe2e8f5b4e456d614ee4c12ca5ce0
  • Dependencies set to 23341

NTL compiles with -std=c++11 from 11.0 upwards. Apparently it is possible to compile with older c standards, but that loses thread safety.

I've added a work in progress PR that also updates the C standard for flint (necessary, see https://github.com/wbhart/flint2/issues/487). However sagelib won't build because lcalc still compiles with gnu++98 (#23341).


New commits:

7f8e380Upgrade ntl to 11.2.0

comment:6 Changed 4 years ago by git

  • Commit changed from 7f8e3807d0ffe2e8f5b4e456d614ee4c12ca5ce0 to 6085b990d8afd3b6011d4ec81c417de31d6f38f4

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

6085b99Upgrade ntl to 11.2.0

comment:7 Changed 4 years ago by gh-timokau

It is not necessary to explicitly set -std for flint, instead this relies on the compiler default (which certainly is >=11 now).


New commits:

6085b99Upgrade ntl to 11.2.0

comment:8 Changed 4 years ago by jdemeyer

  • Dependencies changed from 23341 to #23341

Add a # in front of ticket numbers.

comment:9 Changed 4 years ago by jdemeyer

Why does this depend on #23341? How is lcalc related to NTL?

comment:10 follow-ups: Changed 4 years ago by arojas

ntl 11 uses c++11, so "distutils: extra_compile_args = -std=gnu++98" has to be removed from sage/libs/lcalc/lcalc_Lfunction (which depends on both ntl and lcalc), so lcalc headers need to be c++11 compliant.

FWIW, I've already bumped ntl on Arch, and it mostly works fine in practice. The main issue is that it seems to bail out on some degenerate input, eg

> sage -t sage/matrix/matrix_integer_dense.pyx

[...]

sage: matrix(ZZ,3,[1..9]).hermite_form(algorithm='ntl') ## line 1807 ##

halt 2

comment:11 Changed 4 years ago by gh-timokau

Gentoo also has already updated ntl and apparently applied some patches to fix issues. https://github.com/dimpase/lcalc/pull/1 applies similar patches and updates lcalc to latest master (long unmaintained). That seems ready to merge, but was apparently forgotten or something.

comment:12 in reply to: ↑ 10 Changed 4 years ago by jdemeyer

Replying to arojas:

sage/libs/lcalc/lcalc_Lfunction (which depends on both ntl and lcalc)

It technically "depends" on NTL but for no good reason.

If that's the only issue, untangling this dependency would be the easiest solution.

comment:13 follow-up: Changed 4 years ago by gh-timokau

I'm not sure its the only issue, its just the first non-trivial one I encountered. @fbissey probably knows more about that as he apparently already has proper patches for gentoo.

comment:14 in reply to: ↑ 13 Changed 4 years ago by fbissey

Replying to gh-timokau:

I'm not sure its the only issue, its just the first non-trivial one I encountered. @fbissey probably knows more about that as he apparently already has proper patches for gentoo.

I'll confess that I am not sure what we are talking about here. Patch to ntl or to sage? At the moment I am using stock ntl-10.5.0 from the gentoo tree that has no patch. I don't remember doing anything special to sage to use it. I have an ebuild for 11.1.0/11.2.0 in a private repository that again is not patched - the only thing to be careful of is that to enable threads in ntl you need at least gf2x-1.2.

comment:15 follow-up: Changed 4 years ago by gh-timokau

Oh I just assumed that you used ntl 11 with sage because you mentioned in https://github.com/dimpase/lcalc/pull/1 that you patched your lcalc for c++11 support.

comment:16 in reply to: ↑ 15 Changed 4 years ago by fbissey

Replying to gh-timokau:

Oh I just assumed that you used ntl 11 with sage because you mentioned in https://github.com/dimpase/lcalc/pull/1 that you patched your lcalc for c++11 support.

I seriously need to check to see if I use C++11 with lcalc. I have a good memory but not that good.

comment:17 Changed 4 years ago by fbissey

OK I patch enough that it compile out of the box with g++-7.3.0 without passing a -std=c++... flag so I am guessing I get gnu++11 at the least. Lots of warnings that stuff is deprecated or forbidden, but g++ takes it. clang++, on the other hand, breaks

fatal error: 'bits/c++config.h' file not found

Of course no one should use those kinds of headers in the first place. I am sure it is fixed in the pull request you are quoting at least.

comment:18 Changed 4 years ago by fbissey

But really we should talk about lcalc in another ticket.

comment:19 Changed 4 years ago by gh-timokau

Yes, that would be #23341. Apparently the work is pretty much done but there is some small regression (?) nobody felt qualified to make a judgement about yet.

More relevant to the discussion in this ticket would be the approach @jdmeyer suggested (to "just" remove the dependency).

comment:20 Changed 4 years ago by gh-timokau

  • Description modified (diff)
  • Summary changed from Upgrade to NTL 11.2.0 to Upgrade to NTL 11.2.1

comment:21 follow-up: Changed 4 years ago by gh-timokau

When I tried to update to the last version before 11.0 (10.5.0) I got an interesting error:

File "/nix/store/lv32600c4zla3ldyd0l2sz7sr5y6bhkp-sage-src-8.2/src/sage/misc/trace.py", line 67, in sage.misc.trace.trace
Failed example:
    print(s.before[s.before.find('--'):])
Expected:
    --...
    ipdb> c
    2 * 5
Got:
    --Call--
    > <CSI-0;32m>/nix/store/bs9cxmkyslmnizh9gl7gj4

10.4.0 is the last version that works without any changes.

comment:22 in reply to: ↑ 21 Changed 4 years ago by fbissey

Replying to gh-timokau:

When I tried to update to the last version before 11.0 (10.5.0) I got an interesting error:

File "/nix/store/lv32600c4zla3ldyd0l2sz7sr5y6bhkp-sage-src-8.2/src/sage/misc/trace.py", line 67, in sage.misc.trace.trace
Failed example:
    print(s.before[s.before.find('--'):])
Expected:
    --...
    ipdb> c
    2 * 5
Got:
    --Call--
    > <CSI-0;32m>/nix/store/bs9cxmkyslmnizh9gl7gj4

10.4.0 is the last version that works without any changes.

I am currently on 10.5.0 in gentoo, how do you get that problem?

comment:23 Changed 4 years ago by gh-timokau

Huh. I tried to reproduce it to be sure, but I couldn't. If I remember correctly the time it occured I only changed the ntl version. Maybe I changed something else too and forgot it, maybe its a transient failure. I'll see if it occurs again some time.

Thanks for telling me.

comment:24 in reply to: ↑ 10 Changed 4 years ago by arojas

Replying to arojas:

FWIW, I've already bumped ntl on Arch, and it mostly works fine in practice. The main issue is that it seems to bail out on some degenerate input, eg

> sage -t sage/matrix/matrix_integer_dense.pyx

[...]

sage: matrix(ZZ,3,[1..9]).hermite_form(algorithm='ntl') ## line 1807 ##

halt 2

Scratch that: this is (surprisingly) caused by the Singular 4.1.1 upgrade, not by the NTL one.

comment:25 follow-up: Changed 4 years ago by gh-timokau

How did you get sage to build with ntl >= 11? Did you patch lcalc? Any other patches necessary?

comment:26 in reply to: ↑ 25 Changed 4 years ago by arojas

Replying to gh-timokau:

How did you get sage to build with ntl >= 11? Did you patch lcalc? Any other patches necessary?

Apply the patch from #23341 to lcalc and

https://git.archlinux.org/svntogit/community.git/tree/trunk/sagemath-lcalc-c++11.patch?h=packages/sagemath

comment:27 Changed 4 years ago by gh-timokau

I tried to untangle the (apparently unnecessary) lcalc <- ntl dependency. I failed. NTL apparently gets added to the inclue_dirs of pretty much every sage extension, including lcalc. That is even after I (as an experiment)

  • removed ntl from cython.pys standard_libs
  • removed ntl from setup.pys lib_headers

I couldn't figure out why ntl was still added. Jeroen, do you know why?

comment:28 Changed 3 years ago by slelievre

  • Cc infinity0 saraedum thansen added
  • Description modified (diff)
  • Keywords upgrade ntl added
  • Milestone changed from sage-8.3 to sage-8.5
  • Summary changed from Upgrade to NTL 11.2.1 to Upgrade to NTL 11.3.1

comment:29 Changed 3 years ago by gh-timokau

For what its worth I have since also adopted this archlinux patch (thanks Antonio!) and haven't seen any issues.

Last edited 3 years ago by gh-timokau (previous) (diff)

comment:30 Changed 3 years ago by slelievre

NTL 11.3.2 was released, as announced on sage-devel.

comment:31 Changed 3 years ago by slelievre

  • Description modified (diff)

comment:32 Changed 3 years ago by slelievre

  • Summary changed from Upgrade to NTL 11.3.1 to Upgrade to NTL 11.3.2

comment:33 Changed 3 years ago by git

  • Commit changed from 6085b990d8afd3b6011d4ec81c417de31d6f38f4 to bb47a03d2bd4d597b521eee7f7c9609a11706faf

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

59546fbMerge branch 'develop' into ntl-11.3.2
bb47a03Update to 11.3.2

comment:34 Changed 3 years ago by fbissey

  • Authors set to Timo Kaufmann, François Bissey

New commits:

59546fbMerge branch 'develop' into ntl-11.3.2
bb47a03Update to 11.3.2

comment:35 Changed 3 years ago by fbissey

  • Status changed from new to needs_review

comment:36 follow-up: Changed 3 years ago by jpflori

I've not tested it (with or without the specific hunk) but is the flint export CFLAGS hunk necessary? Didn't we pass CFLAGS to flint before?

comment:37 in reply to: ↑ 36 ; follow-up: Changed 3 years ago by fbissey

Replying to jpflori:

I've not tested it (with or without the specific hunk) but is the flint export CFLAGS hunk necessary? Didn't we pass CFLAGS to flint before?

I am not sure what motivated that. @Timo can you comment here?

In any case there is an export CFLAGS in the conditional for debugging and one outside. We could just keep the one outside.

Could it be that it was meant for CXXFLAGS instead and add -std=c++11 there? There is a single object built with C++ in flint, the ntl interface. I cannot test that theory because I don't have a compiler old enough that would default to something lower than c++11.

Last edited 3 years ago by fbissey (previous) (diff)

comment:38 follow-up: Changed 3 years ago by dimpase

I have a weird problem building this NTL with clang on FreeBSD. It keeps telling me that it is g++, and bails out with some weird error. Could you try clang on another platform?

comment:39 in reply to: ↑ 38 Changed 3 years ago by fbissey

Replying to dimpase:

I have a weird problem building this NTL with clang on FreeBSD. It keeps telling me that it is g++, and bails out with some weird error. Could you try clang on another platform?

Works as expected with clang+linux. More details please.

comment:40 Changed 3 years ago by jpflori

Looks ok on macos without gf2x and outside of Sage. configure knows g++ is clang and make calls g++ but finishes ok.

comment:41 Changed 3 years ago by dimpase

Running manually at sage -sh prompt: to start with:

$ CC=clang CXX=clang++ ./configure
compiler_name=gcc
language_standard=2014
cpu_type=x86

setting TUNE=x86

*** checking -pthread flag
   CXXAUTOFLAGS=" -pthread"
   -pthread works
*** checking -march=native flag
   CXXAUTOFLAGS=" -pthread -march=native"
   -march=native works
*** checking -ffp-contract=off flag
   -ffp-contract=off works
*** threads are OK 

CXXAUTOFLAGS=" -pthread -march=native"
NOCONTRACT="-ffp-contract=off -DNTL_CONTRACTION_FIXED"

generated makefile
generated ../include/NTL/config.h
generated ../include/NTL/ConfigLog.h

why gcc? and why x86, not x86_64? Then,

$ gmake
make clobber
rm -f ntl.a mach_desc.h ../include/NTL/mach_desc.h  GetTime.cpp GetPID.cpp
sh ResetFeatures '..' "ALIGNED_ARRAY BUILTIN_CLZL LL_TYPE SSSE3 AVX PCLMUL AVX2 FMA AVX512F  COPY_TRAITS1 COPY_TRAITS2 CHRONO_TIME MACOS_TIME POSIX_TIME"
rm -f ../include/NTL/gmp_aux.h
sh RemoveProg QuickTest ZZTest SSMulTest ZZ_pXTest lzz_pXTest BerlekampTest CanZassTest  ZZXFacTest MoreFacTest LLLTest   BitMatTest MatrixTest mat_lzz_pTest CharPolyTest RRTest QuadTest GF2XTest   GF2EXTest GF2EXGCDTest subset ZZ_pEXTest ZZ_pEXGCDTest lzz_pEXTest lzz_pEXGCDTest  Timing ThreadTest MakeDesc TestGetTime TestGetPID CheckFeatures CheckCompile GenConfigInfo CheckContract  CheckThreads gen_gmp_aux gf2x_version_1_2_or_later_required
rm -f *.o
rm -rf small
rm -f cfileout mfileout
rm -rf .libs *.lo libntl.la
rm -f setup-phase
make setup1
g++ -I../include -I.  -g -O2 -pthread -march=native  -c MakeDescAux.cpp
...

on FreeBSD g++ and clang++, or c++ are totally different beasts, and they can live together in the same PATH (but not in this case it seems)

comment:42 Changed 3 years ago by dimpase

Removing gcc/g++ results in

$ CC=clang CXX=clang++ ./configure
Compilation failed
See CompilerOutput.log for details
Goodbye! at DoConfig line 616.
$ CC=cc CXX=c++ ./configure
Compilation failed
See CompilerOutput.log for details
Goodbye! at DoConfig line 616.
$ cat CompilerOutput.log 
*** CompilerOutput.log ***
*** building GenConfigInfo
g++ -I../include -I.  -g -O2   -o  GenConfigInfo GenConfigInfo.cpp -lm
make: exec(g++) failed (No such file or directory)
*** Error code 1
...

That is, it only works with g++/gcc. A bug, I'd say. In DoConfig, on sees 'CXX' => 'g++' - I don't know Perl, but I gather it means that CXX is hardcoded to be g++.

It works with NTL 10, don't ask me how...

comment:43 Changed 3 years ago by fbissey

This is silly

$ CC=clang CXX=clang++ ./configure 
compiler_name=gcc
language_standard=2014
cpu_type=x86

setting TUNE=x86

*** checking -pthread flag
   CXXAUTOFLAGS=" -pthread"
   -pthread works
*** checking -march=native flag
   CXXAUTOFLAGS=" -pthread -march=native"
   -march=native works
*** checking -ffp-contract=off flag
   -ffp-contract=off works
*** threads are OK 

CXXAUTOFLAGS=" -pthread -march=native"
NOCONTRACT="-ffp-contract=off -DNTL_CONTRACTION_FIXED"

generated makefile
generated ../include/NTL/config.h
generated ../include/NTL/ConfigLog.h

but

$ ./configure CXX=clang++
compiler_name=clang
language_standard=2014
cpu_type=x86

setting TUNE=x86

*** checking -pthread flag
   CXXAUTOFLAGS=" -pthread"
   -pthread works
*** checking -march=native flag
   CXXAUTOFLAGS=" -pthread -march=native"
   -march=native works
*** threads are OK 

CXXAUTOFLAGS=" -pthread -march=native"
NOCONTRACT=""

generated makefile
generated ../include/NTL/config.h
generated ../include/NTL/ConfigLog.h

comment:44 Changed 3 years ago by fbissey

The thing is CXX is an option of the DoConfig script. It is not pulled from an environment variable.

comment:45 Changed 3 years ago by dimpase

Hmm, OK, over to Sage's installation. If I start as

$ MAKE="gmake -j8" gmake

NTL fails with the usual invitation to look in the following file for errors, and there I see

$ cat src/src/CompilerOutput.log 
*** CompilerOutput.log ***
*** building GenConfigInfo
make[4]: illegal argument to -j -- must be positive integer!

So it seems to assume something about $MAKE...

And if I start as

$ MAKE="gmake" gmake

I get even weirder

*** CompilerOutput.log ***
*** building GenConfigInfo
make[4]: don't know how to make w. Stop
...

comment:46 Changed 3 years ago by dimpase

OK, here is the fix:

  • build/pkgs/ntl/spkg-install

    diff --git a/build/pkgs/ntl/spkg-install b/build/pkgs/ntl/spkg-install
    index 08fb2b148c..88ee9548ea 100644
    a b ntl_configure() 
    9999        LDFLAGS="$LDFLAGS" LIBTOOL_LINK_FLAGS="$LIBTOOL_LINK_FLAGS" \
    100100        NTL_GMP_LIP=on NTL_GF2X_LIB=on \
    101101        "$DISABLE_NATIVE" \
     102        MAKE_PROG="$MAKE" \
    102103        NTL_THREADS=off || sdh_die "Error configuring NTL."
    103104}
    104105

Could you add it to the branch?

comment:47 Changed 3 years ago by fbissey

I really need to go to bed now :( but this is a public branch, you are welcome to push to it. And well spotted.

comment:48 Changed 3 years ago by git

  • Commit changed from bb47a03d2bd4d597b521eee7f7c9609a11706faf to 292e697e8fc1b6bfc5279f99a94003776c3243a9

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

292e697tell NTL what MAKE to use

comment:49 Changed 3 years ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

comment:50 in reply to: ↑ 37 Changed 3 years ago by gh-timokau

Replying to fbissey:

Replying to jpflori:

I've not tested it (with or without the specific hunk) but is the flint export CFLAGS hunk necessary? Didn't we pass CFLAGS to flint before?

I am not sure what motivated that. @Timo can you comment here?

In any case there is an export CFLAGS in the conditional for debugging and one outside. We could just keep the one outside.

Could it be that it was meant for CXXFLAGS instead and add -std=c++11 there? There is a single object built with C++ in flint, the ntl interface. I cannot test that theory because I don't have a compiler old enough that would default to something lower than c++11.

I don't remember my reasoning. Probably some leftover of trying to get it to work nicely with the old lcalc.

Thanks for getting this mergeable :)

comment:51 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

This borks flint on some machines, i guess missing -std=c11

make[7]: Leaving directory '/home/buildbot/slave/sage_git/build/local/var/tmp/sage/build/flint-2.5.2.p3/src/fq_zech_poly_factor'
if [ "1" -eq "1" ]; then \
  make build/interfaces/NTL-interface.lo; \
  g++   -shared -Wl,-soname,libflint.so.13 -Wl,-rpath,/home/buildbot/slave/sage_git/build/local/lib -Wl,-rpath,/home/buildbot/slave/sage_git/build/local/lib build/interfaces/NTL-interface.lo  build/printf.lo  build/fprintf.lo  build/sprintf.lo  build/scanf.lo  build/fscanf.lo  build/sscanf.lo  build/clz_tab.lo  build/memory_manager.lo  build/version.lo  build/profiler.lo  build/thread_support.lo  build/ulong_extras.lo  build/long_extras.lo  build/perm.lo  build/fmpz.lo  build/fmpz_vec.lo  build/fmpz_poly.lo  build/fmpq_poly.lo  build/fmpz_mat.lo  build/fmpz_lll.lo  build/mpfr_vec.lo  build/mpfr_mat.lo  build/mpf_vec.lo  build/mpf_mat.lo  build/nmod_vec.lo  build/nmod_poly.lo  build/nmod_poly_factor.lo  build/arith.lo  build/mpn_extras.lo  build/nmod_mat.lo  build/fmpq.lo  build/fmpq_vec.lo  build/fmpq_mat.lo  build/padic.lo  build/fmpz_poly_q.lo  build/fmpz_poly_mat.lo  build/nmod_poly_mat.lo  build/fmpz_mod_poly.lo  build/fmpz_mod_poly_factor.lo  build/fmpz_factor.lo  build/fmpz_poly_factor.lo  build/fft.lo  build/qsieve.lo  build/double_extras.lo  build/d_vec.lo  build/d_mat.lo  build/padic_poly.lo  build/padic_mat.lo  build/qadic.lo  build/fq.lo  build/fq_vec.lo  build/fq_mat.lo  build/fq_poly.lo  build/fq_poly_factor.lo  build/fq_nmod.lo  build/fq_nmod_vec.lo  build/fq_nmod_mat.lo  build/fq_nmod_poly.lo  build/fq_nmod_poly_factor.lo  build/fq_zech.lo  build/fq_zech_vec.lo  build/fq_zech_mat.lo  build/fq_zech_poly.lo  build/fq_zech_poly_factor.lo  -o libflint.so.13.5.2 -L/home/buildbot/slave/sage_git/build/local/lib -Wl,-rpath,/home/buildbot/slave/sage_git/build/local/lib  -L/home/buildbot/slave/sage_git/build/local/var/tmp/sage/build/flint-2.5.2.p3/src -L/home/buildbot/slave/sage_git/build/local/lib -L/home/buildbot/slave/sage_git/build/local/lib -L/home/buildbot/slave/sage_git/build/local/lib -lmpfr -lgmp -lm -lntl -lpthread ; \
fi
make[7]: Entering directory '/home/buildbot/slave/sage_git/build/local/var/tmp/sage/build/flint-2.5.2.p3/src'
g++ -fPIC  -I/home/buildbot/slave/sage_git/build/local/var/tmp/sage/build/flint-2.5.2.p3/src -I/home/buildbot/slave/sage_git/build/local/include -I/home/buildbot/slave/sage_git/build/local/include -I/home/buildbot/slave/sage_git/build/local/include -c interfaces/NTL-interface.cpp -o build/interfaces/NTL-interface.lo
In file included from /usr/include/c++/4.9/type_traits:35:0,
                 from /home/buildbot/slave/sage_git/build/local/include/NTL/tools.h:25,
                 from /home/buildbot/slave/sage_git/build/local/include/NTL/ZZ.h:19,
                 from interfaces/NTL-interface.cpp:32:
/usr/include/c++/4.9/bits/c++0x_warning.h:32:2: error: #error This file requires compiler and library support for the ISO C++ 2011 standard. This support is currently experimental, and must be enabled with the -std=c++11 or -std=gnu++11 compiler options.
 #error This file requires compiler and library support for the \
  ^
In file included from /home/buildbot/slave/sage_git/build/local/include/NTL/ZZ.h:19:0,
                 from interfaces/NTL-interface.cpp:32:
/home/buildbot/slave/sage_git/build/local/include/NTL/tools.h:1075:1: error: 'constexpr' does not name a type
 constexpr bool Relocate_aux_has_trivial_copy(T*)
 ^
/home/buildbot/slave/sage_git/build/local/include/NTL/tools.h:1075:1: note: C++11 'constexpr' only available with -std=c++11 or -std=gnu++11
/home/buildbot/slave/sage_git/build/local/include/NTL/tools.h:1083:1: error: 'constexpr' does not name a type
 constexpr bool Relocate_aux_has_any_copy(T*)

comment:52 follow-up: Changed 3 years ago by vbraun

Also, how about setting -std=c11 globally (like LDFLAGS rpath) instead of a per-package whackamole...

comment:53 in reply to: ↑ 52 Changed 3 years ago by dimpase

Replying to vbraun:

Also, how about setting -std=c11 globally (like LDFLAGS rpath) instead of a per-package whackamole...

Do you mean c++11 in CXXFLAGS?

There are packages (brial is one case) needing -std=gnu++11.

But yes, it appears now that the compilers must be told to enforce c++11 globally, otherwise some compilers default to something else.

So it seems it cannot be just added, it needs to be a overwriteable per package default in CXXFLAGS...

comment:54 Changed 3 years ago by fbissey

To be honest I should have seen that coming and I should have acted in https://trac.sagemath.org/ticket/25532#comment:37 .

Now for the idea to set std=c++11 globally by default. Well we have a variety of compilers out there defaulting to anything from pre-c++11 to c++14 and soon c++17. I don't think we should enforce going from c++14 to c++11 unless we have too. By the way brial compiles perfectly with gcc-8.2.0 which default to c++14 without having to add std=gnu++11.

In any case we can solve that at configure time with the appropriate macro (like brial does now) to check that the compiler support at least this standard and add the appropriate flag to CXXFLAGS if it doesn't.

I would like to finish this ticket as is and I'll note that since #23341 is merged distro are free to upgrade ntl and we do. This ticket is just about keeping up.

We should have a follow up ticket about minimal c++11 support and clean up for 8.6 in my opinion. This would a major feature and I don't want to start it now.

comment:55 Changed 3 years ago by git

  • Commit changed from 292e697e8fc1b6bfc5279f99a94003776c3243a9 to 94ef0695d3b624cb6dfb64b32ed9fb10113e030e

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

94ef069enforce c++11 in flint interface to ntl.

comment:56 Changed 3 years ago by fbissey

  • Reviewers changed from Dima Pasechnik to Dima Pasechnik, Volker Braun
  • Status changed from needs_work to positive_review

To be more precise, we already check for support for C++11 but we don't enforce it if it isn't the default.

Back to positive review because I only made a trivial change.

comment:57 follow-up: Changed 3 years ago by dimpase

it is not kosher to mix CXXFLAGS with CFLAGS. there are CFLAGS which a C++ compiler won’t take.

comment:58 Changed 3 years ago by git

  • Commit changed from 94ef0695d3b624cb6dfb64b32ed9fb10113e030e to e4006a74d02ddc0e2b0615aa37fb4e29f773fc69
  • 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:

e4006a7CXXFLAGS all the way

comment:59 in reply to: ↑ 57 Changed 3 years ago by fbissey

Replying to dimpase:

it is not kosher to mix CXXFLAGS with CFLAGS. there are CFLAGS which a C++ compiler won’t take.

Thank you for paying attention (very embarrassed).

comment:60 follow-up: Changed 3 years ago by dimpase

NTL does depend on flint, but this is not reflected in build/pkgs/ntl/dependencies. Should we fix this here too?

Also, I imagine CXXFLAGS of mpir/gmp and gf2x need attention too.

comment:61 in reply to: ↑ 60 Changed 3 years ago by fbissey

Replying to dimpase:

NTL does depend on flint, but this is not reflected in build/pkgs/ntl/dependencies. Should we fix this here too?

you got that in reverse. flint depends on ntl, so if ntl uses c++11 so does the flint-ntl interface (there is a single C++ file in flint for this interface, the rest is all C).

Also, I imagine CXXFLAGS of mpir/gmp and gf2x need attention too.

It doesn't currently have any impact, there seem to be some forward compatibility. flint and sage would require backward compatibility.

comment:62 Changed 3 years ago by fbissey

  • Status changed from needs_review to positive_review

Didn't know that pushing to git makes the ticket go back to "need review". Nice behavior in fact.

comment:63 follow-up: Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

I'm getting quite a lot of

checking for NTL >= 5.0... not found
configure: WARNING: Unable to find NTL (which is strongly recommended) on your machine: please use --with-ntl=PATH_TO_DIR_CONTAINING_LIB_AND_INCLUDE (see also ./configure --help if you do not understand what we are talking about)

in Singular. Fun fact: This does not make the build fail, but causes tons of test failures..

Fails on Ubuntu 14, 16, and Debian 8. Works on Ubuntu 18, Debian 9, OSX. Idependent of 32 vs. 64 bit.

comment:64 in reply to: ↑ 63 ; follow-up: Changed 3 years ago by fbissey

  • Milestone changed from sage-8.5 to sage-pending

Replying to vbraun:

I'm getting quite a lot of

checking for NTL >= 5.0... not found
configure: WARNING: Unable to find NTL (which is strongly recommended) on your machine: please use --with-ntl=PATH_TO_DIR_CONTAINING_LIB_AND_INCLUDE (see also ./configure --help if you do not understand what we are talking about)

in Singular. Fun fact: This does not make the build fail, but causes tons of test failures..

Fails on Ubuntu 14, 16, and Debian 8. Works on Ubuntu 18, Debian 9, OSX. Idependent of 32 vs. 64 bit.

Right. That means we would need to make -std=c++11 pervasive in singular as well and possibly all other dependencies of ntl that are not already switching c++11 in configure. So I think we should postpone this until we enforce c++11 across the board. I thought it would be easy but it is turning in a whack-a-mole kind of thing.

Last edited 3 years ago by fbissey (previous) (diff)

comment:65 in reply to: ↑ 64 Changed 3 years ago by arojas

Replying to fbissey:

Right. That means we would need to make -std=c++11 pervasive in singular as well and possibly all other dependencies of ntl that are not already switching c++11 in configure. So I think we should postpone this until we enforce c++11 across the board. I thought it would be easy but it is turning in a whack-a-mole kind of thing.

Singular already does that itself in 4.1.1.p3

comment:66 follow-up: Changed 3 years ago by fbissey

But we are stuck at 4.1.1.p2 here. Even if we finish the singular ticket, we may still have to look at giac which probably doesn't go c++11 by itself. eclib and linbox should be OK.

comment:67 in reply to: ↑ 66 Changed 3 years ago by dimpase

Replying to fbissey:

But we are stuck at 4.1.1.p2 here. Even if we finish the singular ticket, we may still have to look at giac which probably doesn't go c++11 by itself. eclib and linbox should be OK.

giac's upcoming version 1.5.0 should be compileable with c++11 - at least it does work with clang 6.0.1, which is quite picky, and I think enforces c++11 by default. See #26315.

comment:68 Changed 3 years ago by fbissey

1.4.9.xx compiles with gcc-8.2.0 which is c++14 by default, but of course that doesn't mean clang-6.0.1 (also c++14 I think) won't throw a fit at it. But here I suspect that Volker would find in his logs that giac got configured without ntl on those machines which may or may not cause more failures.

comment:69 Changed 3 years ago by fbissey

  • Dependencies changed from #23341 to #26787

comment:70 Changed 3 years ago by git

  • Commit changed from e4006a74d02ddc0e2b0615aa37fb4e29f773fc69 to 9e95e7860c3cab327f9007687494f3a798f65052

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

3668993Merge branch 'develop' into ntl-11.3.2
370c035Update CXX macros and enforce c++11
d34ff33Forgot to commit one change
af9eb58Merge branch 'cpp11' into ntl-11.3.2
0a4af16Adding std=c++11 is not helpful anymore
70f51d2remove c++11 enforcement in spkg in most cases
9e95e78Merge branch 'cpp11' into ntl-11.3.2

comment:71 Changed 3 years ago by fbissey

  • Milestone changed from sage-pending to sage-8.5
  • Status changed from needs_work to needs_review

Still depends on a good review of #26787 which needs a bit of testing on cygwin.

comment:72 Changed 3 years ago by jdemeyer

Can somebody either make this not depend on #26787 or add an explanation to #26787?

comment:73 Changed 3 years ago by dimpase

  • Milestone changed from sage-8.5 to sage-8.6

making it independent of #26787 needs passing std=c++11 in CXXFLAGS...

comment:74 Changed 3 years ago by embray

  • Milestone changed from sage-8.6 to sage-8.7

Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sage-pending or sage-wishlist.

comment:75 Changed 3 years ago by fbissey

  • Dependencies #26787 deleted
  • Status changed from needs_review to positive_review

Forgot about this ticket. All the dependencies for this ticket have been cleared so it should go back to positive review.

comment:76 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work
/home/buildbot/slave/sage_git/build/local/include/NTL/vector.h:198:31: warning: unused variable 'do_BlockConstruct' [-Wunused-variable]
 { VecStrategy<NTL_RELOC_TAG>::do_BlockConstruct(p, n); }
                               ^~~~~~~~~~~~~~~~~
Makefile:275: recipe for target 'build/interfaces/NTL-interface.lo' failed
make[7]: *** [build/interfaces/NTL-interface.lo] Error 1
make[7]: Leaving directory '/home/buildbot/slave/sage_git/build/local/var/tmp/sage/build/flint-2.5.2.p3/src'
g++: error: build/interfaces/NTL-interface.lo: No such file or directory
Makefile:143: recipe for target 'libflint.so.13.5.2' failed
make[6]: *** [libflint.so.13.5.2] Error 1

comment:77 Changed 3 years ago by fbissey

More details please! Version of gcc involved and possibly distro, more logs from flint too. Because this a package upgrade we don't see anything on the patchbot :(

comment:78 Changed 3 years ago by fbissey

OK found the issue. We don't pass any CFLAGS or CXXFLAGS to flint so the default are used. By default, -ansi is added to both CFLAGS and CXXFLAGS. For C is sets the standard to C99 and for C++, to c++98. Frankly I think we should just remove -ansi in both case. And since by default CXXFLAGS is copied from CFLAGS, that should be easy to do in one single change.

And distros don't choke on this because we usually provide some compilation flags.

comment:79 Changed 3 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:80 Changed 3 years ago by git

  • Commit changed from 9e95e7860c3cab327f9007687494f3a798f65052 to 9aa1288319f00c0ae230689768e884c080671b8f

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

26f0cb9Merge branch 'develop' into t25532
9aa1288flint shouldn't override the default language standard for C and C++

comment:81 Changed 3 years ago by fbissey

  • Status changed from needs_work to needs_review

flint issue is now fixed, please review.


New commits:

26f0cb9Merge branch 'develop' into t25532
9aa1288flint shouldn't override the default language standard for C and C++

comment:82 Changed 3 years ago by tscrim

  • Reviewers changed from Dima Pasechnik, Volker Braun to Dima Pasechnik, Volker Braun, Travis Scrimshaw
  • Status changed from needs_review to positive_review

I am kicking this back to the patchbots for final testing as this works on my machine.

comment:83 Changed 3 years ago by embray

  • Status changed from positive_review to needs_work

Please give me a chance to test this on Cygwin.

comment:84 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

comment:85 Changed 3 years ago by embray

  • Owner changed from (none) to embray

comment:86 Changed 3 years ago by embray

  • Reviewers changed from Dima Pasechnik, Volker Braun, Travis Scrimshaw to Dima Pasechnik, Volker Braun, Travis Scrimshaw, Erik Bray
  • Status changed from needs_review to positive_review

Alright, I think this is probably fine. I haven't run the full test suite yet, but it looks good based on an informed subset. If I notice anything after running the full tests I'll create a follow-up. Thank you for waiting.

comment:87 Changed 3 years ago by vbraun

  • Branch changed from public/25532 to 9aa1288319f00c0ae230689768e884c080671b8f
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:88 Changed 3 years ago by dimpase

  • Commit 9aa1288319f00c0ae230689768e884c080671b8f deleted

how do I change -std=gnu++11 to c++11 for NTL? It seems that https://trac.sagemath.org/ticket/27212#comment:112 is related to this. Indeed, if I instead use NTL built with c++11 at https://github.com/Homebrew/homebrew-core/blob/master/Formula/ntl.rb then everything works.

comment:89 follow-up: Changed 3 years ago by fbissey

I don't particularly like the fact that the macro for C++ standard looks for gnu++11 before c++11 but that's what it is. You can just add -std=c++11 to CXXFLAGS on OS X in spkg-install. That should work. There are work around of the sort for cygwin already in fplll if my memory serves me right.

comment:90 in reply to: ↑ 89 Changed 3 years ago by dimpase

Replying to fbissey:

I don't particularly like the fact that the macro for C++ standard looks for gnu++11 before c++11 but that's what it is. You can just add -std=c++11 to CXXFLAGS on OS X in spkg-install. That should work. There are work around of the sort for cygwin already in fplll if my memory serves me right.

no, this does not work. Which packages do need gnu++11 rather than c++11? I am trying now to set c++11 on Darwin globally, IIRC it used to work.

comment:91 follow-up: Changed 3 years ago by fbissey

OK to set up things globally you may want to go back to the macro setting c++11/gnu++11 as the standard at https://github.com/sagemath/sage/blob/develop/build/pkgs/gcc/spkg-configure.m4#L97 and follow the model of fplll that asks specifically for c++11 https://github.com/fplll/fplll/blob/master/configure.ac#L56 (but do not make it mandatory).

From what I see, it would be a good idea to update the macro /ax_cxx_compile_stdcxx as there is a recent fix for clang.

Now the question is whether you want to try c++11 instead as the general default or just for OS X. My understanding is that cygwin works better with gnu++11.

comment:92 Changed 3 years ago by arojas

  • Cc arojas removed

comment:93 in reply to: ↑ 91 Changed 3 years ago by embray

Replying to fbissey:

OK to set up things globally you may want to go back to the macro setting c++11/gnu++11 as the standard at https://github.com/sagemath/sage/blob/develop/build/pkgs/gcc/spkg-configure.m4#L97 and follow the model of fplll that asks specifically for c++11 https://github.com/fplll/fplll/blob/master/configure.ac#L56 (but do not make it mandatory).

From what I see, it would be a good idea to update the macro /ax_cxx_compile_stdcxx as there is a recent fix for clang.

Now the question is whether you want to try c++11 instead as the general default or just for OS X. My understanding is that cygwin works better with gnu++11.

Roughly speaking, as I understand it, the reason -std=gnu++11 is sometimes required on Cygwin is related to the fact that Cygwin's built-in libc is not glibc but rather Cygnus/Redhat?'s newlib. newlib supports most/all the usual GNU extensions but it is quite strict about requiring them to be explicitly enabled; otherwise you just get pure POSIX.

I have no idea what the problem is on macOS, but presumably there's something that is enabled in NTL when using -std=gnu++11 but it's probably something subtle.

comment:94 follow-up: Changed 3 years ago by dimpase

It turns out to be a red herring.

However, one thing I noticed that for fplll there is setting of -std=gnu++11 for Cygwin, whereas it's already set globally...

comment:95 in reply to: ↑ 94 Changed 3 years ago by fbissey

Replying to dimpase:

It turns out to be a red herring.

However, one thing I noticed that for fplll there is setting of -std=gnu++11 for Cygwin, whereas it's already set globally...

This is on purpose. Before I enabled c++11/gnu++11 globally, it was already in place. The reason is that fplll's configure script requires a strict c++11 compiler as noted above. But cygwin needs a gnu++11 one to compile fplll. So we need to override fplll's decision on cygwin. Don't go removing it.

Note: See TracTickets for help on using tickets.