Opened 7 years ago

Last modified 5 years ago

#14346 closed enhancement

The pari spkg is patching upstream too heavily — at Version 21

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 jdemeyer)

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

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
  • get_dlcflags.patch: author known, upstream-worthy, a comment says upstream has a partial fix but doesn't say that better patch has been forwarded, which probably means it wasn't
  • src/kernel/gmp/mp.c: author unknown, upstream-worthy, no comment says it was forwarded, which probably means it wasn't
  • GCC_PR49330.patch: author unknown, upstream-worthy, no comment says it was forwarded, which probably means it wasn't
  • galoisanalysis_p4.patch: author unknown, upstream-worthy, no comment says it was forwarded, which probably means it wasn't
  • polred.patch: author unknown, upstream backport + added function -- the upstream backport is ok, adding a new function isn't [that should be done in the sage library]

Change History (21)

comment:1 Changed 7 years 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 7 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 7 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 7 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 7 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: Changed 7 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 7 years ago by jdemeyer

  • Cc jdemeyer added

comment:8 in reply to: ↑ 6 ; follow-up: Changed 7 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 7 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.

Last edited 7 years ago by jdemeyer (previous) (diff)

comment:10 in reply to: ↑ 8 ; follow-up: Changed 7 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.

Last edited 7 years ago by fbissey (previous) (diff)

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

  • Description modified (diff)

comment:17 follow-up: Changed 7 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 7 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 7 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 7 years ago by Snark

  • Description modified (diff)

comment:21 Changed 7 years ago by jdemeyer

  • Description modified (diff)
Note: See TracTickets for help on using tickets.