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

Apply Cysignals fix for Cygwin exception handler on sigaltstack #27384

Closed
embray opened this issue Feb 28, 2019 · 9 comments
Closed

Apply Cysignals fix for Cygwin exception handler on sigaltstack #27384

embray opened this issue Feb 28, 2019 · 9 comments

Comments

@embray
Copy link
Contributor

embray commented Feb 28, 2019

This adds a version of the patch from sagemath/cysignals#108 modified to apply to cysignals 1.8.1.

Since the issue only affects Cygwin I thought for now it would be easier (as in #27267) to apply the patch locally, rather than wait on Jeroen to approve the upstream PR, make a new release of cysignals, and make Sage depend on that release.

This fix certainly resolves the blocker issue well-enough for me for the time being. Fixing this is a prerequisite for fixing #27214.

CC: @jdemeyer

Component: porting: Cygwin

Author: Erik Bray

Branch/Commit: u/embray/cygwin/patch-cysignals-altstack-exception-handler @ f95dcff

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

@embray embray added this to the sage-8.7 milestone Feb 28, 2019
@embray
Copy link
Contributor Author

embray commented Feb 28, 2019

New commits:

f95dcffTrac #27384: Patch cysignals with fix for Cygwin's sigaltstack exception

@embray

This comment has been minimized.

@embray
Copy link
Contributor Author

embray commented Feb 28, 2019

@embray
Copy link
Contributor Author

embray commented Feb 28, 2019

Commit: f95dcff

@embray
Copy link
Contributor Author

embray commented Feb 28, 2019

Author: Erik Bray

@jdemeyer
Copy link

comment:2

Don't you need sagemath/cysignals@8913e0b also?

It's not a big deal to make a new cysignals release, just not right now.

@embray
Copy link
Contributor Author

embray commented Feb 28, 2019

comment:3

Replying to @jdemeyer:

Don't you need sagemath/cysignals@8913e0b also?

I thought so at first too, but the change that that was fixing was after 1.8.1 so it's not relevant right now (I will need it for any future cysignals update in Sage)

It's not a big deal to make a new cysignals release, just not right now.

I know it's not a big deal. But as I explained (and I think you're agreeing) it's easier to just include the patch in the Sage SPKG for now.

@jdemeyer
Copy link

comment:4

Can this be closed in favour of #27070?

@jdemeyer jdemeyer removed this from the sage-8.7 milestone Mar 10, 2019
@embray
Copy link
Contributor Author

embray commented Mar 11, 2019

comment:5

Sure, as long as Cysignals 1.10 includes the fix (and doesn't break anything else). I will review the changelog to make sure there isn't anything else suspect.

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

2 participants