Re: [sage-devel] Patching policy
To be clear I am not advocating to create a fork of every dependency. That would probably only worsen the problem (making it even easier to pull the "patch trigger" and harder to find the patches for distributions). -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To post to this group, send email to sage-devel@googlegroups.com. Visit this group at https://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/d/optout.
Re: [sage-devel] Patching policy
On 2018-06-08 13:05, Dima Pasechnik wrote: patches are not the best way to update things. VCSs are much better... Fair enough, that's a reasonable argument, although most patches actually don't need to be updated. For example, if the patch comes from upstream, there is no need to do anything. And patches which are not submitted/accepted upstream may survive many versions without conflicts. -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To post to this group, send email to sage-devel@googlegroups.com. Visit this group at https://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/d/optout.
Re: [sage-devel] Patching policy
See also http://doc.sagemath.org/html/en/developer/packaging.html#how-to-maintain-a-set-of-patches On Friday, June 8, 2018 at 4:05:47 AM UTC-7, Dima Pasechnik wrote: > > > > On Friday, June 8, 2018 at 9:25:33 AM UTC+1, Jeroen Demeyer wrote: >> >> On 2018-06-08 07:47, Ralf Stephan wrote: >> > It might be an idea to semi-automatize this, i.e., build a tool that >> > takes a Sage version and creates forks of some packages A,B on github >> > under sage/sage-A, sage/sage-B. >> >> Which problem would that solve? >> >> patches are not the best way to update things. VCSs are much better... > -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To post to this group, send email to sage-devel@googlegroups.com. Visit this group at https://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/d/optout.
Re: [sage-devel] Patching policy
On Friday, June 8, 2018 at 9:25:33 AM UTC+1, Jeroen Demeyer wrote: > > On 2018-06-08 07:47, Ralf Stephan wrote: > > It might be an idea to semi-automatize this, i.e., build a tool that > > takes a Sage version and creates forks of some packages A,B on github > > under sage/sage-A, sage/sage-B. > > Which problem would that solve? > > patches are not the best way to update things. VCSs are much better... -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To post to this group, send email to sage-devel@googlegroups.com. Visit this group at https://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/d/optout.
Re: [sage-devel] Patching policy
On Friday, June 8, 2018 at 10:25:33 AM UTC+2, Jeroen Demeyer wrote: > > On 2018-06-08 07:47, Ralf Stephan wrote: > > It might be an idea to semi-automatize this, i.e., build a tool that > > takes a Sage version and creates forks of some packages A,B on github > > under sage/sage-A, sage/sage-B. > > Which problem would that solve? > For Sage devs, that of creating and maintaining a fork of a package. I do not suggest that making a fork per se solves a problem that is not already solved by the patches that are applied. However, it seems from the discussion that packagers are skeptical of Sage's patches. -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To post to this group, send email to sage-devel@googlegroups.com. Visit this group at https://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/d/optout.
Re: [sage-devel] Patching policy
On 2018-06-08 07:47, Ralf Stephan wrote: It might be an idea to semi-automatize this, i.e., build a tool that takes a Sage version and creates forks of some packages A,B on github under sage/sage-A, sage/sage-B. Which problem would that solve? -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To post to this group, send email to sage-devel@googlegroups.com. Visit this group at https://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/d/optout.
Re: [sage-devel] Patching policy
On Thursday, June 7, 2018 at 2:36:35 PM UTC+2, Timo Kaufmann wrote: > > In some cases where upstream has vanished and sage effectiely maintains > the project through patches anyways, it may be a better idea to just fork > the project. > It might be an idea to semi-automatize this, i.e., build a tool that takes a Sage version and creates forks of some packages A,B on github under sage/sage-A, sage/sage-B. -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To post to this group, send email to sage-devel@googlegroups.com. Visit this group at https://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/d/optout.
Re: [sage-devel] Patching policy
2018-06-07 15:06 GMT+02:00 Jeroen Demeyer : > And I'm not saying there should be absolutely no patches, just that they >> should be the very last resort. >> > > I mostly agree with this, it's what I'm already doing. It just depends > where you put the borderline of "very last resort" and there we probably > differ. > For example the `backports.shutil_get_terminal_size` patch: It pretty much only fixes a formatting issue in the error messages right? I don't see that as last resort. > But I don't see how this would help distributions. As long as a package > has at least 1 essential patch (even a 1-liner in the case of GLPK), > distros have to deal with it. > That assumes that there always is an essential patch already. And even if there is, two patches don't have the same cost as one. They have to both be checked, understood, documented and maybe discussed with a maintainer (who does not see sage as his primary priority). -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To post to this group, send email to sage-devel@googlegroups.com. Visit this group at https://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/d/optout.
Re: [sage-devel] Patching policy
On 2018-06-07 14:36, Timo Kaufmann wrote: For what its worth, here is debians fix (self-labeled as "extra-hacky") for that issue: https://salsa.debian.org/science-team/sagemath/blob/master/debian/patches/dt-version-glpk-4.60-extra-hacky-fixes.patch That's not a fix at all. I could easily come up with a doctest that would break using Debian's "fix". So it's certainly not acceptable for Sage. And I'm not saying there should be absolutely no patches, just that they should be the very last resort. I mostly agree with this, it's what I'm already doing. It just depends where you put the borderline of "very last resort" and there we probably differ. But I don't see how this would help distributions. As long as a package has at least 1 essential patch (even a 1-liner in the case of GLPK), distros have to deal with it. Jeroen. -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To post to this group, send email to sage-devel@googlegroups.com. Visit this group at https://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/d/optout.
Re: [sage-devel] Patching policy
2018-06-07 14:16 GMT+02:00 Jeroen Demeyer : > On 2018-06-07 13:24, Timo Kaufmann wrote: > >> I don't really agree but even if that was the case, the PARI stackwarn >> patch could have been handled through filtering within sage instead >> (which I proposed in that ticket). >> > > You proposed filtering *in the testsuite*. That comes back to my point > about fixing the testsuite: filtering in the testsuite only makes the tests > pass, but it doesn't really fix anything for end users. > That was because in that case I didn't actually see the warning as an error / something to avoid. I simply introduced filtering in the testsuite to avoid having to add warnings to all the tests and making them even more brittle. If you disagree (which you apparently do) we could've instead added filtering directly to the sage<->pari interface. > The problem with your proposal is that in many cases, it is hard or > impossible to work around the bug. For example, I have no idea how to "work > around" build/pkgs/glpk/patches/error_recovery.patch > For what its worth, here is debians fix (self-labeled as "extra-hacky") for that issue: https://salsa.debian.org/science-team/sagemath/blob/master/debian/patches/dt-version-glpk-4.60-extra-hacky-fixes.patch Which kind of supports my points that (1) distributions will have to work around it anyways and (2) the solutions of the distributions are probably not as good as the solutions the sage community could come up with. In that specific case I opted to adopt the glpk patch for nixos because I don't really understand the debian fix (I barely understand the dails of the problem). So if you don't want to patch the upstream package, then the only remaining > option is making Sage worse (either by not implementing the functionality > which requires the patch, or by accepting that a bug cannot be fixed). > And I'm not saying there should be absolutely no patches, just that they should be the very last resort. So if working around the problem is really not possible and there exists no other solution it'll have to be a patch. Just make sure upstream is aware that sage and probably most distributions shipping sage will have to patch their sotware, maybe that will make them reconsider. In some cases where upstream has vanished and sage effectiely maintains the project through patches anyways, it may be a better idea to just fork the project. > I'm biased of course. >> > > We are all biased in our own ways... > > I should also add that my opinion is my opinion only. Other SageMath > developers may have different opinions. Of course. Unless somebody else chimes in to add a different opinion, I think its relatively safe to assume that the other devs think similarly. -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To post to this group, send email to sage-devel@googlegroups.com. Visit this group at https://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/d/optout.
Re: [sage-devel] Patching policy
On 2018-06-07 13:24, Timo Kaufmann wrote: I don't really agree but even if that was the case, the PARI stackwarn patch could have been handled through filtering within sage instead (which I proposed in that ticket). You proposed filtering *in the testsuite*. That comes back to my point about fixing the testsuite: filtering in the testsuite only makes the tests pass, but it doesn't really fix anything for end users. The problem with your proposal is that in many cases, it is hard or impossible to work around the bug. For example, I have no idea how to "work around" build/pkgs/glpk/patches/error_recovery.patch So if you don't want to patch the upstream package, then the only remaining option is making Sage worse (either by not implementing the functionality which requires the patch, or by accepting that a bug cannot be fixed). I'm biased of course. We are all biased in our own ways... I should also add that my opinion is my opinion only. Other SageMath developers may have different opinions. -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To post to this group, send email to sage-devel@googlegroups.com. Visit this group at https://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/d/optout.
Re: [sage-devel] Patching policy
2018-06-07 11:12 GMT+02:00 Jeroen Demeyer : > I think that your post is focusing too much on tests, as if the only > purpose of Sage is to pass its testsuite. It's actually the other way > around: the purpose of the testsuite is to ensure that Sage functions > correctly. Of course. But the testsuite should always pass as long as sage is "working". Its the best proxy available. Sometimes the test suite is a bit too brittle. By patching the testsuite to accept buggy output anyway, you're not really > fixing anything, just hiding the problem. > If its a functional bug (`2 + 2 = 5`) I agree. But if its just a formatting bug, that should not fail the test suite in my opinion. By the way, the difference that you make between category 2 and 3 of > patches is not so relevant: I would argue that the PARI stackwarn.patch is > more essential (as in: more likely to affect users) than the > re_match_index-issue_27177.patch Python patch. > I don't really agree but even if that was the case, the PARI stackwarn patch could have been handled through filtering within sage instead (which I proposed in that ticket). Thats really my main point: use other solutions if possible. > I would propose to make it a policy to only include sage patches when >> *absolutely necessary*. If ugly workarounds or even monkey-patching is >> necessary >> for that, that sucks. But distributions will usually come up with even >> uglier >> workarounds (since they don't know the code) anyways, just resulting in >> duplication of effort. >> > > There is a constant ongoing tension between downstream (Sage), upstream > (PARI, Python, ...) and distributions (Debian, NixOS, ...). Nobody wants to > be the one needing to fix the problem. So upstream tries to convince > downstream that the problem is on their side, downstream tries to push the > problem to distributions and distributions probably bother both upstream > and downstream. > I'd argue distributions is the worst place to fix problems because that results in duplication of work and the people fixing the issues might not know what they're doing. > You seem to be blaming Sage for patching its dependencies, but in many > cases it would be even more valid to blame upstream for not accepting those > patches (or for not making a new release containing those patches). That > way, the problem is really fixed for everybody: not just Sage but all users > of a package. Or you could blame your distro for not applying that patch > too. > I really don't want to blame anyone (especially not you, I'm grateful that you already reviewed many of my patches). Sorry if it sounds that way. I'm just proposing to choose the most pragmatic solution. Its always best to fix the problem as far upstream as possible: Of course it would be ideal if the upstream packages would accept those patches. But if that is not the case for various reasons, I'm arguing that working around it in sage is the next best place. > Of all Sage developers, I could very well be the one adding most patches > to its packages. Whenever I feel the need for patching a package, I ask > myself what the best solution to the problem is: it could be an upstream > patch or it could be a change in Sage. When it's an upstream patch, I make > sure that it fixes a genuine upstream bug and I submit the patch. In most > cases, the patch is then accepted upstream. I very much agree with that process. As you said, fixing problems upstream fixes them for everybody. > However, when it's not accepted (for whatever reason), I don't want to do > the effort of figuring out a solution without patching the package. I feel > like that is just a waste of time since I already have a working solution > for the problem (patching the package) and working around a problem is > often harder than fixing it. > I'm arguing that (if distributions are considered first class citizens) it is very much not a waste of time. I think the ticket should be blocked until upstream has accepted the patch or it is clear they won't. If the ticket author doesn't work around the problem in the second case, distributions will have to. So the time is spent either way. Even if distributions just adopt the patch to the upstream package, figuring out which patch is needed, adopting it, testing it etc. takes significant time. I understand that your time is very valuable and you can do with it whatever you want and I'm not saying you should solve all the problems. I'm just saying we should adopt a policy and adhere to it as a community. What I'm trying to say is that patching a package in Sage is always a very > deliberate conscious choice and not just "whatever, let's patch this > package". So while I certainly understand the concerns of the > distributions, I'm personally not really willing to change anything. > I'm sad to hear that. I think it would be best for sage and gain it a lot of users and potential contributors if the community would invest some effort into being less
Re: [sage-devel] Patching policy
I think that your post is focusing too much on tests, as if the only purpose of Sage is to pass its testsuite. It's actually the other way around: the purpose of the testsuite is to ensure that Sage functions correctly. By patching the testsuite to accept buggy output anyway, you're not really fixing anything, just hiding the problem. By the way, the difference that you make between category 2 and 3 of patches is not so relevant: I would argue that the PARI stackwarn.patch is more essential (as in: more likely to affect users) than the re_match_index-issue_27177.patch Python patch. I would propose to make it a policy to only include sage patches when *absolutely necessary*. If ugly workarounds or even monkey-patching is necessary for that, that sucks. But distributions will usually come up with even uglier workarounds (since they don't know the code) anyways, just resulting in duplication of effort. There is a constant ongoing tension between downstream (Sage), upstream (PARI, Python, ...) and distributions (Debian, NixOS, ...). Nobody wants to be the one needing to fix the problem. So upstream tries to convince downstream that the problem is on their side, downstream tries to push the problem to distributions and distributions probably bother both upstream and downstream. You seem to be blaming Sage for patching its dependencies, but in many cases it would be even more valid to blame upstream for not accepting those patches (or for not making a new release containing those patches). That way, the problem is really fixed for everybody: not just Sage but all users of a package. Or you could blame your distro for not applying that patch too. Of all Sage developers, I could very well be the one adding most patches to its packages. Whenever I feel the need for patching a package, I ask myself what the best solution to the problem is: it could be an upstream patch or it could be a change in Sage. When it's an upstream patch, I make sure that it fixes a genuine upstream bug and I submit the patch. In most cases, the patch is then accepted upstream. However, when it's not accepted (for whatever reason), I don't want to do the effort of figuring out a solution without patching the package. I feel like that is just a waste of time since I already have a working solution for the problem (patching the package) and working around a problem is often harder than fixing it. What I'm trying to say is that patching a package in Sage is always a very deliberate conscious choice and not just "whatever, let's patch this package". So while I certainly understand the concerns of the distributions, I'm personally not really willing to change anything. Jeroen. -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To post to this group, send email to sage-devel@googlegroups.com. Visit this group at https://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/d/optout.
[sage-devel] Patching policy
I have recently re-written the nixpkgs sage package to use system
packages[1]
(not merged yet). While doing that I noticed that while sage patches a lot
of
its dependencies (81 out of 276 packages have a `patches` folder).
There are more or less 3 categories of patches:
1. sage-the-distribution specific patches
These patch paths, makefiles, wrappers etc. to make packages work with
other
spkgs. Thats pretty much what every distro does, too.
2. essential patches
Patches that are absolutely needed for sage to work. I consider the
python2
`re_match_index-issue_27177.patch` an example of this category, because
- it is needed for a basic sage function (regex matching)
- there is no reasonable way to work around it (`re` cannot be monkey
patched).
I'm not entirely sure why sage needs to overwrite the python integer,
but
I'm sure there is a good reason for it.
3. small fixes / opinionated patches / issues that could be worked around
within
sage
Examples for this:
- fixing a formatting failure in `backports.shutil_get_terminal_size` by
patching the spkg. That could instead be fixed by patching the sage
tests
(#25354).
- patch python to fix python bug 5755 instead of either adjusting the
doctests
or monkey-patching instead (#25316).
- patch pari to remove a warning instead of dismissing that warning within
sage or adapting the doctests (#25312)
- patch python to add path security checks (#25358)
There is no way around (1) and those patches don't really hurt anything
(except
it might be nice to clearly mark them as sage-the-distribution specific
patches). There is no way around the (2) patches and including applying
them is
usually a good idea, independent of sage.
(3) is the issue. Those patches are usually included because they fix the
problem in an easier or cleaner way than working around it within sage,
which is
reasonable. Those patches make packaging sage very hard though.
Distributions
have to either include these patches (there are good reasons to be reluctant
against new patches and you'll usually have to convince a maintainer of
their
value) or add patches to sage to work around the issue.
I would propose to make it a policy to only include sage patches when
*absolutely necessary*. If ugly workarounds or even monkey-patching is
necessary
for that, that sucks. But distributions will usually come up with even
uglier
workarounds (since they don't know the code) anyways, just resulting in
duplication of effort.
Of course I don't mean to accuse anybody that added those examples in the
past.
I understand the reasoning behind it ("cleaner" solutions). I just want to
raise
awareness about the problems this causes downstream, especially for new sage
packages (most of those issues can be forgotten once they're "solved" for
your
distribution, but the next distribution will have to solve them all over
again).
[1] https://github.com/NixOS/nixpkgs/pull/39981
--
You received this message because you are subscribed to the Google Groups
"sage-devel" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to sage-devel+unsubscr...@googlegroups.com.
To post to this group, send email to sage-devel@googlegroups.com.
Visit this group at https://groups.google.com/group/sage-devel.
For more options, visit https://groups.google.com/d/optout.
