Opened 2 years ago
Closed 5 weeks ago
#14346 closed enhancement (fixed)
Document/report/fix PARI patches
Reported by: | Snark | Owned by: | tbd |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | packages: standard | Keywords: | |
Cc: | jdemeyer | Merged in: | |
Authors: | Reviewers: | Jeroen Demeyer | |
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 (48)
comment:1 Changed 2 years ago by leif
comment:2 Changed 2 years 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 2 years 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 2 years 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 2 years 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: ↓ 8 Changed 2 years 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 2 years ago by jdemeyer
- Cc jdemeyer added
comment:8 in reply to: ↑ 6 ; follow-up: ↓ 10 Changed 2 years 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 2 years 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.
comment:10 in reply to: ↑ 8 ; follow-up: ↓ 12 Changed 2 years 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.
comment:11 follow-ups: ↓ 13 ↓ 22 Changed 2 years 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 2 years 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 2 years 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: ↓ 15 Changed 2 years ago by Snark
Ok, let's be explicit:
- in general there are fifteen patches in this spkg, which is way too much ;
- 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 2 years 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 2 years ago by jdemeyer
- Description modified (diff)
comment:17 follow-ups: ↓ 18 ↓ 29 Changed 2 years 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 2 years 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 2 years 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 2 years ago by Snark
- Description modified (diff)
comment:21 Changed 2 years ago by jdemeyer
- Description modified (diff)
comment:22 in reply to: ↑ 11 Changed 2 years 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 2 years 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 2 years ago by jdemeyer
- Description modified (diff)
comment:25 Changed 2 years ago by jdemeyer
- Description modified (diff)
comment:26 Changed 2 years 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 2 years 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 2 years ago by jdemeyer
- Description modified (diff)
comment:29 in reply to: ↑ 17 Changed 2 years 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 2 years 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 2 years ago by jdemeyer
- Description modified (diff)
comment:32 Changed 2 years ago by jdemeyer
- Summary changed from The pari spkg is patching upstream too heavily to Document/report/fix PARI patches
comment:33 Changed 2 years ago by jdemeyer
- Description modified (diff)
comment:34 Changed 2 years 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 2 years ago by jdemeyer
- Dependencies set to #14539
- Description modified (diff)
comment:36 follow-up: ↓ 37 Changed 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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) :
- it compiled ok ;
- eclib compiled ;
- genus2reduction didn't compile (error: ‘arither1’ undeclared) ;
- lcalc didn't compile (error: too few arguments to function ‘long int* ellinit(GEN, GEN, long int)');
- sagelib didn't compile (quite a few different problems).
comment:43 Changed 12 months ago by Snark
I think #15767 will fix that bug too.
comment:44 Changed 5 weeks ago by jdemeyer
What should be done with this ticket now? Do you still consider it relevant?
comment:45 Changed 5 weeks ago by Snark
Well, things changed a lot : I vote for close.
comment:46 Changed 5 weeks ago by jdemeyer
- Milestone changed from sage-wishlist to sage-duplicate/invalid/wontfix
- Reviewers set to Jeroen Demeyer
- Status changed from new to needs_review
comment:47 Changed 5 weeks ago by jdemeyer
- Status changed from needs_review to positive_review
comment:48 Changed 5 weeks ago by vbraun
- Resolution set to fixed
- Status changed from positive_review to closed
Does this apply to the PARI library, or gp?
(Of course gp uses the PARI library, but I think^{TM} the stand-alone gp wouldn't need patching for Sage.)