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: |
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
Cc: | gh-tobiasdiez added |
---|
comment:2 Changed 2 years ago by
Cc: | dimpase added |
---|
comment:3 Changed 2 years ago by
Branch: | → public/build/sharedSystemPython |
---|---|
Commit: | → 9f15544b8ddb9ca2770c6eec88ab80180672ce7e |
Owner: | set to mkoeppe |
comment:4 Changed 2 years ago by
Authors: | → Tobias Diez, ... |
---|
comment:5 Changed 2 years ago by
Commit: | 9f15544b8ddb9ca2770c6eec88ab80180672ce7e → 7cb43a574c3869e27824b9b8202310910831ad44 |
---|
comment:6 Changed 2 years ago by
Authors: | Tobias Diez, ... → Tobias Diez |
---|---|
Status: | new → needs_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
Commit: | 7cb43a574c3869e27824b9b8202310910831ad44 → df2822c5c130811bcba1d7e65743b100444083dd |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
df2822c | Update documentation regarding use of python3 vs. sage-bootstrap-python
|
comment:8 Changed 2 years ago by
Authors: | Tobias Diez → Tobias Diez, Matthias Koeppe |
---|
I've revised the documentation a bit.
comment:10 Changed 2 years ago by
Cc: | jhpalmieri added |
---|
comment:11 Changed 2 years ago by
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:13 Changed 2 years ago by
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
The minimal build requirements listed there are regarding sage-bootstrap-python
, which is unrelated to this ticket.
comment:16 Changed 2 years ago by
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:18 Changed 2 years ago by
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
Reviewers: | Matthias Koeppe, Tobias Diez, ... → Matthias Koeppe, Tobias Diez, Dima Pasechnik |
---|---|
Status: | needs_review → positive_review |
lgtm
comment:21 Changed 2 years ago by
Branch: | public/build/sharedSystemPython → df2822c5c130811bcba1d7e65743b100444083dd |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
https://trac.sagemath.org/ticket/30899#comment:55 explains why replacing it with plain
python3
is correct.