Opened 8 years ago
Last modified 6 years ago
#14346 closed enhancement
The pari spkg is patching upstream too heavily — at Version 25
Reported by: | Snark | Owned by: | tbd |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | packages: standard | Keywords: | |
Cc: | jdemeyer | Merged in: | |
Authors: | Reviewers: | ||
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
This spkg has 15 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
- trac_13902_determinant.patch: author unknown, upstream backport, hopefully not too modified
- polred.patch: author unknown, upstream backport
Half-good:
- KERNELCFLAGS.patch: author unknown, you have to look at a bug report to see who the author is and that it was forwarded upstream
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
- src/kernel/gmp/mp.c: author unknown, should be replaced by a Sage library patch.
Needs documentation:
- galoisanalysis_p4.patch: author unknown, forwarded and accepted upstream.
- GCC_PR49330.patch: author unknown, discussed by upstream, upstream decided not to fix this.
- get_dlcflags.patch: author known, partially accepted by upstream.
Change History (25)
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
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 8 years ago by
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 8 years ago by
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 8 years ago by
- Cc jdemeyer added
comment:8 in reply to: ↑ 6 ; follow-up: ↓ 10 Changed 8 years ago by
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 8 years ago by
- 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 8 years ago by
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 8 years ago by
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 8 years ago by
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 8 years ago by
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 8 years ago by
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 8 years ago by
- 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 8 years ago by
- Description modified (diff)
comment:17 follow-up: ↓ 18 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
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 8 years ago by
- Description modified (diff)
comment:21 Changed 8 years ago by
- Description modified (diff)
comment:22 in reply to: ↑ 11 Changed 8 years ago by
- 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 8 years ago by
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 8 years ago by
- Description modified (diff)
comment:25 Changed 8 years ago by
- Description modified (diff)
Does this apply to the PARI library, or
gp
?(Of course
gp
uses the PARI library, but I think^{TM} the stand-alonegp
wouldn't need patching for Sage.)