Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to NTL 11.3.2 #25532

Closed
slel opened this issue Jun 7, 2018 · 106 comments
Closed

Upgrade to NTL 11.3.2 #25532

slel opened this issue Jun 7, 2018 · 106 comments

Comments

@slel
Copy link
Member

slel commented Jun 7, 2018

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

CC: @kiwifb @timokau @infinity0 @jdemeyer @jpflori @nexttime @saraedum @slel @tobihan @tscrim @vbraun

Component: packages: standard

Keywords: upgrade, ntl

Author: Timo Kaufmann, François Bissey

Branch: 9aa1288

Reviewer: Dima Pasechnik, Volker Braun, Travis Scrimshaw, Erik Bray

Issue created by migration from https://trac.sagemath.org/ticket/25532

@slel slel added this to the sage-8.3 milestone Jun 7, 2018
@timokau

This comment has been minimized.

@timokau timokau changed the title Upgrade to NTL 11.1.0 Upgrade to NTL 11.2.0 Jul 9, 2018
@timokau
Copy link
Contributor

timokau commented Jul 9, 2018

Branch: public/25532

@timokau
Copy link
Contributor

timokau commented Jul 9, 2018

Commit: 7f8e380

@timokau
Copy link
Contributor

timokau commented Jul 9, 2018

Dependencies: 23341

@timokau
Copy link
Contributor

timokau commented Jul 9, 2018

comment:5

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 flintlib/flint#487). However sagelib won't build because lcalc still compiles with gnu++98 (#23341).


New commits:

7f8e380Upgrade ntl to 11.2.0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2018

Changed commit from 7f8e380 to 6085b99

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2018

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

6085b99Upgrade ntl to 11.2.0

@timokau
Copy link
Contributor

timokau commented Jul 9, 2018

comment:7

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

@jdemeyer
Copy link

jdemeyer commented Jul 9, 2018

comment:8

Add a # in front of ticket numbers.

@jdemeyer
Copy link

jdemeyer commented Jul 9, 2018

Changed dependencies from 23341 to #23341

@jdemeyer
Copy link

jdemeyer commented Jul 9, 2018

comment:9

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

@antonio-rojas
Copy link
Contributor

comment:10

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

@timokau
Copy link
Contributor

timokau commented Jul 10, 2018

comment:11

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

@jdemeyer
Copy link

comment:12

Replying to @antonio-rojas:

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.

@timokau
Copy link
Contributor

timokau commented Jul 11, 2018

comment:13

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

@kiwifb
Copy link
Member

kiwifb commented Jul 12, 2018

comment:14

Replying to @timokau:

I'm not sure its the only issue, its just the first non-trivial one I encountered. @kiwifb 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.

@timokau
Copy link
Contributor

timokau commented Jul 12, 2018

comment:15

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

@kiwifb
Copy link
Member

kiwifb commented Jul 12, 2018

comment:16

Replying to @timokau:

Oh I just assumed that you used ntl 11 with sage because you mentioned in dimpase/lcalc#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.

@kiwifb
Copy link
Member

kiwifb commented Jul 12, 2018

comment:17

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.

@kiwifb
Copy link
Member

kiwifb commented Jul 12, 2018

comment:18

But really we should talk about lcalc in another ticket.

@timokau
Copy link
Contributor

timokau commented Jul 12, 2018

comment:19

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 @jdemeyer suggested (to "just" remove the dependency).

@timokau

This comment has been minimized.

@timokau timokau changed the title Upgrade to NTL 11.2.0 Upgrade to NTL 11.2.1 Jul 15, 2018
@timokau
Copy link
Contributor

timokau commented Jul 17, 2018

comment:21

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.

@kiwifb
Copy link
Member

kiwifb commented Jul 17, 2018

comment:22

Replying to @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?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 1, 2019

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 1, 2019

Changed commit from 9e95e78 to 9aa1288

@kiwifb
Copy link
Member

kiwifb commented Apr 1, 2019

comment:81

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

@tscrim
Copy link
Collaborator

tscrim commented Apr 4, 2019

Changed reviewer from Dima Pasechnik, Volker Braun to Dima Pasechnik, Volker Braun, Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Apr 4, 2019

comment:82

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

@embray
Copy link
Contributor

embray commented Apr 4, 2019

comment:83

Please give me a chance to test this on Cygwin.

@embray embray self-assigned this Apr 4, 2019
@embray
Copy link
Contributor

embray commented Apr 4, 2019

Changed reviewer from Dima Pasechnik, Volker Braun, Travis Scrimshaw to Dima Pasechnik, Volker Braun, Travis Scrimshaw, Erik Bray

@embray
Copy link
Contributor

embray commented Apr 4, 2019

comment:86

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.

@vbraun
Copy link
Member

vbraun commented Apr 7, 2019

Changed branch from public/25532 to 9aa1288

@dimpase
Copy link
Member

dimpase commented Apr 18, 2019

Changed commit from 9aa1288 to none

@dimpase
Copy link
Member

dimpase commented Apr 18, 2019

comment:88

how do I change -std=gnu++11 to c++11 for NTL? It seems that #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.

@kiwifb
Copy link
Member

kiwifb commented Apr 18, 2019

comment:89

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.

@dimpase
Copy link
Member

dimpase commented Apr 18, 2019

comment:90

Replying to @kiwifb:

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.

@kiwifb
Copy link
Member

kiwifb commented Apr 18, 2019

comment:91

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

@embray
Copy link
Contributor

embray commented Apr 19, 2019

comment:93

Replying to @kiwifb:

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

@dimpase
Copy link
Member

dimpase commented Apr 19, 2019

comment:94

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

@kiwifb
Copy link
Member

kiwifb commented Apr 19, 2019

comment:95

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants