Upgrade eclib to compile with xcode 9

[kiwifb]

Tarball: http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/eclib-20171002.tar.bz2

Upstream: Fixed upstream, in a later stable release.

CC: @jhpalmieri @isuruf

Component: packages: standard

Author: François Bissey, Isuru Fernando, Jeroen Demeyer

Branch/Commit: 5c878e6

Reviewer: John Cremona, John Palmieri, Dima Pasechnik

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

[kiwifb]

Author: François Bissey, Isuru Fernando

[kiwifb]

Commit: 235c09e

[kiwifb]

Branch: u/fbissey/eclib-xcode9

[kiwifb]

New commits:

235c09e

Including upstream PR 29 to compile with xcode 9 until we have a new release.

[jdemeyer]

comment:2

Put an actual link in the patch instead of "PR29".

[jhpalmieri]

Changed branch from u/fbissey/eclib-xcode9 to u/jhpalmieri/eclib-xcode9

[jhpalmieri]

New commits:

8850583

trac 23922: eclib patch from https://github.com/JohnCremona/eclib/pull/29

[jhpalmieri]

Changed commit from 235c09e to 8850583

[jdemeyer]

comment:5

I meant inside the .patch file: please put the link there.

[sagetrac-git]

Changed commit from 8850583 to 92d0489

[sagetrac-git]

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

92d0489

trac 23922: eclib patch from https://github.com/JohnCremona/eclib/pull/29

[jhpalmieri]

comment:7

Like that?

[JohnCremona]

comment:8

I just made a new eclib release v20171002 which includes the fixes here, so perhaps it would be better to replace this with that. I have not yet tried making a new spkg from it but someone else is welcome to (I will not before tomorrow)

[jdemeyer]

Changed author from François Bissey, Isuru Fernando to François Bissey, Isuru Fernando, Jeroen Demeyer

[jdemeyer]

Changed upstream from Fixed upstream, but not in a stable release. to Fixed upstream, in a later stable release.

[jdemeyer]

comment:10

John, is there a proper source tarball for this new version? Earlier versions were at http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/

[jhpalmieri]

comment:20

This builds for me with OS X + Xcode 9 + clang, but the test suite has some numerical noise:

Testing comptest...
5c5
< AGM((3.125,4.25),(1,0)) = (2.07831617217326684,1.5633615899065958201)
---
> AGM((3.125,4.25),(1,0)) = (2.07831617217326684,1.56336158990659582)
14c14
< whose cube is    (2.9999999999999999998,3.9999999999999999999)
---
> whose cube is    (2.9999999999999999999,4)
16c16
< whose cube is    (2.9999999999999999997,3.9999999999999999999)
---
> whose cube is    (2.9999999999999999996,3.9999999999999999998)
make[4]: *** [check_procs] Error 1
make[4]: *** Waiting for unfinished jobs....
Testing theight...
Testing hecketest...
Testing mhcount...
Testing thtconst...
Testing tmanin...
Testing tegr...
Testing telog...
Testing nftest...
Testing oftest...
59,61c59,61
< Periods: [w_1,w_2] = [(-0.51317082433770156405898300169308452073251780098017e-48,-0.63277902985143330237168013012161275230207585428624),(2.0131613909534741204308876218764326959565176646346,-0.31638951492571665118584006506080637615103792714312)]
< tau       = (0.49999999999999999999999999999999999999999999999742,3.1814603455271474113187811974948516017119213527377) (abs(tau)=3.220510818202869536114214880382970192586945735383)
< w_R = (4.0263227819069482408617752437528653919130353292697,0)	w_IR = (2.0131613909534741204308876218764326959565176646351,0.31638951492571665118584006506080637615103792714312)
---
> Periods: [w_1,w_2] = [(0,0.6327790298514333023716801301216127523020758542864),(-2.0131613909534741204308876218764326959565176646351,-0.3163895149257166511858400650608063761510379271432)]
> tau       = (-0.5,3.1814603455271474113187811974948516017119213527374) (abs(tau)=3.2205108182028695361142148803829701925869457353831)
> w_R = (4.0263227819069482408617752437528653919130353292703,0)	w_IR = (2.0131613909534741204308876218764326959565176646351,0.3163895149257166511858400650608063761510379271432)
65c65
< Elliptic log of P is (2.1525156669616024162853317818644956119079357950699,0)
---
> Elliptic log of P is (2.1525156669616024162853317818644956119079357950702,0)
68c68
< Elliptic log of P is (3.4017204102584996326696274231234087985780021511049,0)
---
> Elliptic log of P is (3.4017204102584996326696274231234087985780021513184,0)
make[4]: *** [check_qcurves] Error 1
Testing tnfd...
rm -rf nftmp
rm -rf snftmp
rm -rf tcurves
Testing tequiv...
Testing d2...
rm -f PRIMES 1
make[3]: *** [check] Error 2
make[2]: *** [check-recursive] Error 1
Error: eclib's test suite failed to pass.

[jhpalmieri]

comment:21

The test suite passes when building with GCC 7.2.0 (see #23898).

[jhpalmieri]

comment:22

Oh, and now I remember that numerical noise problems in the eclib test suite with clang are not new, so they should not be an obstacle here: see JohnCremona/eclib#19.

[JohnCremona]

comment:23

Yes, there is numerical noise with some Mac compilers. eclib uses TravisCI now but the Mac test cheat by passing even when it fails for this reason. See https://travis-ci.org/JohnCremona/eclib

[kiwifb]

comment:24

The numerical noise is not particular to OS X but to clang. I get it on linux+clang too.

[dimpase]

comment:25

md5 of the linked tarball does not match the one in the branch. I get

MD5 (eclib-20171002.tar.bz2) = 2c6e90c61f49cf9c38a5c9fd9a1ebcef

[JohnCremona]

comment:26

Probably my fault, indirectly: I left Jeroen to package up the tarball and when testing found that I had done something wrong so replaced the tarball with another with the same filename. Perhaps he forgot to recompute the hash.

[jdemeyer]

comment:27

No, I just haven't revisited this ticket since you fixed the tarball.

[sagetrac-git]

Changed commit from 73218af to 5c878e6

[sagetrac-git]

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

5c878e6

Upgrade eclib to version 20171002

[jdemeyer]

comment:29

Replying to @JohnCremona:

Here is the explanation. There is an optional FLINT module called hmod_mat which is a 32-bit version of nmod_mat which Fredrik wrote for me so that I can do modular linear algebra modulo a 32-bit prime without using double the memory. It would be quite nice if Sage's FLINT had that installed, but until then here's what we (or at least I) do. eclib's configure script sets a parameter called FLINT_LEVEL which should be 0 for "don't use FLINT" or 1 for "use standard FLINT". If it is set to 2 then it means "use FLINT with hmod_mat module installed". For my own used I do the latter but when creating a Sage tarball I should remember for its configure script to halve FLINT_LEVEL set to 1 not 2.

I can make a new tarball with this changed (lines 19176 and 19187 in configure).

By the way the README file has this information :)

It is very easy to check for headers in a configure script. You could check for flint/hmod_mat.h and set FLINT_LEVEL accordingly.

Can you tell me more about this special version of FLINT? Is this officially released somewhere, or is it for John Cremona's eyes only?

[JohnCremona]

comment:30

See https://github.com/fredrik-johansson/hmod_mat

Thanks for the other tip, I did once try without success but it would be good to have this automatic.It would also be good if Sage could have the hmod_mat module in its FLINT.

[dimpase]

comment:31

PR29.patch should be removed in this branch, too.

[jdemeyer]

comment:32

Fixed checksum, this now works for me on Linux x86_64.

[jdemeyer]

comment:33

Replying to @dimpase:

PR29.patch should be removed in this branch, too.

What is PR29.patch?

[dimpase]

comment:34

Replying to @jdemeyer:

Replying to @dimpase:

PR29.patch should be removed in this branch, too.

What is PR29.patch?

Oh, never mind - it's a meanwhile merged eclib PR that is included in #12426 that I am working on ATM...

[jhpalmieri]

comment:35

It's fine with me. Any objections to a positive review?

[dimpase]

comment:36

on a clang I see

[eclib-20171002] sieve_search.cc:819:23: warning: logical not is only applied to the left hand side of this bitwise operator [-Wlogical-not-parentheses]
[eclib-20171002]                 if(!use_odd_nums && !b&1)
[eclib-20171002]                                     ^ ~
[eclib-20171002] sieve_search.cc:819:23: note: add parentheses after the '!' to evaluate the bitwise operator first
[eclib-20171002]                 if(!use_odd_nums && !b&1)
[eclib-20171002]                                     ^
[eclib-20171002]                                      (  )
[eclib-20171002] sieve_search.cc:819:23: note: add parentheses around left hand side expression to silence this warning
[eclib-20171002]                 if(!use_odd_nums && !b&1)
[eclib-20171002]                                     ^
[eclib-20171002]                                     ( )

Only the author knows for sure where () should go...

[JohnCremona]

comment:37

This particular piece of code was borrowed from an old version of Stoll's ratpoints so the author is not quite who you think it is. It should probably be

if((!use_odd_nums) && !(b&1))

but it might be a good idea to look into ratpoints (already built for Sage)... around line 118 of ratpoints' sift.c. But this if() statement is no longer there.

Your next question will be: why does eclib not actually use ratpoints itself? I don't have time to answer that now.

[jdemeyer]

comment:38

Replying to @jhpalmieri:

It's fine with me. Any objections to a positive review?

Can I take this comment as positive_review?

[dimpase]

Reviewer: John Cremona, Jeroen Demeyer, John Palmieri, Dima Pasechnik

[jdemeyer]

Changed reviewer from John Cremona, Jeroen Demeyer, John Palmieri, Dima Pasechnik to John Cremona, John Palmieri, Dima Pasechnik

[vbraun]

Changed branch from u/jdemeyer/eclib-xcode9 to 5c878e6