#28225 closed enhancement (fixed)
Allow sage to run in the absence of sage-env
Reported by: | arojas | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.9 |
Component: | distribution | Keywords: | |
Cc: | saraedum, isuruf, fbissey, gh-timokau | 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 sage-env 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 set to u/arojas/allow_sage_to_run_in_the_absence_of_sage_env
comment:2 Changed 3 years ago by
- Commit set to dda6986e1a3b4c3d06150475d75a207c1e133394
comment:3 Changed 3 years ago by
- Cc saraedum isuruf fbissey gh-timokau added
- Component changed from PLEASE CHANGE to distribution
- Dependencies set to #25786
- Description modified (diff)
comment:4 Changed 3 years ago by
- Type changed from PLEASE CHANGE to enhancement
comment:5 follow-up: ↓ 10 Changed 3 years ago by
I have been sage-env
free for a few releases now in sage-on-gentoo. I ship my own sage script now, but the change I see in the commit are good. I think you should also fix sage-cython
https://github.com/cschwan/sage-on-gentoo/blob/master/sci-mathematics/sage/files/sage-8.1-sage-cython.patch which won't work with stuff from the environment. I also have this patch for various scripts https://github.com/cschwan/sage-on-gentoo/blob/master/sci-mathematics/sage/files/sage-bin-8.7.patch
I also have
# Replace SAGE_EXTCODE with the installation location sed -i "s:\$SAGE_EXTCODE:${EPREFIX}/usr/share/sage/ext:g" \ bin/sage-ipynb2rst \ bin/sage-valgrind
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 sage-cython
.
comment:8 Changed 3 years ago by
- Commit changed from dda6986e1a3b4c3d06150475d75a207c1e133394 to 6d049b14a3a1b77033fa09687a3942fe66f1384e
comment:9 Changed 3 years ago by
- Commit changed from 6d049b14a3a1b77033fa09687a3942fe66f1384e to 2dacae22b2e2da5bbfd89a3ecaec08c2f17f31f9
Branch pushed to git repo; I updated commit sha1. New commits:
2dacae2 | Fix sage-ipynb2rst and sage-valgrind scripts when sage-env is not available
|
comment:10 in reply to: ↑ 5 ; follow-up: ↓ 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/sage-ipynb2rst \ bin/sage-valgrindwhich 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 in reply to: ↑ 10 ; follow-up: ↓ 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/sage-ipynb2rst \ bin/sage-valgrindwhich 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 in reply to: ↑ 11 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 follow-up: ↓ 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 in reply to: ↑ 13 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 changed from u/arojas/allow_sage_to_run_in_the_absence_of_sage_env to u/fbissey/allow_sage_to_run_in_the_absence_of_sage_env
- Commit changed from 2dacae22b2e2da5bbfd89a3ecaec08c2f17f31f9 to 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 sage-env.
|
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 changed from u/fbissey/allow_sage_to_run_in_the_absence_of_sage_env to u/arojas/allow_sage_to_run_in_the_absence_of_sage_env
comment:19 Changed 3 years ago by
- Commit changed from bbaf22f491ab6d334885ba4762256278d18a1bff to 25c686bc4c5696eb63a5ba1fd44f2529749cae8d
comment:20 Changed 3 years ago by
I don't think there is anything to add. Time for review?
comment:21 Changed 3 years ago by
- Status changed from new to needs_review
I am putting in "needs review" to start the patchbots (I hope).
comment:22 Changed 3 years ago by
- Status changed from needs_review to needs_work
Some tests are failing locally without sage-env
sage -t --long /usr/lib/python2.7/site-packages/sage/misc/sagedoc.py # 1 doctest failed sage -t --long /usr/lib/python2.7/site-packages/sage/misc/sphinxify.py # 4 doctests failed sage -t --long /usr/lib/python2.7/site-packages/sage/libs/singular/function.pyx # 1 doctest failed sage -t --long /usr/lib/python2.7/site-packages/sage/interfaces/singular.py # 5 doctests failed
comment:23 Changed 3 years ago by
- Commit changed from 25c686bc4c5696eb63a5ba1fd44f2529749cae8d to 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 follow-up: ↓ 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 sage-on-gentoo. 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 in reply to: ↑ 25 ; follow-up: ↓ 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 in reply to: ↑ 26 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 sage-on-gentoo.
This ticket is dealing with a good number of loose ends, it is quite nice.
comment:28 Changed 3 years ago by
- Commit changed from 045e01c75afdc50562bf3335c9c9977defec0d0d to 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:29 Changed 3 years ago by
- Status changed from needs_work to needs_review
Rebased on top of beta 4
comment:30 Changed 3 years ago by
- Reviewers set to François Bissey
- Status changed from needs_review to 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 merge-tested 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 changed from c9605200e2b5a1cadefb868fd28ef13ef2bef8c3 to 83a6cec200e7179a632daee8658ac95b561d92ac
- Status changed from positive_review to 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 --warn-long 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/sage-8.9.beta5/src/bin/sage-cython", line 12, in <module> from sage.env import SAGE_SRC ImportError: No module named sage.env <BLANKLINE>
The reason is that the sage-cython
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 sage-cython
is run?
comment:36 follow-up: ↓ 38 Changed 3 years ago by
I think sage-cython
should use the python sage uses otherwise, what's the point? While I don't like it, I am guessing it should use sage-python23
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 set to fixed
- Status changed from needs_review to closed
c9605200e2b5a1cadefb868fd28ef13ef2bef8c3 was merged and is in 8.9.beta5, go and make a new ticket...
comment:38 in reply to: ↑ 36 Changed 3 years ago by
Replying to fbissey:
I think
sage-cython
should use the python sage uses otherwise, what's the point? While I don't like it, I am guessing it should usesage-python23
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"
plus some explanatory comments. 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?
comment:39 Changed 3 years ago by
Filed #28320
comment:40 Changed 3 years ago by
Follow up: #29022
comment:41 Changed 3 years ago by
and #25486
Branch pushed to git repo; I updated commit sha1. New commits:
Allow sage to run if sage-env is not present