Opened 5 years ago
Closed 5 years ago
#23106 closed task (fixed)
Python scripts in src/bin are not ready for Sage + Python 3
Reported by: | jhpalmieri | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.2 |
Component: | python3 | Keywords: | |
Cc: | jdemeyer, embray | Merged in: | |
Authors: | Frédéric Chapoton | Reviewers: | Erik Bray |
Report Upstream: | N/A | Work issues: | |
Branch: | 847df2c (Commits, GitHub, GitLab) | Commit: | 847df2c99cbce5f35f4c7c15da1ecb26843f5324 |
Dependencies: | Stopgaps: |
Description
Many of the scripts in src/bin
start with #!/usr/bin/env python
(which is Python 2) and then try to import from the Sage library. This will not work if Sage has been installed using SAGE_PYTHON3=yes
.
These scripts need modification, and perhaps there are others:
- sage-cython:
from sage.env import SAGE_SRC
-- could be modified to avoid this import, for example usingos.getenv('SAGE_SRC')
- sage-eval
- sage-fixdoctests
- sage-ipython: particularly important, since it is used to run Sage.
- sage-list-packages
- sage-notebook
- sage-preparse
- sage-rst2sws
- sage-rst2txt
- sage-run-cython
- sage-runtests
- sage-startuptime.py
- sage-sws2rst
Change History (34)
comment:1 Changed 5 years ago by
comment:2 Changed 5 years ago by
- sage-notebook, sage-rst2sws, sage-rst2txt, sage-sws2rst: I would say that we don't touch these files. They all use SageNB, which is not (yet?) compatible with Python 3. SageNB is also slowly being phased out, which makes these a lower priority anyway.
- sage-env-config.in is a false positive: it is a shell script and does no importing of Python modules.
- sage-cython is easy to fix:
-
src/bin/sage-cython
diff --git a/src/bin/sage-cython b/src/bin/sage-cython index 77ba303b39..ffff293ca2 100755
a b 3 3 import os, sys 4 4 args = sys.argv[1:] 5 5 6 from sage.env import SAGE_SRC 6 if "SAGE_SRC" not in os.environ: 7 raise RuntimeError("The environment variable SAGE_SRC must be set.") 8 SAGE_SRC = os.environ["SAGE_SRC"] 7 9 8 10 # args can have length 0, in case we're printing a usage message (see trac 12207) 9 11 pyx_file = os.path.abspath(args[-1]) if len(args) else None
-
The others may depend on how we approach the Sage + Python 3 runtime question. If we follow the model at #23119, we can create scripts sage3-eval
, sage3-preparse
, etc., and call those from the main sage
script. (The model that I propose at #23119 is that there is a top-level script called "sage", as now, plus a symlink to it called "sage3". They essentially call src/bin/$0
, which translates to src/bin/sage
or src/bin/sage3
, one of which is a symlink of the other. Then that script can call $0-eval
, $0-preparse
, etc.)
comment:4 Changed 5 years ago by
I made #23653 for sage-cython
comment:5 Changed 5 years ago by
- Cc jdemeyer embray added
comment:6 Changed 5 years ago by
Most of these just need to be fixed to use #!/usr/bin/env sage-python23
no?
comment:7 Changed 5 years ago by
- Branch set to public/23106
- Commit set to 5eb29bf3f711fe6593b89a04e0efc4dab56fd49f
I had the same idea, but was rather not sure of its validity. Here is a branch doing that.
New commits:
5eb29bf | modify scripts towards python3 compatibility
|
comment:8 Changed 5 years ago by
- Status changed from new to needs_review
This seems to work smoothly. What do you think ?
comment:9 Changed 5 years ago by
Should we do the same for the remaining 4
sage-notebook sage-rst2sws sage-rst2txt sage-sws2rst
?
comment:10 Changed 5 years ago by
naive question: why is python
pointing to python2
if Sage has been built with SAGE_PYTHON3=yes
!?
comment:11 follow-up: ↓ 13 Changed 5 years ago by
I think Volker once said that one should rather always let python point to python2.
comment:12 follow-up: ↓ 14 Changed 5 years ago by
I view sage-python23
as a build-time tool. Actually, I view the setting of SAGE_PYTHON3
as something which only affects the Sage build, not necessarily how things are run. The Sage community has not made any decisions about how to actually run Sage using Python 3. As a result, I am somewhat wary of just using sage-python23
everywhere. I tried to explain this in comment:2.
comment:13 in reply to: ↑ 11 ; follow-up: ↓ 15 Changed 5 years ago by
Replying to chapoton:
I think Volker once said that one should rather always let python point to python2.
I don't think that makes any sense. It won't be *that* long before Python 2 is deprecated and removed entirely. python
should just be whatever the default python is.
comment:14 in reply to: ↑ 12 Changed 5 years ago by
Replying to jhpalmieri:
I view
sage-python23
as a build-time tool. Actually, I view the setting ofSAGE_PYTHON3
as something which only affects the Sage build, not necessarily how things are run. The Sage community has not made any decisions about how to actually run Sage using Python 3. As a result, I am somewhat wary of just usingsage-python23
everywhere. I tried to explain this in comment:2.
That's a fair point. I think you're right--better it just be /usr/bin/env python
. I don't think it makes sense to have side-by-side Python 2 and Python 3 installations of Sage, or the ability to switch between on the fly. That may be slightly convenient for working on Python 3 compatibility development, but that only affects a few people who can probably afford to have two separate builds for those purposes. One thing I am working with "abstract package" support is a better way (better than setting SAGE_PYTHON3=yes
) to select the default Python at ./configure
time, but changing that selection still requires a rebuild of anything that depends on Python.
comment:15 in reply to: ↑ 13 Changed 5 years ago by
Replying to embray:
Replying to chapoton:
I think Volker once said that one should rather always let python point to python2.
I don't think that makes any sense. It won't be *that* long before Python 2 is deprecated and removed entirely.
python
should just be whatever the default python is.
To clarify, it's not just that Volker once said it, it's PEP 394.
comment:16 Changed 5 years ago by
Boy, that PEP is a mess. But I agree it makes sense, for shebang lines at least, that python2
or python3
is always specified....
comment:17 follow-up: ↓ 18 Changed 5 years ago by
This would now start to be really very useful (as sage starts with python3 using the experimental branch).
comment:18 in reply to: ↑ 17 Changed 5 years ago by
Replying to chapoton:
This would now start to be really very useful (as sage starts with python3 using the experimental branch).
I think we need to make a design decision before we can make progress on this ticket. Once you set SAGE_PYTHON3=yes
(or set the appropriate configure flag, some time in the future), does that forever "pollute" the Sage scripts so that they use python3, or do we want to somehow allow python2 and python3 builds to live side by side? If the former, we need to either save the setting so that future incremental builds in the same directory continue to use SAGE_PYTHON3=yes, or we let users shoot themselves in the foot by building with Python 3 and then a few weeks later upgrading matplotlib and forgetting to set the variable, so it tries to use Python 2 and something goes horribly wrong.
There are a few Sage packages which depend on Python and build libraries which do not reside in local/lib/python$VERSION/
, and those present obstacles to side-by-side builds.
comment:19 Changed 5 years ago by
From a development perspective there's a convenience to allowing simultaneous Python 2 and Python 3 installations to live side-by-side. But currently this doesn't even work since some libraries, such as Pynac, can't be be installed with Python 2 and Python 3 support simultaneously (this could be fixed, for example, by attaching the Python version to the SONAME of the library, but I don't think that's currently supported).
That said, from a more general user perspective I don't think there's much advantage. I think it will be much, much simpler if SAGE_PYTHON3
is treated solely as a built-time variable that indicates what Python should be used to build and run Sage. And of course it should really be replaced with a ./configure
flag. I think that makes the whole affair much, much simpler.
One thing that should still be possible is re-configuring with a different --with-python=
flag, and rebuilding with that Python. I had some success making that work in my experimental ticket #23465.
comment:20 Changed 5 years ago by
- Milestone changed from sage-8.1 to sage-8.2
- Priority changed from critical to major
comment:21 Changed 5 years ago by
I've fixed quite a number of these in my branch just by changing the shebang lines to use sage-python23
. There might be some cases where this isn't appropriate, but I've found it to b the right way to go in most cases.
comment:22 Changed 5 years ago by
Certainly the fixes in the branch attached to this ticket are a good start. I think we should go ahead and merge it, and leave bigger questions for a separate ticket.
comment:23 Changed 5 years ago by
so, positive review ?
comment:24 Changed 5 years ago by
- Reviewers set to Erik Bray
- Status changed from needs_review to positive_review
For me, yeah. I just wasn't sure if there were other objections.
comment:25 follow-up: ↓ 33 Changed 5 years ago by
To the extent that we want to separate the build process from the runtime process, this should be a temporary solution, right? We either shouldn't be calling sage-python23
from runtime pieces (since sage-python23
lives in build/bin
) or we should move it or have a new script in src/bin
which does something similar.
comment:26 Changed 5 years ago by
- Status changed from positive_review to needs_work
sage -t --long --warn-long 78.8 src/sage/misc/gperftools.py ********************************************************************** File "src/sage/misc/gperftools.py", line 244, in sage.misc.gperftools.Profiler._executable Failed example: prof._executable() Expected: '.../python' Got: '/mnt/disk/home/release/Sage/local/bin/python2' ********************************************************************** 1 item had failures: 1 of 4 in sage.misc.gperftools.Profiler._executable [36 tests, 1 failure, 0.94 s]
comment:27 Changed 5 years ago by
- Commit changed from 5eb29bf3f711fe6593b89a04e0efc4dab56fd49f to 25dddc53d545ad2400023000d62d33bb990879aa
comment:28 Changed 5 years ago by
- Status changed from needs_work to needs_review
comment:29 Changed 5 years ago by
- Status changed from needs_review to needs_work
That fix won't work on Python 3--just put .../python...
(and consider squashing)
comment:30 Changed 5 years ago by
- Commit changed from 25dddc53d545ad2400023000d62d33bb990879aa to 847df2c99cbce5f35f4c7c15da1ecb26843f5324
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
847df2c | modify scripts towards python3 compatibility
|
comment:31 Changed 5 years ago by
comment:33 in reply to: ↑ 25 Changed 5 years ago by
Replying to jhpalmieri:
To the extent that we want to separate the build process from the runtime process, this should be a temporary solution, right? We either shouldn't be calling
sage-python23
from runtime pieces (sincesage-python23
lives inbuild/bin
) or we should move it or have a new script insrc/bin
which does something similar.
I don't particularly care where the scripts live in the source code--I think that scripts are needed at build time living in src/bin
makes sense, but don't have strong opinions about it.
All that matters to me is what happens at installation. I think all scripts in build/bin
should be installed to $SAGE_LOCAL/bin
. Similarly I'm of the opinion that most other files in build/
should be installed somewhere under $SAGE_LOCAL
so that, for example, one can install new packages for Sage at runtime without having a full sage source tree lying around. That is, package installations and upgrades fully become a "runtime" feature of Sage. That's all a much bigger topic beyond this though.
comment:34 Changed 5 years ago by
- Branch changed from public/23106 to 847df2c99cbce5f35f4c7c15da1ecb26843f5324
- Resolution set to fixed
- Status changed from positive_review to closed
Indeed,
some of these are in fact
from sagenb
imports