Opened 3 years ago

Closed 2 years ago

#22666 closed defect (fixed)

Fix python3 build on Cygwin

Reported by: embray Owned by:
Priority: blocker Milestone: sage-8.0
Component: porting: Cygwin Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Travis Scrimshaw, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 5031e42 (Commits) Commit: 5031e422023459fdbe3a48cb5159abc63ec57cf2
Dependencies: Stopgaps:

Description

Since #22354, Python 3 is now installed unconditionally as a standard package. That's fine, but the last Python 3 version known to work on Cygwin is Python 3.4 (though I have been working with Python upstream to fix that).

In the meantime, here is a patch set needed to get the Python 3 currently in Sage (3.5.1) to at least build, and nominally work.

Change History (21)

comment:1 Changed 3 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 3 years ago by gouezel

I can confirm that Python 3 builds for me with the patch (and does not without).

comment:3 Changed 3 years ago by embray

There may still be runtime issues that impact Sage, but I will address those as they come. This at least addresses building without error.

comment:4 Changed 3 years ago by jdemeyer

What's the upstream status of these patches? I always like when patches have some kind of pointer to an upstream ticket or commit or whatever...

For this reason, I consider the renaming ncurses-issue_14438.patch -> 2.6.5-ncurses-abi6.patch a regression.

comment:5 follow-up: Changed 3 years ago by embray

It's just for consistency's sake with the rest of my patch set, who cares what the file is called? I can put in a reference to the issue it addresses.

comment:6 in reply to: ↑ 5 Changed 3 years ago by jdemeyer

Replying to embray:

I can put in a reference to the issue it addresses.

Please do (to be clear: renaming the file is fine in that case).

comment:7 Changed 3 years ago by embray

Yeah, no a problem. I'm pretty sure all of these patches are upstream already, but there might be one or two that aren't for various reasons. Need to double-check.

comment:8 Changed 2 years ago by tscrim

Did you double-check the patch status Erik? I'm ready to set a positive review unless Jeoren has any other comments (Python3 builds for me with this on Cygwin64).

comment:9 Changed 2 years ago by tscrim

  • Report Upstream changed from N/A to None of the above - read trac for reasoning.
  • Reviewers set to Travis Scrimshaw

comment:10 Changed 2 years ago by tscrim

  • Report Upstream changed from None of the above - read trac for reasoning. to N/A

comment:11 Changed 2 years ago by embray

No, I'll work on that now...

comment:12 Changed 2 years ago by git

  • Commit changed from 133ae6a2a5ba29c992d8af14905a7052474ca028 to 1618c0f0d26c989df228b28db45c714fbc281178

Branch pushed to git repo; I updated commit sha1. New commits:

a55e734This is fixed, for now, in sage by https://trac.sagemath.org/ticket/21399 so we can live without this patch.
90e46fdThis patch has been part of the Cygwin port of Python since Python 2.6, but it has not been needed in Sage.
cd51023This patch seems to have been part of Cygwin's Python for a long time, but I'm not actually sure what case it's for and can't find anything referencing it.
eb02004This patch was needed for the tkinter module to build. But we don't guarantee
46df7dcRewrote this patch to the actual fix to the issue that was accepted upstream
f7f4535This patch has been part of Cygwin's Python for a long time, but I don't think it's still needed.
1618c0fAdds descriptions of more of the new patches

comment:13 Changed 2 years ago by embray

I removed a handful of patches that didn't actually seem to be needed, and added better descriptions for the rest. With this, Python 3 still builds, and at least nominally works. I should stress that that is the only goal of this ticket. Any other patches needed to Python 3 specifically for Sage features to work (if there are any at all) should be held off until those specific issues come up.

comment:14 Changed 2 years ago by tscrim

Jeroen, do you have any more comments? Otherwise, I will set a positive review.

comment:15 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

6

comment:16 Changed 2 years ago by embray

  • Priority changed from major to blocker

comment:17 Changed 2 years ago by git

  • Commit changed from 1618c0f0d26c989df228b28db45c714fbc281178 to 5031e422023459fdbe3a48cb5159abc63ec57cf2

Branch pushed to git repo; I updated commit sha1. New commits:

5031e42Added a brief description of the ncurses patch with link to the original issue.

comment:18 Changed 2 years ago by embray

  • Status changed from needs_work to positive_review

Feel free to disagree, but this addresses Jeroen's only comment.

comment:19 Changed 2 years ago by embray

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Jeroen Demeyer

comment:20 Changed 2 years ago by jdemeyer

Thanks!

comment:21 Changed 2 years ago by vbraun

  • Branch changed from u/embray/cygwin/python3-build to 5031e422023459fdbe3a48cb5159abc63ec57cf2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.