Opened 4 years ago
Closed 3 years ago
#23263 closed defect (duplicate)
Allow use of an unpatched pari
Reported by: | infinity0 | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | doctest framework | Keywords: | |
Cc: | thansen | Merged in: | |
Authors: | Reviewers: | Tobias Hansen, Julian Rüth | |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
This two-line patch in Sage allows us to use an unpatched pari in Debian - which is maintained by upstream pari who does not want to apply Sage's "ignore stack warnings" patch.
It works by ignoring the pari stack warnings in the doctest parser instead.
(Stack warnings will still appear during Sage's normal usage, but one can optionally use a patched pari to avoid these if one needs to.)
Change History (17)
comment:1 Changed 4 years ago by
- Branch set to u/infinity0/allow_use_of_an_unpatched_pari
comment:2 Changed 4 years ago by
- Cc thansen added
- Commit set to 551a69ab03710260fcf18828a2679b7cea68508b
- Component changed from PLEASE CHANGE to doctest framework
- Description modified (diff)
- Status changed from new to needs_review
- Type changed from PLEASE CHANGE to defect
comment:3 Changed 4 years ago by
I don't really get the point of this patch. We are still patching PARI, so which problem does this fix? I think this branch should be applied by distros if they feel the need, not by Sage.
I don't want to overcomplicate our doctest framework just because upstream PARI is so stubborn to insist that debug messages should be shown unconditionally with no way to disable them. The correct solution here is fixing PARI, not Sage.
comment:4 follow-up: ↓ 5 Changed 4 years ago by
OK, I found the thread and I can agree that the PARI position does not make much sense. Today is 2017, stack size warnings should at best be considered "memory debugging messages" and not in the same category as other types of warnings.
Nevertheless, this patch would help other distros packages Sage more quickly. It is small, but the amount of small things like this in Sage quickly adds up to a major time sink when packaging. Yes, we can keep carrying this in Debian if you don't agree, it's up to you.
When I find some time I'll try to re-raise this issue on the pari mailing list.
comment:5 in reply to: ↑ 4 Changed 4 years ago by
Replying to infinity0:
Nevertheless, this patch would help other distros packages Sage more quickly. It is small, but the amount of small things like this in Sage quickly adds up to a major time sink when packaging. Yes, we can keep carrying this in Debian if you don't agree, it's up to you.
Exactly the same arguments could be used to apply the PARI patch to the Debian PARI package.
comment:6 follow-up: ↓ 7 Changed 4 years ago by
I don't quite follow that - how would applying the PARI patch to Debian, help other distros package Sage (or PARI) more quickly? And even if it did, I don't think it would be appropriate to patch package A to make it easier to package B.
As you have seen, we have applied some Sage patches to other packages in Debian. In this situation, I can't by convention (because there is already an active maintainer), and even if I was the only maintainer, upstream has voiced some opposition to the patch which makes me less certain about applying it - there might be issue or concern that he has about the patch, that I don't know about. If I knew more about PARI's internals and its usecases I might be more confident about trying to override him for this patch, but I'm not.
Anyway, no worries I don't mean to pressure you. The next step for us would be to contact PARI again.
comment:7 in reply to: ↑ 6 ; follow-up: ↓ 8 Changed 4 years ago by
Replying to infinity0:
I don't quite follow that - how would applying the PARI patch to Debian, help other distros package Sage (or PARI) more quickly?
Right, I was just thinking about Debian...
Anyway, I still think that this patch has no place in the Sage doctest framework.
comment:8 in reply to: ↑ 7 Changed 4 years ago by
Replying to jdemeyer:
Replying to infinity0:
I don't quite follow that - how would applying the PARI patch to Debian, help other distros package Sage (or PARI) more quickly?
Right, I was just thinking about Debian...
Anyway, I still think that this patch has no place in the Sage doctest framework.
Not so long ago, there was a proposal on sage-devel to allow some of the packages to be picked from the system. The patch in this ticket would be useful if the PARI/GP used is the one from the system.
comment:9 Changed 4 years ago by
I am still hoping that PARI upstream will see the light and just accept my stack warning patch.
comment:10 Changed 4 years ago by
Well, I am lucky enough to do more or less what I want in sage-on-gentoo. I have the nagging feeling that I could inherit a number of packages in the official Gentoo tree (including pari) if I was to finally become an official dev.
comment:11 Changed 4 years ago by
- Reviewers set to Julian Rüth
I think this patch makes perfect sense. If the author could add a comment to the source code explaining that this is not relevant for the patched PARI that Sage is shipping as of now, that would be great. Anyway, I would be willing to give it positive review unless jdemeyer has very strong feelings about it.
comment:12 Changed 4 years ago by
To be honest, I mostly have strong feelings about the upstream PARI patch that they do not want to accept (why???). It feels like accepting this ticket in Sage is like admitting defeat. So I certainly won't set this to positive_review.
On the other hand, if this patch makes other people's lives easier, then I will not veto this patch.
comment:13 Changed 3 years ago by
Oh, it's been a while. thansen, if you still think that this would make things easier for packagers, please add a short comment, and I will set this to positive review.
comment:14 follow-up: ↓ 15 Changed 3 years ago by
Breaking news (from last month or so): upstream PARI finally accepted this patch in master
. So it should be sufficient to wait for the next PARI release (or backport the patch in Debian).
comment:15 in reply to: ↑ 14 Changed 3 years ago by
Replying to jdemeyer:
upstream PARI finally accepted this patch in
master
.
Actually, I just saw that they reverted part of that patch again. So we are getting closer, but we are not quite there yet.
comment:16 Changed 3 years ago by
- Branch u/infinity0/allow_use_of_an_unpatched_pari deleted
- Commit 551a69ab03710260fcf18828a2679b7cea68508b deleted
- Milestone changed from sage-8.0 to sage-duplicate/invalid/wontfix
- Reviewers changed from Julian Rüth to Tobias Hansen, Julian Rüth
I think that this can be closed in light of #24481.
comment:17 Changed 3 years ago by
- Resolution set to duplicate
- Status changed from needs_review to closed
New commits:
Ignore warnings of PARI increasing the stack size when parsing doctests