Opened 7 years ago
Closed 6 years ago
#20581 closed enhancement (fixed)
Upgrade PARI to latest master
Reported by:  JeanPierre Flori  Owned by:  

Priority:  major  Milestone:  sage7.3 
Component:  packages: standard  Keywords:  days74 
Cc:  Jeroen Demeyer, Kiran Kedlaya, François Bissey  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  JeanPierre Flori, Peter Bruin 
Report Upstream:  N/A  Work issues:  
Branch:  bedff7b (Commits, GitHub, GitLab)  Commit:  bedff7be09d52fc7eba721074eaab6ad5607876b 
Dependencies:  Stopgaps: 
Description (last modified by )
Some recent fixes needed to correctly count points on hyperelliptic curves.
Tarball: http://sage.ugent.be/www/jdemeyer/sage/pari2.82771gb70b447.tar.gz
Change History (38)
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
Cc:  François Bissey added 

comment:3 Changed 7 years ago by
Authors:  → Jeroen Demeyer 

comment:4 Changed 7 years ago by
Description:  modified (diff) 

comment:5 Changed 7 years ago by
Branch:  → u/jdemeyer/upgrade_pari_to_latest_master 

comment:6 Changed 7 years ago by
Commit:  → 1f90f95e7c12901dba8078c84a588d9b3cd231a7 

Status:  new → needs_review 
New commits:
1f90f95  Upgrade PARI to latest master

comment:7 followup: 8 Changed 7 years ago by
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 Changed 7 years ago by
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
Reviewers:  → JeanPierre Flori 

Status:  needs_review → positive_review 
comment:10 Changed 7 years ago by
Status:  positive_review → needs_work 

I'm getting failures on OSX
In file included from /Users/buildslavesage/slave/sage_git/build/src/build/cythonized/sage/libs/pari/handle_error.c:324:0: /Users/buildslavesage/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/buildslavesage/slave/sage_git/build/local/include/pari/pari.h:39:0, from /Users/buildslavesage/slave/sage_git/build/src/sage/libs/pari/parisage.h:3, from /Users/buildslavesage/slave/sage_git/build/src/build/cythonized/sage/libs/pari/handle_error.c:320: /Users/buildslavesage/slave/sage_git/build/local/include/pari/paripriv.h: In function 'clone_lock': /Users/buildslavesage/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
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 followup: 13 Changed 7 years ago by
I don't understand why anything would change on OS X...
I do plan to investigate this, but maybe not this week.
comment:13 Changed 7 years ago by
Replying to jdemeyer:
I don't understand why anything would change on OS X...
Yes that's a mystery as well.
comment:17 Changed 7 years ago by
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
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
Commit:  1f90f95e7c12901dba8078c84a588d9b3cd231a7 → 7690e7cfc166d6026e8be2c09c24ee80ef73426b 

comment:20 Changed 7 years ago by
Commit:  7690e7cfc166d6026e8be2c09c24ee80ef73426b → 3b7c89077f2319c88fc92297685e47ced319aa87 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
3b7c890  Undefining ulong does not work

comment:22 Changed 7 years ago by
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
Status:  needs_work → needs_review 

comment:24 Changed 7 years ago by
Keywords:  days74 added 

comment:25 Changed 6 years ago by
Commit:  3b7c89077f2319c88fc92297685e47ced319aa87 → 5a461395244933a16e9b578eeca63b586f861e91 

comment:27 Changed 6 years ago by
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
Description:  modified (diff) 

Status:  needs_review → needs_work 
comment:29 Changed 6 years ago by
Commit:  5a461395244933a16e9b578eeca63b586f861e91 → e796224eafee210bc2711d67afc9ffc59779208c 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e796224  Upgrade PARI to latest master

comment:30 Changed 6 years ago by
Status:  needs_work → needs_review 

Upgraded to really latest master.
comment:31 Changed 6 years ago by
Branch:  u/jdemeyer/upgrade_pari_to_latest_master → u/pbruin/20581upgrade_pari_to_latest_master 

Commit:  e796224eafee210bc2711d67afc9ffc59779208c → bedff7be09d52fc7eba721074eaab6ad5607876b 
Reviewers:  JeanPierre Flori → JeanPierre 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
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
Status:  needs_review → positive_review 

comment:34 followup: 35 Changed 6 years ago by
Status:  positive_review → needs_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 Changed 6 years ago by
comment:37 Changed 6 years ago by
Status:  needs_work → positive_review 

comment:38 Changed 6 years ago by
Branch:  u/pbruin/20581upgrade_pari_to_latest_master → bedff7be09d52fc7eba721074eaab6ad5607876b 

Resolution:  → fixed 
Status:  positive_review → closed 
@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.)