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

Fix python3 build on Cygwin #22666

Closed
embray opened this issue Mar 21, 2017 · 22 comments
Closed

Fix python3 build on Cygwin #22666

embray opened this issue Mar 21, 2017 · 22 comments

Comments

@embray
Copy link
Contributor

embray commented Mar 21, 2017

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.

Component: porting: Cygwin

Author: Erik Bray

Branch/Commit: 5031e42

Reviewer: Travis Scrimshaw, Jeroen Demeyer

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

@embray embray added this to the sage-8.0 milestone Mar 21, 2017
@sagetrac-gouezel
Copy link
Mannequin

sagetrac-gouezel mannequin commented Mar 22, 2017

comment:2

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

@embray
Copy link
Contributor Author

embray commented Mar 23, 2017

comment:3

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

@jdemeyer
Copy link

comment:4

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.

@embray
Copy link
Contributor Author

embray commented Mar 28, 2017

comment:5

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.

@jdemeyer
Copy link

comment:6

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).

@embray
Copy link
Contributor Author

embray commented Mar 28, 2017

comment:7

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.

@tscrim
Copy link
Collaborator

tscrim commented Apr 19, 2017

comment:8

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).

@tscrim
Copy link
Collaborator

tscrim commented Apr 19, 2017

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Apr 19, 2017

Upstream: None of the above - read trac for reasoning.

@tscrim
Copy link
Collaborator

tscrim commented Apr 19, 2017

Changed upstream from None of the above - read trac for reasoning. to none

@embray
Copy link
Contributor Author

embray commented Apr 20, 2017

comment:11

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 20, 2017

Changed commit from 133ae6a to 1618c0f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 20, 2017

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

a55e734This is fixed, for now, in sage by https://github.com/sagemath/sage-prod/issues/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

@embray
Copy link
Contributor Author

embray commented Apr 20, 2017

comment:13

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.

@tscrim
Copy link
Collaborator

tscrim commented Apr 20, 2017

comment:14

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

@jdemeyer
Copy link

comment:15

[comment:6]

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 21, 2017

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 21, 2017

Changed commit from 1618c0f to 5031e42

@embray
Copy link
Contributor Author

embray commented Apr 21, 2017

comment:18

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

@embray
Copy link
Contributor Author

embray commented Apr 21, 2017

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Jeroen Demeyer

@jdemeyer
Copy link

comment:20

Thanks!

@vbraun
Copy link
Member

vbraun commented Apr 23, 2017

Changed branch from u/embray/cygwin/python3-build to 5031e42

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