Opened 7 years ago

Closed 6 years ago

#20581 closed enhancement (fixed)

Upgrade PARI to latest master

Reported by: Jean-Pierre Flori Owned by:
Priority: major Milestone: sage-7.3
Component: packages: standard Keywords: days74
Cc: Jeroen Demeyer, Kiran Kedlaya, François Bissey Merged in:
Authors: Jeroen Demeyer Reviewers: Jean-Pierre Flori, Peter Bruin
Report Upstream: N/A Work issues:
Branch: bedff7b (Commits, GitHub, GitLab) Commit: bedff7be09d52fc7eba721074eaab6ad5607876b
Dependencies: Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

Some recent fixes needed to correctly count points on hyperelliptic curves.

Tarball: http://sage.ugent.be/www/jdemeyer/sage/pari-2.8-2771-gb70b447.tar.gz

Change History (38)

comment:1 Changed 7 years ago by Jean-Pierre Flori

@Jeroen: you already did the upgrade a bunch of time IIRC. I could try to review the ticket (which should be trivial) while I have some time this week. (Later than that I can not promise anything.)

comment:2 Changed 7 years ago by François Bissey

Cc: François Bissey added

comment:3 Changed 7 years ago by Jeroen Demeyer

Authors: Jeroen Demeyer

comment:4 Changed 7 years ago by Jeroen Demeyer

Description: modified (diff)

comment:5 Changed 7 years ago by Jeroen Demeyer

Branch: u/jdemeyer/upgrade_pari_to_latest_master

comment:6 Changed 7 years ago by Jeroen Demeyer

Commit: 1f90f95e7c12901dba8078c84a588d9b3cd231a7
Status: newneeds_review

New commits:

1f90f95Upgrade PARI to latest master

comment:7 Changed 7 years ago by Jean-Pierre Flori

I see a lot of functions' signatures changed because of the addition of the const modifier. Was it on purpose? (IIRC Cython does not care about constness anyway.)

comment:8 in reply to:  7 Changed 7 years ago by Jeroen Demeyer

Replying to jpflori:

Was it on purpose? (IIRC Cython does not care about constness anyway.)

Well, I just took the declarations from PARI and they have const in them. And it's true that it probably does not matter for Cython anyway.

comment:9 Changed 7 years ago by Jean-Pierre Flori

Reviewers: Jean-Pierre Flori
Status: needs_reviewpositive_review

comment:10 Changed 7 years ago by Volker Braun

Status: positive_reviewneeds_work

I'm getting failures on OSX

In file included from /Users/buildslave-sage/slave/sage_git/build/src/build/cythonized/sage/libs/pari/handle_error.c:324:0:
/Users/buildslave-sage/slave/sage_git/build/local/include/pari/paripriv.h:16:48: error: unknown type name 'ulong'
 hashtable *hashstr_import_static(hashentry *e, ulong size);
                                                ^
In file included from /Users/buildslave-sage/slave/sage_git/build/local/include/pari/pari.h:39:0,
                 from /Users/buildslave-sage/slave/sage_git/build/src/sage/libs/pari/parisage.h:3,
                 from /Users/buildslave-sage/slave/sage_git/build/src/build/cythonized/sage/libs/pari/handle_error.c:320:
/Users/buildslave-sage/slave/sage_git/build/local/include/pari/paripriv.h: In function 'clone_lock':
/Users/buildslave-sage/slave/sage_git/build/local/include/pari/paripriv.h:36:25: error: 'ulong' undeclared (first use in this function)
 clone_lock(GEN C) { if (isclone(C)) ++bl_refc(C); }

comment:11 Changed 7 years ago by Jean-Pierre Flori

FLINT and PARI ulong mess... I guess that handle_error.pxd pulls the PARI headers and parisage.h which undefs ulong and when handle_error.pyx is treated and pulls in paripriv.h which wants ulong we lose... Not sure what the cleanest solution would be. Maybe #define pari_ulong in parisage.h and define/undefine ulong in a custom pariprivsage.h which pulls paripriv.h in (just as parisage.h does for pari.h).

comment:12 Changed 7 years ago by Jeroen Demeyer

I don't understand why anything would change on OS X...

I do plan to investigate this, but maybe not this week.

comment:13 in reply to:  12 Changed 7 years ago by Jean-Pierre Flori

Replying to jdemeyer:

I don't understand why anything would change on OS X...

Yes that's a mystery as well.

comment:14 Changed 7 years ago by Jean-Pierre Flori

@Volker: can you post the generated C file (handle_error.c)?

comment:15 Changed 7 years ago by Volker Braun

You have an account on the OSX buildbot, right?

comment:16 Changed 7 years ago by Jean-Pierre Flori

Cannot remember if I have one, nor it's name...

comment:17 Changed 7 years ago by Volker Braun

Ok apparently not... if you send me privately your desired account name and Apple ID (for login) then I can make one.

comment:18 Changed 7 years ago by Jeroen Demeyer

This works by accident on Linux due to this bit in /usr/include/sys/types.h:

#ifdef __USE_MISC
/* Old compatibility names for C types.  */
typedef unsigned long int ulong;
typedef unsigned short int ushort;
typedef unsigned int uint;
#endif

comment:19 Changed 7 years ago by git

Commit: 1f90f95e7c12901dba8078c84a588d9b3cd231a77690e7cfc166d6026e8be2c09c24ee80ef73426b

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

b1a2888Upgrade PARI to latest master
7690e7cSafely include paripriv.h

comment:20 Changed 7 years ago by git

Commit: 7690e7cfc166d6026e8be2c09c24ee80ef73426b3b7c89077f2319c88fc92297685e47ced319aa87

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

3b7c890Undefining ulong does not work

comment:21 Changed 7 years ago by Jean-Pierre Flori

Is that because of typedef?

comment:22 in reply to:  21 Changed 7 years ago by Jeroen Demeyer

Replying to jpflori:

Is that because of typedef?

No, the problem is that ulong appears also indirectly in macros. For example, the typ() macro in PARI uses ulong.

comment:23 Changed 7 years ago by Jeroen Demeyer

Status: needs_workneeds_review

comment:24 Changed 7 years ago by Travis Scrimshaw

Keywords: days74 added

comment:25 Changed 6 years ago by git

Commit: 3b7c89077f2319c88fc92297685e47ced319aa875a461395244933a16e9b578eeca63b586f861e91

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

e9a3a3eUpgrade PARI to latest master
5a46139Undefining ulong does not work

comment:26 Changed 6 years ago by Jeroen Demeyer

Rebased on latest develop.

comment:27 Changed 6 years ago by Peter Bruin

Note in case you intend to update this ticket to the very latest PARI version: if you do so, you can remove build/pkgs/pari/patches/do_QXQ_eval.patch (added in #20749, corresponding to upstream commit 6db8fba).

comment:28 Changed 6 years ago by Jeroen Demeyer

Description: modified (diff)
Status: needs_reviewneeds_work

comment:29 Changed 6 years ago by git

Commit: 5a461395244933a16e9b578eeca63b586f861e91e796224eafee210bc2711d67afc9ffc59779208c

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

e796224Upgrade PARI to latest master

comment:30 Changed 6 years ago by Jeroen Demeyer

Status: needs_workneeds_review

Upgraded to really latest master.

comment:31 Changed 6 years ago by Peter Bruin

Branch: u/jdemeyer/upgrade_pari_to_latest_masteru/pbruin/20581-upgrade_pari_to_latest_master
Commit: e796224eafee210bc2711d67afc9ffc59779208cbedff7be09d52fc7eba721074eaab6ad5607876b
Reviewers: Jean-Pierre FloriJean-Pierre Flori, Peter Bruin

Two small patches; the second one mirrors the changes to paridecl.h made in upstream commit 0d28865 (split FpXQX_factor.c from FpX_factor.c).

comment:32 Changed 6 years ago by Peter Bruin

Looks good to me, and doctests pass on GNU/Linux x86_64. If you are happy with the above patches and the ulong problem on OSX has been resolved, this can get a positive review as far as I'm concerned.

comment:33 Changed 6 years ago by Jeroen Demeyer

Status: needs_reviewpositive_review

comment:34 Changed 6 years ago by Volker Braun

Status: positive_reviewneeds_work
sage -t --long src/sage/modular/dirichlet.py  # 1 doctest failed
sage -t --long src/sage/modular/overconvergent/weightspace.py  # 2 doctests failed

comment:35 in reply to:  34 Changed 6 years ago by François Bissey

Replying to vbraun:

sage -t --long src/sage/modular/dirichlet.py  # 1 doctest failed
sage -t --long src/sage/modular/overconvergent/weightspace.py  # 2 doctests failed

I honestly thought those were caused by #15692 rather than this one.

comment:36 Changed 6 years ago by Volker Braun

maybe, let me double check...

comment:37 Changed 6 years ago by Volker Braun

Status: needs_workpositive_review

comment:38 Changed 6 years ago by Volker Braun

Branch: u/pbruin/20581-upgrade_pari_to_latest_masterbedff7be09d52fc7eba721074eaab6ad5607876b
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.