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

With Python 3 build, do not install python2 #28426

Closed
jhpalmieri opened this issue Aug 29, 2019 · 17 comments
Closed

With Python 3 build, do not install python2 #28426

jhpalmieri opened this issue Aug 29, 2019 · 17 comments

Comments

@jhpalmieri
Copy link
Member

When Sage is built with Python 3, there is no need to build Python 2, so let's skip it.

CC: @jdemeyer @fchapoton @embray @kiwifb @slel @antonio-rojas @isuruf

Component: python3

Author: John Palmieri

Branch/Commit: fa1482d

Reviewer: Frédéric Chapoton

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

@jhpalmieri jhpalmieri added this to the sage-8.9 milestone Aug 29, 2019
@jhpalmieri
Copy link
Member Author

Branch: u/jhpalmieri/no-python2

@jhpalmieri
Copy link
Member Author

Commit: f5de6f9

@jhpalmieri
Copy link
Member Author

comment:2

With this branch and a Python 3 build, I get two new doctest failures:

sage -t --warn-long 60.3 src/sage/doctest/control.py
**********************************************************************
File "src/sage/doctest/control.py", line 630, in sage.doctest.control.DocTestController.test_safe_directory
Failed example:
    DC.test_safe_directory(d)
Expected:
    Traceback (most recent call last):
    ...
    RuntimeError: refusing to run doctests...
Got:
    <BLANKLINE>
**********************************************************************
1 item had failures:
   1 of   9 in sage.doctest.control.DocTestController.test_safe_directory
    [204 tests, 1 failure, 4.24 s]
sage -t --warn-long 60.3 src/sage/tests/cmdline.py
**********************************************************************
File "src/sage/tests/cmdline.py", line 412, in sage.tests.cmdline.test_executable
Failed example:
    print(err)
Expected:
    ...
    RuntimeError: refusing to run doctests...
Got:
    <BLANKLINE>
**********************************************************************
File "src/sage/tests/cmdline.py", line 416, in sage.tests.cmdline.test_executable
Failed example:
    print(err)
Expected:
    ...
    RuntimeError: refusing to run doctests...
Got:
    <BLANKLINE>
**********************************************************************
1 item had failures:
   2 of 245 in sage.tests.cmdline.test_executable
    [244 tests, 2 failures, 69.42 s]
----------------------------------------------------------------------
sage -t --warn-long 60.3 src/sage/doctest/control.py  # 1 doctest failed
sage -t --warn-long 60.3 src/sage/tests/cmdline.py  # 2 doctests failed
----------------------------------------------------------------------

Should these be marked # py2?


New commits:

f5de6f9trac 28426: In a Python 3 build of Sage, do not build Python 2.

@fchapoton
Copy link
Contributor

comment:4

see the discussion in #27529 about the same failing doctests

@jhpalmieri
Copy link
Member Author

comment:5

Replying to @fchapoton:

see the discussion in #27529 about the same failing doctests

One comment there was that if python is python3, then the tests would work. I just created a symlink python -> python3 here, and the tests still fail. I think either marking the tests as py2, or removing them (#26457) would be good. Or would #25358 help?

@fchapoton
Copy link
Contributor

comment:6

Let us just tag them with #py2.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 3, 2019

Changed commit from f5de6f9 to fa1482d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 3, 2019

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

656f6b7trac 28426: In a Python 3 build of Sage, do not build Python 2.
fa1482dtrac 28426: tag some doctests as 'py2'

@jhpalmieri
Copy link
Member Author

comment:8

Okay, ready for review.

@fchapoton
Copy link
Contributor

comment:9

This looks good to me. I would like to have more opinions from experts, in particular people packaging sage for various Linux distributions.

And this will have to wait for 9.0.beta1, I think.

@embray
Copy link
Contributor

embray commented Sep 18, 2019

comment:10

I agree this should wait for 9.0 at the earliest, but +1 in principle. No reason to burden people with two Pythons.

@embray embray modified the milestones: sage-8.9, sage-9.0 Sep 18, 2019
@fchapoton
Copy link
Contributor

comment:11

So can we switch to positive review ? No packager did react.

@davidlowryduda
Copy link
Contributor

comment:12

For what it's worth, this also looks good to me and works on a minimal arch setup and an ubuntu 19.04 setup.

@fchapoton
Copy link
Contributor

comment:13

I am setting to positive, for entry in 9.0.beta0

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@jhpalmieri
Copy link
Member Author

comment:14

I'm wondering about a followup ticket, but I don't know if this would be a good idea: currently, the script sage-python23 checks for the value of SAGE_PYTHON3 to decide which version of Python to run, and the same is true when running sage --python. Should we (a) actively delete local/bin/python when building with Python 3 (relevant in the case of updating existing installations of Sage), and then (b) have sage-python23 and sage --python run SAGE_LOCAL/bin/python if if exists, SAGE_LOCAL/bin/python3 otherwise? Is it good to not depend on the environment variable SAGE_PYTHON3?

If we ever hope to use a system version of Python, I guess this would be a bad idea.

@vbraun
Copy link
Member

vbraun commented Oct 3, 2019

Changed branch from u/jhpalmieri/no-python2 to fa1482d

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

5 participants