Cleanup sage.env

[Snark]

1. sage.all should be importable and mostly usable even if no environment variables like SAGE_ROOT or SAGE_LOCAL are set. This requires a new function join which is like os.path.join except that None values are propagated.

2. Several unneeded variables are removed.

3. Get rid of the over-engineered $VAR replacement stuff.

4. In some places, SAGE_LIB (installed location of Sage) can be used instead of SAGE_SRC.

5. Determine SAGE_LIB not from site-packages but from sage.__file__

6. Rename _add_variable_or_fallback -> var to shorten line lengths.

There is a ​thread on sage-devel that complains about the failure in Debian of import sage.all from system Python.

[fbissey]

SAGE_ROOT is harder. It doesn't correspond to any configuration features of anything. At best it is SAGE_LOCAL/... Any corners in which that wouldn't be true?

[dunfield]

Replying to fbissey:

SAGE_ROOT is harder. It doesn't correspond to any configuration features of anything. At best it is SAGE_LOCAL/... Any corners in which that wouldn't be true?

One place where the SAGE_LOCAL/.. pattern does not hold is the Debian package, where SAGE_LOCAL=/usr but SAGE_ROOT=/usr/share/sagemath. Moreover, on Debian SAGE_SCRIPTS_DIR is $SAGE_ROOT/bin rather than the more typical $SAGE_LOCAL/bin.

[fbissey]

I see. debian choose to do something like that. In sage-on-gentoo SAGE_ROOT points to a location where there used to be something but is now empty. Does debian use SAGE_ROOT anywhere else? Otherwise they could just define SAGE_SCRIPTS_DIR directly.

[dunfield]

Replying to fbissey:

Does debian use SAGE_ROOT anywhere else? Otherwise they could just define SAGE_SCRIPTS_DIR directly.

To clarify, Debian doesn't change anything in env.py related to SAGE_SCRIPTS_DIR; the only fallback there is $SAGE_LOCAL/bin which does not contain the relevant scripts. Instead, in /usr/share/sagemath/bin/sage-env they manually set SAGE_ROOT=/usr/share/sagemath/ and SAGE_SCRIPTS_DIR=/usr/share/sagemath/bin.

On Debian, I tried setting SAGE_ROOT to a garbage value in sage-env, and Sage still started and worked fine near as I can tell.

[fbissey]

I can see it now. In vanilla sage SAGE_SCRIPTS_DIR is only set in sage-env and used to source sage-env-config. But I certainly know that debian has plan for that folder.

So as far as we can see, it would be fine to set SAGE_ROOT in env.py. But it could probably be left as is without impacting anything and doing from sage import all in python should work without setting it.

Looking at it there is one thing I have set SAGE_ROOT for! sage/misc/copying.py tries to access SAGE_ROOT/COPYING.txt. So its current value in sage-on-gentoo is just so that bit works :) I need to change that to be more conformant to Gentoo policies.

There is a somewhat harmless appearance of SAGE_ROOT in sage/misc/citation.pyx. There is a more disturbing one in sage/misc/persist.pyx where I would have used DOT_SAGE to be honest. The rest is mostly doctesting, packaging, doctests themselves and documentation strings.

[fbissey]

Scope extension. That's brave! I notice you mention SAGE_SRC and SAGE_LIB, is it because you spotted I have

_add_variable_or_fallback('SAGE_SRC',        opj('$SAGE_LIB'))

in sage-on-gentoo? I override it with the appropriate value during build of course. But it should work in most places.

[jdemeyer]

Replying to fbissey:

is it because you spotted I have

_add_variable_or_fallback('SAGE_SRC',        opj('$SAGE_LIB'))

in sage-on-gentoo?

Actually no. It's something I figured out independently.

[fbissey]

Will using sage.__file__ change the way we use SAGE_LIB or do you plan to restore the value starting from it?

[jdemeyer]

Replying to fbissey:

Will using sage.__file__ change the way we use SAGE_LIB

No.

SAGE_LIB is supposed to be the installed location of Sage. I guess that by definition, Sage is run from its installed location. So if you know sage.__file__, you know the installed location of Sage.

[fbissey]

Replying to jdemeyer:

Replying to fbissey:

Will using sage.__file__ change the way we use SAGE_LIB

No.

SAGE_LIB is supposed to be the installed location of Sage. I guess that by definition, Sage is run from its installed location. So if you know sage.__file__, you know the installed location of Sage.

I get your argument and won't contest. but it has been used as the site-package location in the code so I am expecting changes, particularly where you exchange SAGE_SRC for SAGE_LIB.

[jdemeyer]

Replying to fbissey:

it has been used as the site-package location in the code so I am expecting changes

If Sage is installed in site-packages (which is almost certainly the case), then the old value and new value for SAGE_LIB will be the same. In this case, there won't be any breakage.

If Sage is not installed in site-packages, then any existing code using SAGE_LIB to find the installed location of Sage is already broken.

Do you know any code which is using SAGE_LIB to find the path of something else besides sage?

[fbissey]

Let's take some code from sage/doctest/control.py

F = x.__file__.replace(SAGE_LIB, SAGE_SRC)

It basically works because in both case you have to add sage to find what you want.

All the current code using SAGE_LIB is based on it mirroring SAGE_SRC by the look of it.

Nevertheless a change may be good. I'll certainly have a good look at what you proposing!

---

New commits:

​4b2dd54

Clean up sage.env

[fbissey]

In env.py in the definition of sage_include_directories

    if use_sources :
        include_directories.extend([SAGE_SRC,
                                    opj(SAGE_SRC, 'sage', 'ext')])
    else:
        include_directories.extend([SAGE_LIB,
                                    opj(SAGE_LIB, 'sage', 'ext')])

since SAGE_LIB should now include sage, this should be changed unless there is something I don't get.

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​b818436

Clean up sage.env

[jdemeyer]

I'm guessing that those top-level include directories like SAGE_SRC and SAGE_LIB are not actually needed. But the patchbot needs to confirm...

[jdemeyer]

That SAGE_SRC in the include directories was introduced by #14544. I'll recheck that bug. But hopefully Cython has become more clever now.

[embray]

+
+def j(*args):
+    """
+    Join paths like ``os.path.join`` except that the result is ``None``
+    if any of the components is ``None``.
+    """
+    if any(a is None for a in args):
+        return None
+    import os
+    return os.path.join(*args)
+
+

What's with the superfluous import os in here? I also don't like that this function is called just j. I know it's convenient to have a short name here, but I'd prefer something a little clearer, like join.

[jdemeyer]

Replying to embray:

I know it's convenient to have a short name here, but I'd prefer something a little clearer, like join.

At least it's not as bad as gettext's convention of using _ :-)

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​f10ae3f

Clean up sage.env

[embray]

Replying to jdemeyer:

Replying to embray:

I know it's convenient to have a short name here, but I'd prefer something a little clearer, like join.

At least it's not as bad as gettext's convention of using _ :-)

Heh, I think there's a decent excuse for that, but it's still definitely questionable.

Thanks anyways. Just glancing over the rest of the diff this looks like a vast improvement. I probably won't have a chance to test it out next week, but I want to try it on Cygwin.

[jdemeyer]

Various doctest failures on the patchbot but only in deprecated functionality.

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​108a42e	Remove deprecated stuff related to Cython
​de3be03	Clean up sage.env

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​3b0a094

Clean up sage.env

[embray]

This is definitely a vast improvement over the old version of this module. If any further changes are needed for any reason we can add them in follow-up tickets (though if maintainers of any particular downstream packages want to chime in with their own comments feel free to set back to needs_work/needs_info).

Changing milestone to 8.7 as I don't think this is urgent: Most packagers already have their own patches to this module, and drastically changing it now is going to break those patches, only making this harder for getting an 8.6 release out.

[jdemeyer]

Note that the dependency #27041 still needs review.

[gh-timokau]

On nix, the test

Verify that Sage can be started without any ``SAGE_`` environment
variables::

    sage: env = {k:v for (k,v) in os.environ.items() if not k.startswith("SAGE_")}
    sage: import subprocess
    sage: cmd = "from sage.all import SAGE_ROOT; print(SAGE_ROOT)"
    sage: res = subprocess.call(["python", "-c", cmd], env=env)  # long time
    None

breaks because maxima can't be found:

Failed example:
    res = subprocess.call(["python", "-c", cmd], env=env)  # long time
Expected:
    None
Got:
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/nix/store/yhjx1nw8fiivfhpi1pg45v5fkawbpzv9-python-2.7.15-env/lib/python2.7/site-packages/sage/all.py", line 83, in <module>
        from sage.misc.all       import *         # takes a while
      File "/nix/store/yhjx1nw8fiivfhpi1pg45v5fkawbpzv9-python-2.7.15-env/lib/python2.7/site-packages/sage/misc/all.py", line 84, in <module>
        from .functional import (additive_order,
      File "/nix/store/yhjx1nw8fiivfhpi1pg45v5fkawbpzv9-python-2.7.15-env/lib/python2.7/site-packages/sage/misc/functional.py", line 27, in <module>
        from sage.rings.complex_double import CDF
      File "sage/rings/complex_double.pyx", line 101, in init sage.rings.complex_double (build/cythonized/sage/rings/complex_double.c:23780)
      File "/nix/store/yhjx1nw8fiivfhpi1pg45v5fkawbpzv9-python-2.7.15-env/lib/python2.7/site-packages/sage/rings/complex_field.py", line 114, in ComplexField
        C = ComplexField_class(prec)
      File "/nix/store/yhjx1nw8fiivfhpi1pg45v5fkawbpzv9-python-2.7.15-env/lib/python2.7/site-packages/sage/rings/complex_field.py", line 208, in __init__
        self._populate_coercion_lists_(coerce_list=[RRtoCC(self._real_field(), self)])
      File "sage/rings/complex_number.pyx", line 2580, in sage.rings.complex_number.RRtoCC.__init__ (build/cythonized/sage/rings/complex_number.c:20548)
      File "sage/categories/map.pyx", line 125, in sage.categories.map.Map.__init__ (build/cythonized/sage/categories/map.c:3621)
      File "/nix/store/yhjx1nw8fiivfhpi1pg45v5fkawbpzv9-python-2.7.15-env/lib/python2.7/site-packages/sage/categories/homset.py", line 395, in Hom
        H = Hom(X, Y, category, check=False)
      File "/nix/store/yhjx1nw8fiivfhpi1pg45v5fkawbpzv9-python-2.7.15-env/lib/python2.7/site-packages/sage/categories/homset.py", line 422, in Hom
        H = X._Hom_(Y, category)
      File "/nix/store/yhjx1nw8fiivfhpi1pg45v5fkawbpzv9-python-2.7.15-env/lib/python2.7/site-packages/sage/categories/rings.py", line 361, in _Hom_
        from sage.rings.homset import RingHomset
      File "/nix/store/yhjx1nw8fiivfhpi1pg45v5fkawbpzv9-python-2.7.15-env/lib/python2.7/site-packages/sage/rings/homset.py", line 19, in <module>
        from . import quotient_ring
      File "/nix/store/yhjx1nw8fiivfhpi1pg45v5fkawbpzv9-python-2.7.15-env/lib/python2.7/site-packages/sage/rings/quotient_ring.py", line 116, in <module>
        import sage.rings.polynomial.multi_polynomial_ideal
      File "/nix/store/yhjx1nw8fiivfhpi1pg45v5fkawbpzv9-python-2.7.15-env/lib/python2.7/site-packages/sage/rings/polynomial/multi_polynomial_ideal.py", line 239, in <module>
        from sage.interfaces.all import (singular as singular_default,
      File "/nix/store/yhjx1nw8fiivfhpi1pg45v5fkawbpzv9-python-2.7.15-env/lib/python2.7/site-packages/sage/interfaces/all.py", line 24, in <module>
        from .maxima import maxima, Maxima
      File "/nix/store/yhjx1nw8fiivfhpi1pg45v5fkawbpzv9-python-2.7.15-env/lib/python2.7/site-packages/sage/interfaces/maxima.py", line 1238, in <module>
        script_subdirectory=None)
      File "/nix/store/yhjx1nw8fiivfhpi1pg45v5fkawbpzv9-python-2.7.15-env/lib/python2.7/site-packages/sage/interfaces/maxima.py", line 529, in __init__
        raise RuntimeError('You must get the file local/bin/sage-maxima.lisp')
    RuntimeError: You must get the file local/bin/sage-maxima.lisp

Is it the intention for that test to work in every environment? If its not, I think it is a step back.

[fbissey]

It is very strange. Nothing in this ticket changes sage/interface/maxima.py or the variable it gets from env.py. The error message could use a bit of work from the point of view of distro though.

       STARTUP = os.path.join(SAGE_LOCAL,'bin','sage-maxima.lisp')

       if not os.path.exists(STARTUP):
            raise RuntimeError('You must get the file local/bin/sage-maxima.lisp')

if the message in runtime error was quoting STARTUP instead of a fixed path we may have more of a clue as to what's happening to you.

[gh-timokau]

I suspect it is because the SAGE_LOCAL I set was replaced by

var('SAGE_LOCAL', os.path.abspath(sys.prefix))

Which isn't accurate for me. I don't mind that the fallbacks won't work on every system (if they would, there would be no point in making it configurable in the first place). But I'm not sure if we should include a test for those fallbacks.

I've planned to just migrate my custom sage-env.sh to sage-env.py and import that from env.py for a while now, so that would solve the problem for me. Still not entirely convinced the test is a good idea.

[jdemeyer]

I think the test is a good idea because it should work in all "normal" installation scenarios. Nix is the special one here, so it's not surprising to me that stuff breaks on Nix.

[gh-timokau]

I disagree. In my opinion if tests depend on a certain environment, that is a bug.

But I won't fight anyone on this.

[fbissey]

That value for SAGE_LOCAL follows my own suggestion with suppose that sage and its software is all installed under the same "prefix". Of course in nix case that's a little bit different. In a way, it's a more general case.

You would have to rethink quite a bit of the wiring of sage to eliminate the need for SAGE_LOCAL - I wouldn't be against, but that's too much work in one sitting.

[gh-timokau]

Replying to fbissey:

That value for SAGE_LOCAL follows my own suggestion with suppose that sage and its software is all installed under the same "prefix". Of course in nix case that's a little bit different. In a way, it's a more general case.

You would have to rethink quite a bit of the wiring of sage to eliminate the need for SAGE_LOCAL - I wouldn't be against, but that's too much work in one sitting.

I'm not saying this change is bad. If it works for more cases than it did before, that is great. I just think that the tests should work in the more "general" case (a python library can be anywhere in sys.path, doesn't have to be in prefix) and thus we shouldn't test for this.

[fbissey]

Sure. SAGE_LOCAL is really mostly an installation "prefix" and it make sense in the most usual case which is why the current value is useful for most people. There are arguments for eliminating it like SAGE_ROOT was mostly disposed of before.

In the case of STARTUP and sage-maxima.lisp, my own view is that it ends up in SAGE_LOCAL/bin by accident. It is a freaking configuration file not an executable and it belongs to SAGE_ETC but it was easier to put it in src/bin where it will be installed neatly with the rest. Same with sage-env*. One day we'll have to fix that properly in vanilla sage.

[embray]

I think sage-maxima.lisp should not be where it currently lives. If it's needed by the sage Python package to work then it should just be installed inside the package (e.g. as package_data in setup.py). Please open a ticket. I opened #27171.

[jdemeyer]

Serious breakage: #27389