search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.38k stars 469 forks source link

Upgrade eclib to compile with xcode 9 #23922

Closed kiwifb closed 7 years ago

kiwifb commented 7 years ago

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 commented 7 years ago

Author: François Bissey, Isuru Fernando

kiwifb commented 7 years ago

Commit: 235c09e

kiwifb commented 7 years ago

Branch: u/fbissey/eclib-xcode9

kiwifb commented 7 years ago

New commits:

235c09eIncluding upstream PR 29 to compile with xcode 9 until we have a new release.
jdemeyer commented 7 years ago
comment:2

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

jhpalmieri commented 7 years ago

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

jhpalmieri commented 7 years ago

New commits:

8850583trac 23922: eclib patch from https://github.com/JohnCremona/eclib/pull/29
jhpalmieri commented 7 years ago

Changed commit from 235c09e to 8850583

jdemeyer commented 7 years ago
comment:5

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 8850583 to 92d0489

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

92d0489trac 23922: eclib patch from https://github.com/JohnCremona/eclib/pull/29
jhpalmieri commented 7 years ago
comment:7

Like that?

JohnCremona commented 7 years ago
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 commented 7 years ago

Description changed:

--- 
+++ 
@@ -1,2 +1 @@
-The need for the patch will disappear with the next release of eclib. We fix 
- https://github.com/JohnCremona/eclib/issues/28 with https://github.com/JohnCremona/eclib/pull/29 already merged upstream.
+**Tarball**: https://github.com/JohnCremona/eclib/archive/v20171002.tar.gz
jdemeyer commented 7 years ago

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

jdemeyer commented 7 years ago

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

jdemeyer commented 7 years ago

Description changed:

--- 
+++ 
@@ -1 +1 @@
-**Tarball**: https://github.com/JohnCremona/eclib/archive/v20171002.tar.gz
+**Tarball**: https://github.com/JohnCremona/eclib/archive/v20171002.tar.gz (???)
jdemeyer commented 7 years ago
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/

jdemeyer commented 7 years ago
comment:11

John: from reading http://homepages.warwick.ac.uk/staff/J.E.Cremona/mwrank/index.html it seems that you are confusing a git repo with a source tarball. The latter is usually created by the former by running a command like make dist. It differs from a git repo by including also some auto-generated files (for example, configure in your case).

Note that it is possible to use Travis CI to automatically create proper releases, see https://docs.travis-ci.com/user/deployment/releases/

JohnCremona commented 7 years ago

Description changed:

--- 
+++ 
@@ -1 +1 @@
-**Tarball**: https://github.com/JohnCremona/eclib/archive/v20171002.tar.gz (???)
+**Tarball**: http://homepages.warwick.ac.uk/staff/J.E.Cremona/ftp/eclib-20171002.tar.bz2
JohnCremona commented 7 years ago
comment:12

THanks for the comments. I use github to make "proper releases". The tarball which I now realise that I forgot to put in the usual place is made using "make dist" (actually "make dist-bzip2") . It is there now and I updated the link.

jdemeyer commented 7 years ago
comment:13

Remember to update also http://homepages.warwick.ac.uk/staff/J.E.Cremona/mwrank/index.html

jdemeyer commented 7 years ago

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

jdemeyer commented 7 years ago
comment:15

Unfortunately, it doesn't build:

[eclib-20171002] mat.cc:1485:28: fatal error: flint/hmod_mat.h: No such file or directory
[eclib-20171002]  #include "flint/hmod_mat.h"

Does this require a more recent version of FLINT?

New commits:

73218afUpgrade eclib to version 20171002
jdemeyer commented 7 years ago

Changed commit from 92d0489 to 73218af

jdemeyer commented 7 years ago
comment:16

The latest release of FLINT is 2.5.2, released 2015-08-07

It does not contain a file hmod_mat.h

jdemeyer commented 7 years ago
comment:17

In fact, I couldn't find any version of FLINT containing a file hmod_mat.h (I tried some older versions and git master).

JohnCremona commented 7 years ago
comment:18

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

JohnCremona commented 7 years ago
comment:19

I have changed the tarball to have the standard setting in configure. Could you try biulding again please?

jhpalmieri commented 7 years ago
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 commented 7 years ago
comment:21

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

jhpalmieri commented 7 years ago
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 https://github.com/JohnCremona/eclib/issues/19.

JohnCremona commented 7 years ago
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 commented 7 years ago
comment:24

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

dimpase commented 7 years ago
comment:25

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

MD5 (eclib-20171002.tar.bz2) = 2c6e90c61f49cf9c38a5c9fd9a1ebcef
JohnCremona commented 7 years ago
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 commented 7 years ago
comment:27

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 73218af to 5c878e6

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

5c878e6Upgrade eclib to version 20171002
jdemeyer commented 7 years ago
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 commented 7 years ago
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 commented 7 years ago
comment:31

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

jdemeyer commented 7 years ago
comment:32

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

jdemeyer commented 7 years ago
comment:33

Replying to @dimpase:

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

What is PR29.patch?

dimpase commented 7 years ago
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 commented 7 years ago
comment:35

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

dimpase commented 7 years ago
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 commented 7 years ago
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 commented 7 years ago
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 commented 7 years ago

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