Opened 4 months ago

Closed 4 months ago

Last modified 4 months ago

#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) Commit: 83a6cec200e7179a632daee8658ac95b561d92ac
Dependencies: #25786 Stopgaps:

Description (last modified by arojas)

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 (39)

comment:1 Changed 4 months ago by arojas

  • Branch set to u/arojas/allow_sage_to_run_in_the_absence_of_sage_env

comment:2 Changed 4 months ago by git

  • Commit set to dda6986e1a3b4c3d06150475d75a207c1e133394

Branch pushed to git repo; I updated commit sha1. New commits:

dda6986Allow sage to run if sage-env is not present

comment:3 Changed 4 months ago by arojas

  • Authors set to Antonio Rojas
  • Cc saraedum isuruf fbissey gh-timokau added
  • Component changed from PLEASE CHANGE to distribution
  • Dependencies set to #25786
  • Description modified (diff)

comment:4 Changed 4 months ago by arojas

  • Type changed from PLEASE CHANGE to enhancement

comment:5 follow-up: Changed 4 months ago by fbissey

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 4 months ago by arojas

Thanks. Most of these work fine here just because /bin symlinked to /usr/bin, but that certainly shouldn't be assumed

comment:7 Changed 4 months ago by fbissey

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 4 months ago by git

  • Commit changed from dda6986e1a3b4c3d06150475d75a207c1e133394 to 6d049b14a3a1b77033fa09687a3942fe66f1384e

Branch pushed to git repo; I updated commit sha1. New commits:

e59156bFix sage-cython when sage-env is not available
a52972eexport DOT_SAGE so it can be used in spawned scripts
6d049b1Fix several scripts to work without sage-env

comment:9 Changed 4 months ago by git

  • Commit changed from 6d049b14a3a1b77033fa09687a3942fe66f1384e to 2dacae22b2e2da5bbfd89a3ecaec08c2f17f31f9

Branch pushed to git repo; I updated commit sha1. New commits:

2dacae2Fix sage-ipynb2rst and sage-valgrind scripts when sage-env is not available

comment:10 in reply to: ↑ 5 ; follow-up: Changed 4 months ago by 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-valgrind

which 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: Changed 4 months ago by fbissey

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-valgrind

which 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 4 months ago by arojas

Certainly does look hackish. I would have never thought of using print that way. Does this really work?

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: Changed 4 months ago by 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 import SAGE_LOCAL from env.py instead and insert that value.

comment:14 Changed 4 months ago by fbissey

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 4 months ago by fbissey

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 import SAGE_LOCAL from env.py instead and insert that value.

After line 1078 we also have $SAGE_EXTCODE in lines 1099 to 1101.

comment:16 Changed 4 months ago by fbissey

  • 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:

bbaf22fFix control.py so it doesn't depend on values from sage-env.

comment:17 Changed 4 months ago by fbissey

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 4 months ago by arojas

  • 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 4 months ago by arojas

  • Commit changed from bbaf22f491ab6d334885ba4762256278d18a1bff to 25c686bc4c5696eb63a5ba1fd44f2529749cae8d

I think all SAGE_LOCAL usage in the sage executable part that is relevant for !sage-the-distro is gone now


New commits:

2c31b26Replace SAGE_LOCAL usage in sage executable
25c686bSkip uncompiled tarball check if sage-env is not present

comment:20 Changed 4 months ago by fbissey

I don't think there is anything to add. Time for review?

comment:21 Changed 4 months ago by fbissey

  • Status changed from new to needs_review

I am putting in "needs review" to start the patchbots (I hope).

comment:22 Changed 4 months ago by arojas

  • 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 4 months ago by git

  • Commit changed from 25c686bc4c5696eb63a5ba1fd44f2529749cae8d to 045e01c75afdc50562bf3335c9c9977defec0d0d

Branch pushed to git repo; I updated commit sha1. New commits:

c86bd9dSearch for custom html themes also in SAGE_DOC, so they are found at runtime if SAGE_DOC_SRC is not set
045e01cDefine SINGULARPATH in env.py so it is available at runtime when sage-env is not imported

comment:24 Changed 4 months ago by arojas

All tests pass now. This branch conflicts with #25786 so it will need rebasing once that one goes in.

comment:25 follow-up: Changed 4 months ago by fbissey

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: Changed 4 months ago by 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.

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 4 months ago by fbissey

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 4 months ago by git

  • Commit changed from 045e01c75afdc50562bf3335c9c9977defec0d0d to c9605200e2b5a1cadefb868fd28ef13ef2bef8c3

Branch pushed to git repo; I updated commit sha1. New commits:

c960520Merge branch 'develop' of git://trac.sagemath.org/sage into t/28225/allow_sage_to_run_in_the_absence_of_sage_env

comment:29 Changed 4 months ago by arojas

  • Status changed from needs_work to needs_review

Rebased on top of beta 4

comment:30 Changed 4 months ago by fbissey

  • 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 4 months ago by fbissey

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 4 months ago by fbissey

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 4 months ago by git

  • 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:

83a6cecSet 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 4 months ago by fbissey

I should have said to wait for a follow up ticket. Nevermind, let's see how it plays out.

comment:35 Changed 4 months ago by jhpalmieri

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: Changed 4 months ago by 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 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 4 months ago by vbraun

  • 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 4 months ago by jhpalmieri

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 use sage-python23 instead of plain python. 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?

Last edited 4 months ago by jhpalmieri (previous) (diff)

comment:39 Changed 4 months ago by arojas

Filed #28320

Note: See TracTickets for help on using tickets.