Opened 13 months ago

Closed 9 months 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:

Status badges

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 11 months ago by mkoeppe

  • Cc gh-tobiasdiez added

comment:2 Changed 11 months 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 11 months ago by gh-tobiasdiez

  • Branch set to public/build/sharedSystemPython
  • Commit set to 9f15544b8ddb9ca2770c6eec88ab80180672ce7e
  • Owner changed from (none) to mkoeppe

New commits:

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

comment:4 Changed 11 months ago by gh-tobiasdiez

  • Authors set to Tobias Diez, ...

comment:5 Changed 10 months ago by git

  • Commit changed from 9f15544b8ddb9ca2770c6eec88ab80180672ce7e to 7cb43a574c3869e27824b9b8202310910831ad44

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 10 months ago by gh-tobiasdiez

  • Authors changed from Tobias Diez, ... to Tobias Diez
  • Status changed from new to needs_review

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

comment:7 Changed 10 months ago by git

  • Commit changed from 7cb43a574c3869e27824b9b8202310910831ad44 to df2822c5c130811bcba1d7e65743b100444083dd

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

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

comment:8 Changed 10 months ago by mkoeppe

  • Authors changed from Tobias Diez to Tobias Diez, Matthias Koeppe

I've revised the documentation a bit.

comment:9 Changed 10 months ago by gh-tobiasdiez

  • Reviewers set to Tobias Diez, ...

Thanks, LGTM!

comment:10 Changed 10 months ago by mkoeppe

  • Cc jhpalmieri added

comment:11 Changed 10 months ago by mkoeppe

  • Reviewers changed from Tobias Diez, ... to 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 9 months ago by gh-tobiasdiez

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

comment:13 Changed 9 months 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 9 months ago by mkoeppe

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

comment:15 Changed 9 months ago by mkoeppe

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

comment:16 Changed 9 months 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 9 months ago by gh-tobiasdiez

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

comment:18 Changed 9 months 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 9 months ago by dimpase

  • Reviewers changed from Matthias Koeppe, Tobias Diez, ... to Matthias Koeppe, Tobias Diez, Dima Pasechnik
  • Status changed from needs_review to positive_review

lgtm

comment:20 Changed 9 months ago by gh-tobiasdiez

Thanks!

comment:21 Changed 9 months ago by vbraun

  • Branch changed from public/build/sharedSystemPython to df2822c5c130811bcba1d7e65743b100444083dd
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.