Opened 2 years ago

Closed 2 years ago

#30731 closed enhancement (fixed)

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

Reported by: mkoeppe Owned by: mkoeppe
Priority: minor Milestone: sage-9.3
Component: build Keywords:
Cc: gh-tobiasdiez, dimpase, jhpalmieri Merged in:
Authors: Tobias Diez, Matthias Koeppe Reviewers: Matthias Koeppe, Tobias Diez, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: df2822c (Commits, GitHub, GitLab) Commit: df2822c5c130811bcba1d7e65743b100444083dd
Dependencies: Stopgaps:

GitHub link to the corresponding issue

Description

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

This simplification will help with implementing #29013.

Change History (21)

comment:1 Changed 2 years ago by mkoeppe

Cc: gh-tobiasdiez added

comment:2 Changed 2 years ago by mkoeppe

Cc: dimpase added

https://trac.sagemath.org/ticket/30899#comment:55 explains why replacing it with plain python3 is correct.

comment:3 Changed 2 years ago by gh-tobiasdiez

Branch: public/build/sharedSystemPython
Commit: 9f15544b8ddb9ca2770c6eec88ab80180672ce7e
Owner: set to mkoeppe

New commits:

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

comment:4 Changed 2 years ago by gh-tobiasdiez

Authors: Tobias Diez, ...

comment:5 Changed 2 years ago by git

Commit: 9f15544b8ddb9ca2770c6eec88ab80180672ce7e7cb43a574c3869e27824b9b8202310910831ad44

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

comment:6 Changed 2 years ago by gh-tobiasdiez

Authors: Tobias Diez, ...Tobias Diez
Status: newneeds_review

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

comment:7 Changed 2 years ago by git

Commit: 7cb43a574c3869e27824b9b8202310910831ad44df2822c5c130811bcba1d7e65743b100444083dd

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

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

comment:8 Changed 2 years ago by mkoeppe

Authors: Tobias DiezTobias Diez, Matthias Koeppe

I've revised the documentation a bit.

comment:9 Changed 2 years ago by gh-tobiasdiez

Reviewers: Tobias Diez, ...

Thanks, LGTM!

comment:10 Changed 2 years ago by mkoeppe

Cc: jhpalmieri added

comment:11 Changed 2 years ago by mkoeppe

Reviewers: Tobias Diez, ...Matthias Koeppe, Tobias Diez, ...

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

comment:12 Changed 2 years ago by gh-tobiasdiez

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

comment:13 Changed 2 years ago by jhpalmieri

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

comment:14 Changed 2 years ago by mkoeppe

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

comment:15 Changed 2 years ago by mkoeppe

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

comment:16 Changed 2 years ago by jhpalmieri

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?

comment:17 Changed 2 years ago by gh-tobiasdiez

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

comment:18 Changed 2 years ago by jhpalmieri

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.

comment:19 Changed 2 years ago by dimpase

Reviewers: Matthias Koeppe, Tobias Diez, ...Matthias Koeppe, Tobias Diez, Dima Pasechnik
Status: needs_reviewpositive_review

lgtm

comment:20 Changed 2 years ago by gh-tobiasdiez

Thanks!

comment:21 Changed 2 years ago by vbraun

Branch: public/build/sharedSystemPythondf2822c5c130811bcba1d7e65743b100444083dd
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.