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:  sage8.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:
 sagecython:
from sage.env import SAGE_SRC
 could be modified to avoid this import, for example usingos.getenv('SAGE_SRC')
 sageeval
 sagefixdoctests
 sageipython: particularly important, since it is used to run Sage.
 sagelistpackages
 sagenotebook
 sagepreparse
 sagerst2sws
 sagerst2txt
 sageruncython
 sageruntests
 sagestartuptime.py
 sagesws2rst
Change History (34)
comment:1 Changed 5 years ago by
comment:2 Changed 5 years ago by
 sagenotebook, sagerst2sws, sagerst2txt, sagesws2rst: 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.
 sageenvconfig.in is a false positive: it is a shell script and does no importing of Python modules.
 sagecython is easy to fix:

src/bin/sagecython
diff git a/src/bin/sagecython b/src/bin/sagecython 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 sage3eval
, sage3preparse
, etc., and call those from the main sage
script. (The model that I propose at #23119 is that there is a toplevel 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 $0eval
, $0preparse
, etc.)
comment:4 Changed 5 years ago by
I made #23653 for sagecython
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 sagepython23
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
sagenotebook sagerst2sws sagerst2txt sagesws2rst
?
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 followup: ↓ 13 Changed 5 years ago by
I think Volker once said that one should rather always let python point to python2.
comment:12 followup: ↓ 14 Changed 5 years ago by
I view sagepython23
as a buildtime 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 sagepython23
everywhere. I tried to explain this in comment:2.
comment:13 in reply to: ↑ 11 ; followup: ↓ 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
sagepython23
as a buildtime 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 usingsagepython23
everywhere. I tried to explain this in comment:2.
That's a fair point. I think you're rightbetter it just be /usr/bin/env python
. I don't think it makes sense to have sidebyside 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 followup: ↓ 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 sidebyside 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 sidebyside. 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 builttime 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 reconfiguring with a different withpython=
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 sage8.1 to sage8.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 sagepython23
. 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 followup: ↓ 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 sagepython23
from runtime pieces (since sagepython23
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 warnlong 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 3just 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
sagepython23
from runtime pieces (sincesagepython23
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 codeI 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