Allow sage to run in the absence of sage-env

[antonio-rojas]

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.

Depends on #25786

CC: @saraedum @isuruf @kiwifb @timokau

Component: distribution

Author: Antonio Rojas

Branch/Commit: u/arojas/allow_sage_to_run_in_the_absence_of_sage_env @ 83a6cec

Reviewer: François Bissey

Issue created by migration from https://trac.sagemath.org/ticket/28225

[antonio-rojas]

Branch: u/arojas/allow_sage_to_run_in_the_absence_of_sage_env

[sagetrac-git]

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

dda6986

Allow sage to run if sage-env is not present

[sagetrac-git]

Commit: dda6986

[antonio-rojas]

Dependencies: #25786

[antonio-rojas]

Author: Antonio Rojas

[kiwifb]

comment:5

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.

[antonio-rojas]

comment:6

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

[kiwifb]

comment:7

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.

[sagetrac-git]

Changed commit from dda6986 to 6d049b1

[sagetrac-git]

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

e59156b	Fix sage-cython when sage-env is not available
a52972e	export DOT_SAGE so it can be used in spawned scripts
6d049b1	Fix several scripts to work without sage-env

[sagetrac-git]

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

[sagetrac-git]

Changed commit from 6d049b1 to 2dacae2

[antonio-rojas]

comment:10

Replying to @kiwifb:

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)

[kiwifb]

comment:11

Replying to @antonio-rojas:

Replying to @kiwifb:

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?

[antonio-rojas]

comment:12

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.

[kiwifb]

comment:13

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.

[kiwifb]

comment:14

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.

[kiwifb]

comment:15

Replying to @kiwifb:

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.

[kiwifb]

Changed branch 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

[kiwifb]

Changed commit from 2dacae2 to bbaf22f

[kiwifb]

comment:16

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.

[kiwifb]

comment:17

It may be impossible to completely fix the sage script at this stage. Best efforts at improvement would be acceptable to me.

[antonio-rojas]

Changed branch 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

[antonio-rojas]

Changed commit from bbaf22f to 25c686b

[antonio-rojas]

comment:19

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

---

New commits:

2c31b26	Replace SAGE_LOCAL usage in sage executable
25c686b	Skip uncompiled tarball check if sage-env is not present

[antonio-rojas]

comment:24

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

[kiwifb]

comment:25

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-prod/blob/master/src/sage/misc/copying.py) but I am wondering if you do anything about it.

[antonio-rojas]

comment:26

Replying to @kiwifb:

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-prod/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.

[kiwifb]

comment:27

Replying to @antonio-rojas:

Replying to @kiwifb:

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-prod/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.

[sagetrac-git]

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

[sagetrac-git]

Changed commit from 045e01c to c960520

[antonio-rojas]

comment:29

Rebased on top of beta 4

[kiwifb]

Reviewer: François Bissey

[kiwifb]

comment:30

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?

[kiwifb]

comment:31

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.

[kiwifb]

comment:32

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?

[sagetrac-git]

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

[sagetrac-git]

Changed commit from c960520 to 83a6cec

[kiwifb]

comment:34

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

[jhpalmieri]

comment:35

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?

[kiwifb]

comment:36

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.

[vbraun]

comment:37

c960520 was merged and is in 8.9.beta5, go and make a new ticket...

[jhpalmieri]

comment:38

Replying to @kiwifb:

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?

[antonio-rojas]

comment:39

Filed #28320

[mkoeppe]

comment:40

Follow up: #29022

[mkoeppe]

comment:41

and #25486