Opened 6 years ago

Closed 6 years ago

#20581 closed enhancement (fixed)

Upgrade PARI to latest master

Reported by: jpflori Owned by:
Priority: major Milestone: sage-7.3
Component: packages: standard Keywords: days74
Cc: jdemeyer, kedlaya, fbissey 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 jdemeyer)

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 6 years ago by jpflori

@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 6 years ago by fbissey

  • Cc fbissey added

comment:3 Changed 6 years ago by jdemeyer

  • Authors set to Jeroen Demeyer

comment:4 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 6 years ago by jdemeyer

  • Branch set to u/jdemeyer/upgrade_pari_to_latest_master

comment:6 Changed 6 years ago by jdemeyer

  • Commit set to 1f90f95e7c12901dba8078c84a588d9b3cd231a7
  • Status changed from new to needs_review

New commits:

1f90f95Upgrade PARI to latest master

comment:7 follow-up: Changed 6 years ago by jpflori

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 6 years ago by jdemeyer

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 6 years ago by jpflori

  • Reviewers set to Jean-Pierre Flori
  • Status changed from needs_review to positive_review

comment:10 Changed 6 years ago by vbraun

  • Status changed from positive_review to needs_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 6 years ago by jpflori

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 follow-up: Changed 6 years ago by jdemeyer

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 6 years ago by jpflori

Replying to jdemeyer:

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

Yes that's a mystery as well.

comment:14 Changed 6 years ago by jpflori

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

comment:15 Changed 6 years ago by vbraun

You have an account on the OSX buildbot, right?

comment:16 Changed 6 years ago by jpflori

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

comment:17 Changed 6 years ago by vbraun

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 6 years ago by jdemeyer

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 6 years ago by git

  • Commit changed from 1f90f95e7c12901dba8078c84a588d9b3cd231a7 to 7690e7cfc166d6026e8be2c09c24ee80ef73426b

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 6 years ago by git

  • Commit changed from 7690e7cfc166d6026e8be2c09c24ee80ef73426b to 3b7c89077f2319c88fc92297685e47ced319aa87

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

3b7c890Undefining ulong does not work

comment:21 follow-up: Changed 6 years ago by jpflori

Is that because of typedef?

comment:22 in reply to: ↑ 21 Changed 6 years ago by jdemeyer

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 6 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:24 Changed 6 years ago by tscrim

  • Keywords days74 added

comment:25 Changed 6 years ago by git

  • Commit changed from 3b7c89077f2319c88fc92297685e47ced319aa87 to 5a461395244933a16e9b578eeca63b586f861e91

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 jdemeyer

Rebased on latest develop.

comment:27 Changed 6 years ago by pbruin

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 jdemeyer

  • Description modified (diff)
  • Status changed from needs_review to needs_work

comment:29 Changed 6 years ago by git

  • Commit changed from 5a461395244933a16e9b578eeca63b586f861e91 to e796224eafee210bc2711d67afc9ffc59779208c

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 jdemeyer

  • Status changed from needs_work to needs_review

Upgraded to really latest master.

comment:31 Changed 6 years ago by pbruin

  • Branch changed from u/jdemeyer/upgrade_pari_to_latest_master to u/pbruin/20581-upgrade_pari_to_latest_master
  • Commit changed from e796224eafee210bc2711d67afc9ffc59779208c to bedff7be09d52fc7eba721074eaab6ad5607876b
  • Reviewers changed from Jean-Pierre Flori to Jean-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 pbruin

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 jdemeyer

  • Status changed from needs_review to positive_review

comment:34 follow-up: Changed 6 years ago by vbraun

  • Status changed from positive_review to 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 in reply to: ↑ 34 Changed 6 years ago by fbissey

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 vbraun

maybe, let me double check...

comment:37 Changed 6 years ago by vbraun

  • Status changed from needs_work to positive_review

comment:38 Changed 6 years ago by vbraun

  • Branch changed from u/pbruin/20581-upgrade_pari_to_latest_master to bedff7be09d52fc7eba721074eaab6ad5607876b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.