Opened 5 years ago

Closed 5 years ago

#23922 closed defect (fixed)

Upgrade eclib to compile with xcode 9

Reported by: François Bissey Owned by:
Priority: critical Milestone: sage-8.1
Component: packages: standard Keywords:
Cc: John Palmieri, Isuru Fernando Merged in:
Authors: François Bissey, Isuru Fernando, Jeroen Demeyer Reviewers: John Cremona, John Palmieri, Dima Pasechnik
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: 5c878e6 (Commits, GitHub, GitLab) Commit: 5c878e6454fe3156fbd9c8dd3187d6a26edd6c42
Dependencies: Stopgaps:

Status badges

Change History (41)

comment:1 Changed 5 years ago by François Bissey

Authors: François Bissey, Isuru Fernando
Branch: u/fbissey/eclib-xcode9
Cc: Isuru Fernando added
Commit: 235c09e0a653aac120098c0ad45e3ac71f24babe
Status: newneeds_review

New commits:

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

comment:2 Changed 5 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

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

comment:3 Changed 5 years ago by John Palmieri

Branch: u/fbissey/eclib-xcode9u/jhpalmieri/eclib-xcode9

comment:4 Changed 5 years ago by John Palmieri

Commit: 235c09e0a653aac120098c0ad45e3ac71f24babe88505833dc0ccb96d304edf888d84862a53f0ee4
Status: needs_workneeds_review

New commits:

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

comment:5 Changed 5 years ago by Jeroen Demeyer

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

comment:6 Changed 5 years ago by git

Commit: 88505833dc0ccb96d304edf888d84862a53f0ee492d0489ec6436e2d52c633be62c45dd082801258

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

comment:7 Changed 5 years ago by John Palmieri

Like that?

comment:8 Changed 5 years ago by John Cremona

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)

comment:9 Changed 5 years ago by Jeroen Demeyer

Authors: François Bissey, Isuru FernandoFrançois Bissey, Isuru Fernando, Jeroen Demeyer
Description: modified (diff)
Report Upstream: Fixed upstream, but not in a stable release.Fixed upstream, in a later stable release.
Status: needs_reviewneeds_work
Summary: patch eclib to compile with xcode 9Upgrade eclib to compile with xcode 9

comment:10 Changed 5 years ago by Jeroen Demeyer

Description: modified (diff)
Status: needs_workneeds_info

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/

comment:11 Changed 5 years ago by Jeroen Demeyer

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/

comment:12 Changed 5 years ago by John Cremona

Description: modified (diff)

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.

comment:14 Changed 5 years ago by Jeroen Demeyer

Branch: u/jhpalmieri/eclib-xcode9u/jdemeyer/eclib-xcode9

comment:15 Changed 5 years ago by Jeroen Demeyer

Commit: 92d0489ec6436e2d52c633be62c45dd08280125873218af556810c40af6a8c4a1c5b80bfaf6631eb

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

comment:16 Changed 5 years ago by Jeroen Demeyer

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

It does not contain a file hmod_mat.h

comment:17 Changed 5 years ago by Jeroen Demeyer

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

comment:18 Changed 5 years ago by John Cremona

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

comment:19 Changed 5 years ago by John Cremona

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

comment:20 Changed 5 years ago by John Palmieri

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.

comment:21 Changed 5 years ago by John Palmieri

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

comment:22 Changed 5 years ago by John Palmieri

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.

Last edited 5 years ago by John Palmieri (previous) (diff)

comment:23 Changed 5 years ago by John Cremona

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

comment:24 Changed 5 years ago by François Bissey

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

comment:25 Changed 5 years ago by Dima Pasechnik

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

MD5 (eclib-20171002.tar.bz2) = 2c6e90c61f49cf9c38a5c9fd9a1ebcef
Last edited 5 years ago by Dima Pasechnik (previous) (diff)

comment:26 Changed 5 years ago by John Cremona

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.

comment:27 Changed 5 years ago by Jeroen Demeyer

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

comment:28 Changed 5 years ago by git

Commit: 73218af556810c40af6a8c4a1c5b80bfaf6631eb5c878e6454fe3156fbd9c8dd3187d6a26edd6c42

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

5c878e6Upgrade eclib to version 20171002

comment:29 in reply to:  18 Changed 5 years ago by Jeroen Demeyer

Replying to cremona:

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?

comment:30 Changed 5 years ago by John Cremona

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.

comment:31 Changed 5 years ago by Dima Pasechnik

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

comment:32 Changed 5 years ago by Jeroen Demeyer

Status: needs_infoneeds_review

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

comment:33 in reply to:  31 ; Changed 5 years ago by Jeroen Demeyer

Replying to dimpase:

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

What is PR29.patch?

comment:34 in reply to:  33 Changed 5 years ago by Dima Pasechnik

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

comment:35 Changed 5 years ago by John Palmieri

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

comment:36 Changed 5 years ago by Dima Pasechnik

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

comment:37 Changed 5 years ago by John Cremona

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.

comment:38 in reply to:  35 Changed 5 years ago by Jeroen Demeyer

Replying to jhpalmieri:

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

Can I take this comment as positive_review?

comment:39 Changed 5 years ago by Dima Pasechnik

Reviewers: John Cremona, Jeroen Demeyer, John Palmieri, Dima Pasechnik
Status: needs_reviewpositive_review

comment:40 Changed 5 years ago by Jeroen Demeyer

Reviewers: John Cremona, Jeroen Demeyer, John Palmieri, Dima PasechnikJohn Cremona, John Palmieri, Dima Pasechnik

comment:41 Changed 5 years ago by Volker Braun

Branch: u/jdemeyer/eclib-xcode95c878e6454fe3156fbd9c8dd3187d6a26edd6c42
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.