Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Pynac unordered_map patch #27221

Closed
jdemeyer opened this issue Feb 5, 2019 · 14 comments
Closed

Add Pynac unordered_map patch #27221

jdemeyer opened this issue Feb 5, 2019 · 14 comments

Comments

@jdemeyer
Copy link

jdemeyer commented Feb 5, 2019

In order to finish #22029, we need one patch to Pynac, accepted upstream at
pynac/pynac#339

This affects only 2 doctests in all of Sage, so it's quite innocent.

CC: @rwst @kiwifb @antonio-rojas

Component: packages: standard

Author: Jeroen Demeyer

Branch/Commit: u/jdemeyer/add_pynac_unordered_map_patch @ 86bbcf5

Issue created by migration from https://trac.sagemath.org/ticket/27221

@jdemeyer jdemeyer added this to the sage-8.7 milestone Feb 5, 2019
@jdemeyer
Copy link
Author

jdemeyer commented Feb 5, 2019

@jdemeyer
Copy link
Author

jdemeyer commented Feb 5, 2019

New commits:

86bbcf5Add Pynac unordered_map patch

@jdemeyer
Copy link
Author

jdemeyer commented Feb 5, 2019

Commit: 86bbcf5

@timokau
Copy link
Contributor

timokau commented Feb 5, 2019

comment:3

We've had the same discussion already, but: Why patch? Given that sage is the main consumer of pynac, why not cut a new release instead?

@kiwifb
Copy link
Member

kiwifb commented Feb 5, 2019

comment:4

Replying to @timokau:

We've had the same discussion already, but: Why patch? Given that sage is the main consumer of pynac, why not cut a new release instead?

Right, right. Is there a matching pull request? Does the maintainer (Ralph) react in a swift enough fashion for a new release? If a new release is cut now, are there any new features or changes beside this patch that will require changes on sage side?

Yes in an ideal world we want and wait for the next release and we can make that request but sage is a bit of a fast conveyor belt at times :)

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

jdemeyer commented Feb 5, 2019

comment:6

Replying to @timokau:

Given that sage is the main consumer of pynac, why not cut a new release instead?

Sage is the main consumer of pynac, but Sage is not pynac.

@jdemeyer
Copy link
Author

jdemeyer commented Feb 5, 2019

comment:7

Replying to @kiwifb:

If a new release is cut now, are there any new features or changes beside this patch that will require changes on sage side?

No. That might actually be a reason not to make a new release: we just had an upgrade (#26995) and the PR for this ticket is the only change to master since then.

As I mentioned in the ticket description, this patch affects only 2 doctests in Sage which would otherwise break with #22029.

@timokau
Copy link
Contributor

timokau commented Feb 5, 2019

comment:8

Replying to @jdemeyer:

Replying to @kiwifb:

If a new release is cut now, are there any new features or changes beside this patch that will require changes on sage side?

No. That might actually be a reason not to make a new release: we just had an upgrade (#26995) and the PR for this ticket is the only change to master since then.

Why would that be an argument against a new release? The only reason against frequent releases I can think of is packager fatigue. Adding a patch to pynac would be more work for packagers than just bumping the version. Is there another reason?

@jdemeyer
Copy link
Author

jdemeyer commented Feb 5, 2019

comment:9

Replying to @timokau:

Adding a patch to pynac would be more work for packagers than just bumping the version.

You could also not add the patch and live with 2 additional doctest failures.

I think that #22029 is too important to be held back by a Pynac release.

@timokau
Copy link
Contributor

timokau commented Feb 5, 2019

comment:10

Replying to @jdemeyer:

Replying to @timokau:

Adding a patch to pynac would be more work for packagers than just bumping the version.

You could also not add the patch and live with 2 additional doctest failures.

I couldn't, any test failures fail the build.

I think that #22029 is too important to be held back by a Pynac release.

What makes that 2yr old ticket so urgent now?

@jdemeyer
Copy link
Author

jdemeyer commented Feb 8, 2019

comment:11

Replying to @timokau:

Replying to @jdemeyer:

Replying to @timokau:

Adding a patch to pynac would be more work for packagers than just bumping the version.

You could also not add the patch and live with 2 additional doctest failures.

I couldn't, any test failures fail the build.

Debian seems to think otherwise, but I'm actually glad that you don't want any test failures. OK, but then you could still add a patch to remove/disable those 2 tests.

What makes that 2yr old ticket so urgent now?

It's a ticket which makes far-reaching changing and I'm worried about regressions against it.

@timokau
Copy link
Contributor

timokau commented Feb 8, 2019

comment:12

Replying to @jdemeyer:

Replying to @timokau:

Replying to @jdemeyer:

Replying to @timokau:

Adding a patch to pynac would be more work for packagers than just bumping the version.

You could also not add the patch and live with 2 additional doctest failures.

I couldn't, any test failures fail the build.

Debian seems to think otherwise, but I'm actually glad that you don't want any test failures. OK, but then you could still add a patch to remove/disable those 2 tests.

Debian is in a somewhat more difficult position, with their limited control of the environment sage executes in and additional bureaucracy. Still, if you ask me they would be better off explicitly marking known failures instead of just picking some threshold of acceptable test failures. This doesn't really fit into this ticket though, if anyone wants to continue that discussion we can do that on sage-packaging :)

(One more thing I'm pretty excited about: Nix is now available in debian)

What makes that 2yr old ticket so urgent now?

It's a ticket which makes far-reaching changing and I'm worried about regressions against it.

Alright, I haven't looked into the details and don't know how important it is. Patching pynac is not a terribly big deal for me (especially since sage is its only consumer in nixpkgs). So I'm mostly arguing out of principle, since I'm trying to change the "just patch" attitude.

Having expressed my opinion about this (I'm still not convinced patching is worth it here, waiting for a new pynac release would probably only introduce a couple of days delay), I won't argue any further.

@mezzarobba
Copy link
Member

comment:13

Ralf just released Pynac 0.7.24 = 0.7.23 + this patch. See #27241.

@mezzarobba mezzarobba removed this from the sage-8.7 milestone Feb 9, 2019
@jdemeyer jdemeyer closed this as completed Feb 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants