Opened 19 months ago

Last modified 7 months ago

#14346 new enhancement

Document/report/fix PARI patches

Reported by: Snark Owned by: tbd
Priority: major Milestone: sage-wishlist
Component: packages: standard Keywords:
Cc: jdemeyer Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14539 Stopgaps:

Description (last modified by jdemeyer)

This spkg has many patches, with the following:

Good:

  • get_ld.patch: author known, pure packaging
  • get_tests.patch: author known, pure packaging
  • install_doc_no_make.patch: author known, pure packaging
  • perl_path.patch: author known, pure packaging
  • cygwin_dll_a.patch: author known, pure packaging
  • GCC_PR49330.patch: author mentioned, discussed with upstream, upstream decided not to fix this
  • get_dlcflags.patch: author known, partially accepted by upstream
  • KERNELCFLAGS.patch: author known, forwarded upstream
  • trac_13902_determinant.patch: author mentioned, upstream backport
  • polred.patch: author mentioned, upstream backport

Bad:

  • get_config_options.patch: author known, upstream-worthy, no comment says it was forwarded, which probably means it wasn't
  • get_fltk.patch: author known, upstream-worthy, no comment says it was forwarded, which probably means it wasn't
  • get_X11.patch: author known, upstream-worthy, no comment says it was forwarded, which probably means it wasn't
  • mp.c.patch: author unknown, should be replaced by a Sage library patch.

Change History (43)

comment:1 Changed 19 months ago by leif

Does this apply to the PARI library, or gp?

(Of course gp uses the PARI library, but I thinkTM the stand-alone gp wouldn't need patching for Sage.)

comment:2 Changed 19 months ago by Snark

Both the library and the executable come out of the same spkg anyway so I don't see how the distinction matters...

comment:3 Changed 19 months ago by fbissey

Have to second that discusion on debian. I have actually pushed almost every single sage patch to pari in sage-on-gentoo. I think it is ok when they are patches included upstream. I hadn't looked at the particulars of polred. Which yes alter the ABI somewhat. Was the change necessary and will it be picked up upstream?

comment:4 Changed 19 months ago by Snark

If you read B.A.'s comment on my debian bug report, I think it's 100% sure they won't take it in the stable branch, as it breaks ABI. Now for another branch, the question is open -- but since this polred.patch is based on upstream commits, they might have better up their sleeve already!

comment:5 Changed 19 months ago by jpflori

IIRC PARI 2.6 release sould happen quite soon, so hopefully the commits we used are from this devel branch, see http://pari.math.u-bordeaux1.fr/timeline.html

comment:6 follow-up: Changed 19 months ago by Snark

The commits used were from upstream, but the patch is based on those commits -- it's not just them. That means that the pari in sage is neither compatible with upstream's 2.5.3, nor with the upcoming 2.6.0.

So yes, we can hope that when sage will package pari 2.6.0, then it will stop forking pari and become compatible with something debian can package seriously.

comment:7 Changed 19 months ago by jdemeyer

  • Cc jdemeyer added

comment:8 in reply to: ↑ 6 ; follow-up: Changed 19 months ago by jdemeyer

Replying to Snark:

The commits used were from upstream, but the patch is based on those commits -- it's not just them. That means that the pari in sage is neither compatible with upstream's 2.5.3, nor with the upcoming 2.6.0.

I thought backporting patches from later upstream versions is standard practice.

comment:9 in reply to: ↑ description Changed 19 months ago by jdemeyer

  • Milestone changed from sage-5.9 to sage-wishlist
  • Type changed from defect to enhancement

Replying to Snark:

The pari package is full of patches

So what? Since when is patching a package a bug?

some of which should have been proposed upstream

Which patches are you refering to?

the correct solution would be for sage to *add* what is needed to the sage library instead of patching upstream.

Please elaborate how we should add PARI patches to the Sage library.

Last edited 19 months ago by jdemeyer (previous) (diff)

comment:10 in reply to: ↑ 8 ; follow-up: Changed 19 months ago by fbissey

Replying to jdemeyer:

Replying to Snark:

The commits used were from upstream, but the patch is based on those commits -- it's not just them. That means that the pari in sage is neither compatible with upstream's 2.5.3, nor with the upcoming 2.6.0.

I thought backporting patches from later upstream versions is standard practice.

I'll have a shot at that one but it's probably nothing new to you. It depends on the trade off. Backporting in a distro like debian that is keeping fixed version of the software has much as possible is good.... so long as you preserve API. If the patch breaks API that means potentially all dependencies will need patching for the new API. So the trade off is between the number of dependencies you have to patch for the new API and how badly you want/need the backport.

It applies also to sage but the scale is a priori less than in a full distro. In the case of pari in particular, I don't think the list of dependencies (especially dependencies not in sage) affected by your change is very long (make that non existant until proven otherwise).

Debian may have enshrined the above discussion in a precise policy but I am not a deban-er.

Last edited 19 months ago by fbissey (previous) (diff)

comment:11 follow-ups: Changed 19 months ago by Snark

Backporting is standard practice, to gain a bugfix or a new feature which is only available in a later version.

The current situation is different : some modifications are backports, and some are new code.

The backported patches have their places in the pari spkg, but the new code should be in the sage library.

comment:12 in reply to: ↑ 10 Changed 19 months ago by jdemeyer

Replying to fbissey:

If the patch breaks API

In which sense does the polred patch "break API" (I probably don't understand what you mean exactly with "breaks API")

comment:13 in reply to: ↑ 11 Changed 19 months ago by jdemeyer

Replying to Snark:

the new code should be in the sage library.

Which code are you refering too?

Details please, this is all so vague...

comment:14 follow-up: Changed 19 months ago by Snark

Ok, let's be explicit:

  1. in general there are fifteen patches in this spkg, which is way too much ;
  2. in particular polred.patch is a mix between a backport and a new development.

I claim putting new developments in a package is against standard packaging practice.

And I use the verb "break" because that's what will happen when sage will want to include the next pari version...

comment:15 in reply to: ↑ 14 Changed 19 months ago by jdemeyer

  • Description modified (diff)

Replying to Snark:

Ok, let's be explicit:

That's not explicit at all, that's essentially just repeating what you already said. To make it easier for you, I changed the description and let you fill in the blanks.

comment:16 Changed 19 months ago by jdemeyer

  • Description modified (diff)

comment:17 follow-ups: Changed 19 months ago by fbissey

So I did a bit more digging because it is a case of show me the code, I will say that after looking at it carefully I have reached a pragmatic view on this and I don't actually mind the change in question. If Julien is worried about some other code he can put it on display. So in the polred patch

diff -ruN src/src/headers/paridecl.h b/src/headers/paridecl.h
--- src/src/headers/paridecl.h	2012-09-25 23:10:47.000000000 +0200
+++ b/src/headers/paridecl.h	2013-01-31 14:49:05.557525771 +0100
@@ -889,6 +889,7 @@
 GEN     polredabs0(GEN x, long flag);
 GEN     polredabs2(GEN x);
 GEN     polredabsall(GEN x, long flun);
+GEN     polredbest(GEN x, long flag);
 GEN     smallpolred(GEN x);
 GEN     smallpolred2(GEN x);
 GEN     tschirnhaus(GEN x);

That's technically a change in API you are exposing more functions than in the original. That kind of change is unlikely to break anything. All the rest of the code apart from the new function is private and shouldn't be called directly, so _it_ doesn't matter.
A more disturbing change would have been changing the way a public function is called. If someone has been using a private function in their code and it breaks, honestly they get what they deserve.

I haven't looked upstream, Julien may be worried that this extra function is not upstream or will be upstream under another name or call structure. It is the only thing that I can think could be extracted and put separately inside the sage library. And it technically could be done. BUT I haven't digged the code deep to see if it was possible without calling private functions from pari. I think I said something before about calling private functions...

There used to be some very invasive change in pari 2.3.xx where the sage spkg added some error handling mechanism. I cannot see it anymore. In fact looking at the patchset more closely than I did for some time, it is much cleaner than it used to be.

comment:18 in reply to: ↑ 17 Changed 19 months ago by jdemeyer

Replying to fbissey:

That's technically a change in API you are exposing more functions than in the original.

As I tried to tell you several times, this patch is a backport from upstream PARI-2.6, the added function comes from PARI-2.6.

In fact looking at the patchset more closely than I did for some time, it is much cleaner than it used to be.

So we can close this ticket as invalid ;-)

comment:19 Changed 19 months ago by fbissey

I played weather van a bit on that one. I would like to see if Julien has anything more to say. If he doesn't I'll happilly review it as invalid for you

comment:20 Changed 19 months ago by Snark

  • Description modified (diff)

comment:21 Changed 19 months ago by jdemeyer

  • Description modified (diff)

comment:22 in reply to: ↑ 11 Changed 19 months ago by jdemeyer

  • Description modified (diff)

Replying to Snark:

Backporting is standard practice, to gain a bugfix or a new feature which is only available in a later version.

By this criterion, moving polred.patch to GOOD.

comment:23 Changed 19 months ago by jpflori

FYI, the cygwin_dll_a.patch is upstream, see http://trac.sagemath.org/sage_trac/ticket/13333, corresponding upstream commits are mentioned there.

comment:24 Changed 19 months ago by jdemeyer

  • Description modified (diff)

comment:25 Changed 19 months ago by jdemeyer

  • Description modified (diff)

comment:26 Changed 19 months ago by jdemeyer

  • Description modified (diff)

Honest question: why is "patch submitted upstream but upstream ignored it" so much better than "patch not submitted upstream"? Because the end result is the same.

comment:27 Changed 19 months ago by fbissey

Scanning the gcc bugzilla entry associated with GCC_PR49330.patch it seems like gcc dev think people are just doing stuff they shouldn't with pointer arythmetics. i.e. the source code should be fixed - not that i completely understand the discussion mind you. And I learned in that entry that you can pessimize!

comment:28 Changed 19 months ago by jdemeyer

  • Description modified (diff)

comment:29 in reply to: ↑ 17 Changed 19 months ago by jpflori

Replying to fbissey:

So I did a bit more digging because it is a case of show me the code, I will say that after looking at it carefully I have reached a pragmatic view on this and I don't actually mind the change in question. If Julien is worried about some other code he can put it on display. So in the polred patch

diff -ruN src/src/headers/paridecl.h b/src/headers/paridecl.h
--- src/src/headers/paridecl.h	2012-09-25 23:10:47.000000000 +0200
+++ b/src/headers/paridecl.h	2013-01-31 14:49:05.557525771 +0100
@@ -889,6 +889,7 @@
 GEN     polredabs0(GEN x, long flag);
 GEN     polredabs2(GEN x);
 GEN     polredabsall(GEN x, long flun);
+GEN     polredbest(GEN x, long flag);
 GEN     smallpolred(GEN x);
 GEN     smallpolred2(GEN x);
 GEN     tschirnhaus(GEN x);

That's technically a change in API you are exposing more functions than in the original. That kind of change is unlikely to break anything. All the rest of the code apart from the new function is private and shouldn't be called directly, so _it_ doesn't matter.
A more disturbing change would have been changing the way a public function is called. If someone has been using a private function in their code and it breaks, honestly they get what they deserve.

I haven't looked upstream, Julien may be worried that this extra function is not upstream or will be upstream under another name or call structure.

I just checked and in PARI's git at least, it's just the same:

but I cannot tell you what will be in PARI 2.6.

It is the only thing that I can think could be extracted and put separately inside the sage library. And it technically could be done. BUT I haven't digged the code deep to see if it was possible without calling private functions from pari. I think I said something before about calling private functions...

There used to be some very invasive change in pari 2.3.xx where the sage spkg added some error handling mechanism. I cannot see it anymore. In fact looking at the patchset more closely than I did for some time, it is much cleaner than it used to be.

comment:30 Changed 19 months ago by Snark

I checked too, and indeed polredbest looks like it's upstream, so I confirm that I was wrong and that polred.patch isn't that bad.

Let me notice that patches without a properly identified author could probably go in the "Needs documentation" list.

comment:31 Changed 19 months ago by jdemeyer

  • Description modified (diff)

comment:32 Changed 19 months ago by jdemeyer

  • Summary changed from The pari spkg is patching upstream too heavily to Document/report/fix PARI patches

comment:33 Changed 19 months ago by jdemeyer

  • Description modified (diff)

comment:34 Changed 18 months ago by jdemeyer

  • Description modified (diff)

In #14539, I updated the patches README. Please verify that all needed documentation has been added.

I remove the mention of galoisanalysis_p4.patch since that patch is upstream in PARI-2.5.4 and therefore not needed anymore.

comment:35 Changed 18 months ago by jdemeyer

  • Dependencies set to #14539
  • Description modified (diff)

comment:36 follow-up: Changed 18 months ago by Snark

The documentation is much better, but it looks like sage-the-distribution is going to ship with a 'polredbest' function which will not be in pari 2.5.4 ; it will hence not be compatible with what other distributions will ship. Can't this function's code go into sage's codebase instead?

comment:37 in reply to: ↑ 36 Changed 18 months ago by jdemeyer

Replying to Snark:

Can't this function's code go into sage's codebase instead?

Why should it go into Sage's codebase? Sage is probably always going to need PARI + extra patches, not just vanilla PARI. So what's the point of moving this one patch into Sage?

comment:38 Changed 18 months ago by jpflori

I guess the point is to ease integration of Sage into Debian (or other distributions).
It may be against Debian rules to change the ABI, I don't really know.

comment:39 Changed 18 months ago by jdemeyer

Well, I don't agree that we should add extra complications to Sage to please distributions. And moving pieces of PARI into the Sage library sounds like a non-trivial thing to do.

comment:40 Changed 18 months ago by jpflori

Yup, I agree with that, I was just trying to get Julien's point.

Anyway, for Debian, I guess he'll just have to wait for PARI 2.6 which is supposedly not that far away (unless things changed since the workshop).

comment:41 Changed 18 months ago by fbissey

I don't think this particular code should go in sage. As Jeroen said it wouldn't be trivial. It looks to like pari internal functions are called. If you could implement it only with public functions it would be fine to just stick it in sage - and that would be the way to go in my opinion. But I don't see that as being the case with polredbest.

Gentoo policy on that case is it's ok if upstream intends to include the code which is the case here I believe. But really I have more freedom than Julien here.

comment:42 Changed 18 months ago by Snark

It is again a problem of confusion between sage-the-distribution and sage-the-software ; the current sage-the-software won't work with pari 2.5.4, because it depends on polredbest (at least).

This is a problem for debian ; see the discussion about my
debian bug report.

I'm not sure pari 2.6 will be a solution real soon: I just tried to make a pari 2.6 spkg from an upstream snapshot (taking the current spkg and removing the patches) :

  1. it compiled ok ;
  2. eclib compiled ;
  3. genus2reduction didn't compile (error: ‘arither1’ undeclared) ;
  4. lcalc didn't compile (error: too few arguments to function ‘long int* ellinit(GEN, GEN, long int)');
  5. sagelib didn't compile (quite a few different problems).

comment:43 Changed 7 months ago by Snark

I think #15767 will fix that bug too.

Note: See TracTickets for help on using tickets.