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

Replace use of build/bin/sage-python23 by just python3 #30731

Closed
mkoeppe opened this issue Oct 5, 2020 · 28 comments
Closed

Replace use of build/bin/sage-python23 by just python3 #30731

mkoeppe opened this issue Oct 5, 2020 · 28 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 5, 2020

After the removal of python2 support, this script always calls $SAGE_LOCAL/bin/python3.

This simplification will help with implementing #29013.

CC: @tobiasdiez @dimpase @jhpalmieri

Component: build

Author: Tobias Diez, Matthias Koeppe

Branch/Commit: df2822c

Reviewer: Matthias Koeppe, Tobias Diez, Dima Pasechnik

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

@mkoeppe mkoeppe added this to the sage-9.3 milestone Oct 5, 2020
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 19, 2020

comment:2

#30899 comment:55 explains why replacing it with plain python3 is correct.

@tobiasdiez
Copy link
Contributor

New commits:

218fe09Use system python for generation of conway_polynomials and elliptic_curves
9f15544Use python3 instead of system-python

@tobiasdiez
Copy link
Contributor

Branch: public/build/sharedSystemPython

@tobiasdiez
Copy link
Contributor

Commit: 9f15544

@tobiasdiez
Copy link
Contributor

Author: Tobias Diez, ...

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 18, 2020

Changed commit from 9f15544 to 7cb43a5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 18, 2020

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

f7323eeMerge branch 'develop' of git://github.com/sagemath/sage into public/build/sharedSystemPython
7cb43a5Remove other instances of sage-python23

@tobiasdiez
Copy link
Contributor

Changed author from Tobias Diez, ... to Tobias Diez

@tobiasdiez
Copy link
Contributor

comment:6

I've now replaced all instances of sage-python23, and deleted the script itself. Ready for review.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 19, 2020

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

df2822cUpdate documentation regarding use of python3 vs. sage-bootstrap-python

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 19, 2020

Changed commit from 7cb43a5 to df2822c

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 19, 2020

Changed author from Tobias Diez to Tobias Diez, Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 19, 2020

comment:8

I've revised the documentation a bit.

@tobiasdiez
Copy link
Contributor

Reviewer: Tobias Diez, ...

@tobiasdiez
Copy link
Contributor

comment:9

Thanks, LGTM!

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 21, 2020

comment:11

This works well for me, but as this ticket removes the safety check that would stop a python package that forgot to include $(PYTHON) in dependencies from picking up the wrong python3 from somewhere else in the path, other developers should take a look if this is acceptable to them

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 21, 2020

Changed reviewer from Tobias Diez, ... to Matthias Koeppe, Tobias Diez, ...

@tobiasdiez
Copy link
Contributor

comment:12

I would appreciate if this ticket could be reviewed. Thanks!

@jhpalmieri
Copy link
Member

comment:13

In the installation guide, do we need to remove the option to use Python 2.7? Or is that not related to this ticket?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 6, 2021

comment:14

The minimal build requirements listed there are regarding sage-bootstrap-python, which is unrelated to this ticket.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 6, 2021

comment:15

Update of this line of the installation guide now in #31192.

@jhpalmieri
Copy link
Member

comment:16

In principle it looks fine to me. How widely has it been tested?

  • with Sage's Python 3?
  • on systems with Python 3 (linux, OS X, OS X + homebrew)?
  • on systems with only Python 2? (Should not be an issue, but should be tested anyway.)
  • are there other configurations that need testing?

@tobiasdiez
Copy link
Contributor

comment:17

I've tested it on Ubunut 20.10 (via WSL) and python 3 system.

@jhpalmieri
Copy link
Member

comment:18

Unfortunately I am currently unable to build Sage on my OS X Catalina machine using homebrew's Python, and this makes it hard for me to test this. As I said before, in principle it looks fine, so if others have tested it on a variety of platforms, I would be happy with a positive review.

@dimpase
Copy link
Member

dimpase commented Jan 15, 2021

comment:19

lgtm

@dimpase
Copy link
Member

dimpase commented Jan 15, 2021

Changed reviewer from Matthias Koeppe, Tobias Diez, ... to Matthias Koeppe, Tobias Diez, Dima Pasechnik

@tobiasdiez
Copy link
Contributor

comment:20

Thanks!

@vbraun
Copy link
Member

vbraun commented Jan 24, 2021

Changed branch from public/build/sharedSystemPython to df2822c

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