Opened 13 months ago

Closed 12 months ago

Last modified 11 months ago

#27040 closed enhancement (fixed)

Cleanup sage.env

Reported by: Snark Owned by:
Priority: major Milestone: sage-8.7
Component: porting Keywords:
Cc: ​infinity0, thansen Merged in:
Authors: Jeroen Demeyer Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: 3b0a094 (Commits) Commit:
Dependencies: #27041 Stopgaps:

Description (last modified by jdemeyer)

  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.
  1. Several unneeded variables are removed.
  1. Get rid of the over-engineered $VAR replacement stuff.
  1. In some places, SAGE_LIB (installed location of Sage) can be used instead of SAGE_SRC.
  1. Determine SAGE_LIB not from site-packages but from sage.__file__
  1. 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.

Change History (49)

comment:1 Changed 13 months ago by Snark

  • Type changed from PLEASE CHANGE to enhancement

comment:2 follow-up: Changed 13 months ago by 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?

comment:3 Changed 13 months ago by fbissey

  • Description modified (diff)

comment:4 in reply to: ↑ 2 Changed 13 months ago by 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.

comment:5 follow-up: Changed 13 months ago by 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.

comment:6 in reply to: ↑ 5 Changed 13 months ago by 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.

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

comment:8 Changed 13 months ago by jdemeyer

  • Component changed from scripts to porting
  • Description modified (diff)
  • Priority changed from trivial to major

comment:9 Changed 13 months ago by jdemeyer

  • Authors set to Jeroen Demeyer

comment:10 Changed 13 months ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Make env.py use system values instead of using seeds to Cleanup sage.env

comment:11 Changed 13 months ago by jdemeyer

  • Description modified (diff)

comment:12 follow-up: Changed 13 months ago by fbissey

  • Description modified (diff)

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.

comment:13 Changed 13 months ago by jdemeyer

  • Description modified (diff)

comment:14 in reply to: ↑ 12 Changed 13 months ago by 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.

comment:15 follow-up: Changed 13 months ago by fbissey

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

comment:16 Changed 13 months ago by vdelecroix

  • Cc ​infinity0 thansen added

comment:17 Changed 13 months ago by vdelecroix

  • Description modified (diff)

comment:18 in reply to: ↑ 15 ; follow-up: Changed 13 months ago by 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.

comment:19 in reply to: ↑ 18 ; follow-up: Changed 13 months ago by 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.

comment:20 in reply to: ↑ 19 Changed 13 months ago by 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?

comment:21 Changed 13 months ago by jdemeyer

  • Description modified (diff)

comment:22 Changed 13 months ago by jdemeyer

  • Status changed from new to needs_review

comment:23 Changed 13 months ago by jdemeyer

  • Branch set to u/jdemeyer/make_env_py_use_system_values_instead_of_using_seeds

comment:24 Changed 13 months ago by fbissey

  • Commit set to 4b2dd54c91227ffe98c0c399d0cbfd64305b9d81

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:

4b2dd54Clean up sage.env

comment:25 Changed 13 months ago by 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.

comment:26 Changed 13 months ago by git

  • Commit changed from 4b2dd54c91227ffe98c0c399d0cbfd64305b9d81 to b81843623428181fc79d51bdb2b1ebccdc60af79

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

b818436Clean up sage.env

comment:27 Changed 13 months ago by 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...

comment:28 Changed 13 months ago by 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.

comment:29 follow-up: Changed 13 months ago by 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.

Last edited 13 months ago by embray (previous) (diff)

comment:30 in reply to: ↑ 29 ; follow-up: Changed 13 months ago by 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 _ :-)

comment:31 Changed 13 months ago by git

  • Commit changed from b81843623428181fc79d51bdb2b1ebccdc60af79 to f10ae3ff592931ee0ed237cf206900d2180ea813

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

f10ae3fClean up sage.env

comment:32 Changed 13 months ago by jdemeyer

  • Description modified (diff)

comment:33 in reply to: ↑ 30 Changed 13 months ago by 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.

comment:34 Changed 13 months ago by jdemeyer

  • Dependencies set to #27041

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

comment:35 Changed 13 months ago by git

  • Commit changed from f10ae3ff592931ee0ed237cf206900d2180ea813 to de3be03dcb49dc1327514f5169d75f6c1b6ee568

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

108a42eRemove deprecated stuff related to Cython
de3be03Clean up sage.env

comment:36 Changed 13 months ago by git

  • Commit changed from de3be03dcb49dc1327514f5169d75f6c1b6ee568 to 3b0a0942b06750089bfae780fc07c32519f2c2aa

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

3b0a094Clean up sage.env

comment:37 Changed 13 months ago by embray

  • Milestone changed from sage-8.6 to sage-8.7
  • Reviewers set to Erik Bray
  • Status changed from needs_review to positive_review

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.

comment:38 Changed 13 months ago by jdemeyer

Note that the dependency #27041 still needs review.

comment:39 Changed 12 months ago by 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.

comment:40 Changed 12 months ago by 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.

comment:41 Changed 12 months ago by 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.

comment:42 Changed 12 months ago by 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.

comment:43 Changed 12 months ago by 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.

comment:44 follow-up: Changed 12 months ago by 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.

comment:45 in reply to: ↑ 44 Changed 12 months ago by 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.

comment:46 Changed 12 months ago by 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.

comment:47 Changed 12 months ago by vbraun

  • Branch changed from u/jdemeyer/make_env_py_use_system_values_instead_of_using_seeds to 3b0a0942b06750089bfae780fc07c32519f2c2aa
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:48 Changed 12 months ago by embray

  • Commit 3b0a0942b06750089bfae780fc07c32519f2c2aa deleted

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.

Last edited 12 months ago by embray (previous) (diff)

comment:49 Changed 11 months ago by jdemeyer

Serious breakage: #27389

Note: See TracTickets for help on using tickets.