#28225 closed enhancement (fixed)
Allow sage to run in the absence of sageenv
Reported by:  Antonio Rojas  Owned by:  

Priority:  major  Milestone:  sage8.9 
Component:  distribution  Keywords:  
Cc:  Julian Rüth, Isuru Fernando, François Bissey, Timo Kaufmann  Merged in:  
Authors:  Antonio Rojas  Reviewers:  François Bissey 
Report Upstream:  N/A  Work issues:  
Branch:  u/arojas/allow_sage_to_run_in_the_absence_of_sage_env (Commits, GitHub, GitLab)  Commit:  83a6cec200e7179a632daee8658ac95b561d92ac 
Dependencies:  #25786  Stopgaps: 
Description (last modified by )
The sageenv script has never been particularly useful in distributions, and has actually been more of a nuisance: it needs to be heavily patched or even replaced with a custom one.
After #25786 (which removes SAGE_DOC_SRC usage at runtime) the only use for it that I can think of is setting DOT_SAGE. This patch allows to completely do away with this script, by setting a fallback for DOT_SAGE in the sage executable.
Change History (41)
comment:1 Changed 3 years ago by
Branch:  → u/arojas/allow_sage_to_run_in_the_absence_of_sage_env 

comment:2 Changed 3 years ago by
Commit:  → dda6986e1a3b4c3d06150475d75a207c1e133394 

comment:3 Changed 3 years ago by
Authors:  → Antonio Rojas 

Cc:  Julian Rüth Isuru Fernando François Bissey Timo Kaufmann added 
Component:  PLEASE CHANGE → distribution 
Dependencies:  → #25786 
Description:  modified (diff) 
comment:4 Changed 3 years ago by
Type:  PLEASE CHANGE → enhancement 

comment:5 followup: 10 Changed 3 years ago by
I have been sageenv
free for a few releases now in sageongentoo. I ship my own sage script now, but the change I see in the commit are good. I think you should also fix sagecython
https://github.com/cschwan/sageongentoo/blob/master/scimathematics/sage/files/sage8.1sagecython.patch which won't work with stuff from the environment. I also have this patch for various scripts https://github.com/cschwan/sageongentoo/blob/master/scimathematics/sage/files/sagebin8.7.patch
I also have
# Replace SAGE_EXTCODE with the installation location sed i "s:\$SAGE_EXTCODE:${EPREFIX}/usr/share/sage/ext:g" \ bin/sageipynb2rst \ bin/sagevalgrind
which may be harder to replace more generally.
I haven't made any fix to runtests, but DOT_SAGE
should definitely be set  for some exotic usage with valgrind and other tools.
comment:6 Changed 3 years ago by
Thanks. Most of these work fine here just because /bin symlinked to /usr/bin, but that certainly shouldn't be assumed
comment:7 Changed 3 years ago by
Yes, I hadn't thought of that scenario at all, where the variable being undefined is actually helpful. But it will break on other system. But that doesn't help with sagecython
.
comment:8 Changed 3 years ago by
Commit:  dda6986e1a3b4c3d06150475d75a207c1e133394 → 6d049b14a3a1b77033fa09687a3942fe66f1384e 

comment:9 Changed 3 years ago by
Commit:  6d049b14a3a1b77033fa09687a3942fe66f1384e → 2dacae22b2e2da5bbfd89a3ecaec08c2f17f31f9 

Branch pushed to git repo; I updated commit sha1. New commits:
2dacae2  Fix sageipynb2rst and sagevalgrind scripts when sageenv is not available

comment:10 followup: 11 Changed 3 years ago by
Replying to fbissey:
I also have
# Replace SAGE_EXTCODE with the installation location sed i "s:\$SAGE_EXTCODE:${EPREFIX}/usr/share/sage/ext:g" \ bin/sageipynb2rst \ bin/sagevalgrindwhich may be harder to replace more generally.
Fixed in the latest commit (in a somewhat hackish way, would be nice if someone could come up with a nicer way to do it)
comment:11 followup: 12 Changed 3 years ago by
Replying to arojas:
Replying to fbissey:
I also have
# Replace SAGE_EXTCODE with the installation location sed i "s:\$SAGE_EXTCODE:${EPREFIX}/usr/share/sage/ext:g" \ bin/sageipynb2rst \ bin/sagevalgrindwhich may be harder to replace more generally.
Fixed in the latest commit (in a somewhat hackish way, would be nice if someone could come up with a nicer way to do it)
Certainly does look hackish. I would have never thought of using print
that way. Does this really work?
comment:12 Changed 3 years ago by
Certainly does look hackish. I would have never thought of using
Sure it does  we use this kind of thing frequently in Arch build scripts to get the python lib dir independently of the version.
comment:13 followup: 15 Changed 3 years ago by
OK, I am reviewing all my patches and stuff for interesting bits. In sage/doctest/control.py
There are several commands build with $SAGE_LOCAL
in a way that requires it to be picked up from the environment (lines 1028, 1078 in code and lines 1026, 1062, 1069 for stuff that could be doctests). We probably want to import SAGE_LOCAL
from env.py
instead and insert that value.
comment:14 Changed 3 years ago by
How do you deal with SAGE_LOCAL
in the sage
script? Because of arch having /usr/bin
and /bin
symlinked you may not catch problems in line 7 and 8. There are a number of calls to SAGE_LOCAL
in that file you are probably missing altogether.
comment:15 Changed 3 years ago by
Replying to fbissey:
OK, I am reviewing all my patches and stuff for interesting bits. In
sage/doctest/control.py
There are several commands build with$SAGE_LOCAL
in a way that requires it to be picked up from the environment (lines 1028, 1078 in code and lines 1026, 1062, 1069 for stuff that could be doctests). We probably want to importSAGE_LOCAL
fromenv.py
instead and insert that value.
After line 1078 we also have $SAGE_EXTCODE
in lines 1099 to 1101.
comment:16 Changed 3 years ago by
Branch:  u/arojas/allow_sage_to_run_in_the_absence_of_sage_env → u/fbissey/allow_sage_to_run_in_the_absence_of_sage_env 

Commit:  2dacae22b2e2da5bbfd89a3ecaec08c2f17f31f9 → bbaf22f491ab6d334885ba4762256278d18a1bff 
Adding my ideas for sage/doctests/control.py
. Feel free to regain control of the branch.
New commits:
bbaf22f  Fix control.py so it doesn't depend on values from sageenv.

comment:17 Changed 3 years ago by
It may be impossible to completely fix the sage
script at this stage. Best efforts at improvement would be acceptable to me.
comment:18 Changed 3 years ago by
Branch:  u/fbissey/allow_sage_to_run_in_the_absence_of_sage_env → u/arojas/allow_sage_to_run_in_the_absence_of_sage_env 

comment:19 Changed 3 years ago by
Commit:  bbaf22f491ab6d334885ba4762256278d18a1bff → 25c686bc4c5696eb63a5ba1fd44f2529749cae8d 

comment:21 Changed 3 years ago by
Status:  new → needs_review 

I am putting in "needs review" to start the patchbots (I hope).
comment:22 Changed 3 years ago by
Status:  needs_review → needs_work 

Some tests are failing locally without sageenv
sage t long /usr/lib/python2.7/sitepackages/sage/misc/sagedoc.py # 1 doctest failed sage t long /usr/lib/python2.7/sitepackages/sage/misc/sphinxify.py # 4 doctests failed sage t long /usr/lib/python2.7/sitepackages/sage/libs/singular/function.pyx # 1 doctest failed sage t long /usr/lib/python2.7/sitepackages/sage/interfaces/singular.py # 5 doctests failed
comment:23 Changed 3 years ago by
Commit:  25c686bc4c5696eb63a5ba1fd44f2529749cae8d → 045e01c75afdc50562bf3335c9c9977defec0d0d 

comment:24 Changed 3 years ago by
All tests pass now. This branch conflicts with #25786 so it will need rebasing once that one goes in.
comment:25 followup: 26 Changed 3 years ago by
I missed the singularpath one because I deal with it in the same patch as I do adjustments to env.py
. I neglected that. I don't have a problem with SAGE_DOC_SRC
because I have it default to SAGE_DOC
not SAGE_SRC/doc
in sageongentoo. I am surprised that's the only place you had to fix. I am expecting anything calling SAGE_DOC_SRC
at runtime to fail if left at that value.
I know there is no doctests in sage/misc/copying.py
(https://github.com/sagemath/sage/blob/master/src/sage/misc/copying.py) but I am wondering if you do anything about it.
comment:26 followup: 27 Changed 3 years ago by
Replying to fbissey:
I am surprised that's the only place you had to fix. I am expecting anything calling
SAGE_DOC_SRC
at runtime to fail if left at that value.
The only remaining usages of SAGE_DOC_SRC in sagelib are in sage/doctest/control.py (which adds it to the list of tested dirs when calling sage t a) and sage/docs/conf.py (where AFAICS all usages besides the one fixed here are for doc build), so I don't expect more failures.
I know there is no doctests in
sage/misc/copying.py
(https://github.com/sagemath/sage/blob/master/src/sage/misc/copying.py) but I am wondering if you do anything about it.
No. COPYING.txt should probably be installed somewhere in SAGE_SHARE. But given that this is currently broken for distros anyway, I'd say it's material for a different ticket.
comment:27 Changed 3 years ago by
Replying to arojas:
Replying to fbissey:
I am surprised that's the only place you had to fix. I am expecting anything calling
SAGE_DOC_SRC
at runtime to fail if left at that value.The only remaining usages of SAGE_DOC_SRC in sagelib are in sage/doctest/control.py (which adds it to the list of tested dirs when calling sage t a) and sage/docs/conf.py (where AFAICS all usages besides the one fixed here are for doc build), so I don't expect more failures.
Cool, that's a clean up that's almost done then.
I know there is no doctests in
sage/misc/copying.py
(https://github.com/sagemath/sage/blob/master/src/sage/misc/copying.py) but I am wondering if you do anything about it.No. COPYING.txt should probably be installed somewhere in SAGE_SHARE. But given that this is currently broken for distros anyway, I'd say it's material for a different ticket.
Agreed. What to do with that file may be dependent on distro policies as well. It belongs to the location I set SAGE_DOC
to, by policy in sageongentoo.
This ticket is dealing with a good number of loose ends, it is quite nice.
comment:28 Changed 3 years ago by
Commit:  045e01c75afdc50562bf3335c9c9977defec0d0d → c9605200e2b5a1cadefb868fd28ef13ef2bef8c3 

Branch pushed to git repo; I updated commit sha1. New commits:
c960520  Merge branch 'develop' of git://trac.sagemath.org/sage into t/28225/allow_sage_to_run_in_the_absence_of_sage_env

comment:30 Changed 3 years ago by
Reviewers:  → François Bissey 

Status:  needs_review → positive_review 
Well, it all looks good to me. Anyone knows why the patchbot doesn't seem to want to pick it? Is it too early after 8.9.beta4?
comment:31 Changed 3 years ago by
This is being mergetested right now. Not runtime but build time, I wish I had spotted some instances of os.environ
in sage_setup/docbuild/__init__.py
.
I am hoping the ticket will be merged in the current state and then we can address those little left overs in a follow up ticket.
comment:32 Changed 3 years ago by
Another kink I missed before because of my setting of SAGE_DOC_SRC
. sage doctest the documentation source not the installed one. I guess it also test the source code by looking at SAGE_SRC which default to SAGE_LIB when SAGE_ROOT is undefined. May be we should have gone for a similar model for SAGE_DOC_SRC  instead of the fix currently in sage/docs/conf.py
?
comment:33 Changed 3 years ago by
Commit:  c9605200e2b5a1cadefb868fd28ef13ef2bef8c3 → 83a6cec200e7179a632daee8658ac95b561d92ac 

Status:  positive_review → needs_review 
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
83a6cec  Set SAGE_DOC as fallback for SAGE_DOC_SRC in env.py, and make sure that the fallback is used if SAGE_ROOT is unset

comment:34 Changed 3 years ago by
I should have said to wait for a follow up ticket. Nevermind, let's see how it plays out.
comment:35 Changed 3 years ago by
Some version of this ticket was merged into 8.9.beta5, and I'm guessing the last commit didn't make it. In any case, the changes that were merged cause an error with Python 3:
sage t warnlong 78.7 src/sage/tests/cmdline.py ********************************************************************** File "src/sage/tests/cmdline.py", line 503, in sage.tests.cmdline.test_executable Failed example: print(err) Expected: Cython (http://cython.org) is a compiler for code written in the Cython language. Cython is based on Pyrex by Greg Ewing. ... Got: Traceback (most recent call last): File "/Users/palmieri/Desktop/Sage_stuff/sage_builds/PYTHON3/sage8.9.beta5/src/bin/sagecython", line 12, in <module> from sage.env import SAGE_SRC ImportError: No module named sage.env <BLANKLINE>
The reason is that the sagecython
script begins
#!/usr/bin/env python
That means that it uses Python 2 by default. With a Python 3 build, Python 2 doesn't know about the sage
module.
I'm not sure what you should do instead. Maybe
try: from sage.env import SAGE_SRC except ImportError: SAGE_SRC = (something involving SAGE_ROOT or SAGE_LIB?)
What variables are guaranteed to be set when sagecython
is run?
comment:36 followup: 38 Changed 3 years ago by
I think sagecython
should use the python sage uses otherwise, what's the point? While I don't like it, I am guessing it should use sagepython23
instead of plain python
. I am open to contrary opinions.
We need a follow up ticket for that last commit that didn't make and a couple of things I spotted in sage_setup
we can throw that in as well.
comment:37 Changed 3 years ago by
Resolution:  → fixed 

Status:  needs_review → closed 
c9605200e2b5a1cadefb868fd28ef13ef2bef8c3 was merged and is in 8.9.beta5, go and make a new ticket...
comment:38 Changed 3 years ago by
Replying to fbissey:
I think
sagecython
should use the python sage uses otherwise, what's the point? While I don't like it, I am guessing it should usesagepython23
instead of plainpython
. I am open to contrary opinions.
The only thing that script does is to check arguments and run cython
, so it should run fine with the system Python. For distro use, do you even care about how SAGE_SRC is used here? Maybe you could have
try: SAGE_SRC = os.environ["SAGE_SRC"] except ImportError: SAGE_SRC = "/does/not/exist"
The whole script is deprecated, so maybe it doesn't matter much what happens to it, but you might cc jdemeyer on the followup ticket.
We need a follow up ticket for that last commit that didn't make and a couple of things I spotted in
sage_setup
we can throw that in as well.
Either one or two: one for your followup issues, maybe a separate one to fix the doctest?
Branch pushed to git repo; I updated commit sha1. New commits:
Allow sage to run if sageenv is not present