Opened 9 months ago

Closed 9 months ago

Last modified 9 months ago

#27384 closed defect (wontfix)

Apply Cysignals fix for Cygwin exception handler on sigaltstack

Reported by: embray Owned by:
Priority: blocker Milestone: sage-duplicate/invalid/wontfix
Component: porting: Cygwin Keywords:
Cc: jdemeyer Merged in:
Authors: Erik Bray Reviewers:
Report Upstream: N/A Work issues:
Branch: u/embray/cygwin/patch-cysignals-altstack-exception-handler (Commits) Commit: f95dcff22000f525b3a114d89dd10b2e134589fa
Dependencies: Stopgaps:

Description (last modified by embray)

This adds a version of the patch from https://github.com/sagemath/cysignals/pull/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.

Change History (6)

comment:1 Changed 9 months ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/cygwin/patch-cysignals-altstack-exception-handler
  • Cc jdemeyer added
  • Commit set to f95dcff22000f525b3a114d89dd10b2e134589fa
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

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

comment:2 follow-up: Changed 9 months ago by jdemeyer

Don't you need https://github.com/sagemath/cysignals/commit/8913e0b66a9acea11a5971868d748cde368ae29b also?

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

comment:3 in reply to: ↑ 2 Changed 9 months ago by embray

Replying to jdemeyer:

Don't you need https://github.com/sagemath/cysignals/commit/8913e0b66a9acea11a5971868d748cde368ae29b 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.

comment:4 Changed 9 months ago by jdemeyer

  • Milestone changed from sage-8.7 to sage-duplicate/invalid/wontfix
  • Resolution set to duplicate
  • Status changed from needs_review to closed

Can this be closed in favour of #27070?

comment:5 Changed 9 months ago by embray

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.

comment:6 Changed 9 months ago by embray

  • Resolution changed from duplicate to wontfix
Note: See TracTickets for help on using tickets.