add sage/env.py and fix many inappropriate references to SAGE_ROOT

[ohanar]

There are many places within the Sage library that make explicit references to SAGE_ROOT, when SAGE_LOCAL, SAGE_DATA, etc. are more appropriate. These references assume a particular directory structure which unnecessarily break various components when using a different directory structure (i.e. transitioning to git or sage-on-gentoo). This ticket aims to correct many of these references.

This ticket also adds sage.env as a place to keep global used Sage variables (these currently live in sage.misc.misc). Currently this file does little more than separate out some of these variables, but the intent is to provide a dedicated place in python for determining the environment for the Sage library (this will eventually need to be independent of sage-env if we are ever to truly transition to argparse).

Followup: #14226.

Installation Instructions:

• apply attachment:trac13432.patch​ to the sage library
• apply attachment:trac_13432-fix_expect.patch​ to the sage library

[jdemeyer]

+1 to the idea.

[fbissey]

I am very supportive as well as some people would know. The main challenge is to avoid duplication with sage-env. How is the sage "executable" supposed to know about SAGE_ROOT/SAGE_LOCAL from the new env.py?

[jdemeyer]

Replying to fbissey:

I am very supportive as well as some people would know. The main challenge is to avoid duplication with sage-env.

I don't think there is duplication if sage/env.py simply reads the environment variables set by sage-env. I see it as a library interface to the sage-env script.

[ohanar]

Eventually I would like sage.env to be independent of sage-env, so that our executable can be pure python (using argparse), but that is probably in distant future :).

[fbissey]

OK I read it a bit more carefully and I see it is sourced from the environment. I guess sage in its current state can work like that without problem.

And yes from my sage-on-gentoo point of view where sage is part of the system I would need it to be independent. There are issues in s-o-g when you import thing like sympy in python because it will pull sage if installed. If the variables are not in the environment things break in sageinspect.

We used to define SAGE variable in the global environment but that had its issue and the lmnd people (should copy burcin on this) didn't like it. At the moment I have pushed things in misc/misc.py to make things work in most cases.

[jdemeyer]

Replying to ohanar:

Eventually I would like sage.env to be independent of sage-env, so that our executable can be pure python (using argparse), but that is probably in distant future :).

I'm not sure whether I like that, but since it's in the distant future, I don't have to worry about it.

[ohanar]

Ok, this has been fixed up and should be ready to go now.

[fbissey]

That looks so much like the mega patch that I apply to sage-on-gentoo. OK my equivalent of env.py is not nearly as sophisticated and I don't have some of these new variables. But otherwise that's scary and this will simplify my life a great deal.

I am going through comparing with my own stuff.

[fbissey]

OK so far the main differences with my own stuff is that I have gone on a hunt on a wider ranger of variables. SAGE_ROOT is the lead bad guy but I also went for most instances of os.environSAGE_LOCAL? and others that made sense to me. This ticket is just for SAGE_ROOT and that's already a big chunk.

[ohanar]

SAGE_ROOT is the main bad guy for my purposes (which is the transition to git). I haven't completely fixed every single usage of it in the library, because some bits required a good amount of logic to rework, so if your patch has fixes for the bits that I omitted that would be appreciated.

I think that a follow up ticket for the SAGE_LOCAL instances would be a good idea. You should check my usage's of SAGE_SRC and see if your patch gets away something more or less equivalent to my SAGE_LIB in any of those places.

[fbissey]

In one of my first attempt I ended up with an unbuildable sage :( but getting all the variables in an independent file is the big cure for that. I understand the problem with the logic. I still have one doctest failure in lazy_import_cache in sage-on-gentoo and I am not sure why.

I didn't create any new variables like SAGE_SRC and SAGE_LIB, that was a lot of work as it is.

I believe the other instances of SAGE_ROOT in sage/misc/cython.py can go.

@@ -335,10 +336,10 @@
     Before :trac:`12975`, it would have beeen needed to write ``#clang c++``,
     but upper case ``C++`` has resulted in an error::

-        sage: from sage.all import SAGE_ROOT
+        sage: from sage.env import SAGE_LOCAL
         sage: code = [
         ... "#clang C++",
-        ... "#cinclude %s/local/include/singular %s/local/include/factory"%(SAGE_ROOT,SAGE_ROOT),
+        ... "#cinclude %s/include/singular %s/include/singular"%(SAGE_LOCAL,SAGE_LOCAL),
         ... "#clib m readline singular givaro ntl gmpxx gmp",
         ... "from sage.rings.polynomial.multi_polynomial_libsingular cimport MPolynomial_libsin$
         ... "from sage.libs.singular.polynomial cimport singular_polynomial_pow",
@@ -449,12 +450,7 @@
 import distutils.sysconfig, os, sys
 from distutils.core import setup, Extension

-if not os.environ.has_key('SAGE_ROOT'):
-    print "    ERROR: The environment variable SAGE_ROOT must be defined."
-    sys.exit(1)
-else:
-    SAGE_ROOT  = os.environ['SAGE_ROOT']
-    SAGE_LOCAL = SAGE_ROOT + '/local/'
+from sage.env import SAGE_ROOT, SAGE_LOCAL

 extra_link_args =  ['-L' + SAGE_LOCAL + '/lib']
 extra_compile_args = %s

I edited it so it is closer to your style.

[fbissey]

sage/sandpiles/sandpile.py you have

SAGE_ROOT = os.environ['SAGE_ROOT'] 
path_to_zsolve = SAGE_ROOT+'/local/bin/' 

becoming

path_to_zsolve = os.path.join(SAGE_LOCAL,'bin','zsolve')

Is it correcting a bug as well?

[ohanar]

Replying to fbissey:

sage/sandpiles/sandpile.py you have

SAGE_ROOT = os.environ['SAGE_ROOT'] 
path_to_zsolve = SAGE_ROOT+'/local/bin/' 

becoming

path_to_zsolve = os.path.join(SAGE_LOCAL,'bin','zsolve')

Is it correcting a bug as well?

No, if you look a little further down, I change a path_to_zsolve+'zsolve' to just path_to_zsolve -- this is a little cleaner, and it removes the string arithmetic that should be os.path.join's.

[fbissey]

Replying to ohanar:

Replying to fbissey:

sage/sandpiles/sandpile.py you have

SAGE_ROOT = os.environ['SAGE_ROOT'] 
path_to_zsolve = SAGE_ROOT+'/local/bin/' 

becoming

path_to_zsolve = os.path.join(SAGE_LOCAL,'bin','zsolve')

Is it correcting a bug as well?

No, if you look a little further down, I change a path_to_zsolve+'zsolve' to just path_to_zsolve -- this is a little cleaner, and it removes the string arithmetic that should be os.path.join's.

I missed that line.

Is the replacement of SAGE_INC in setup.py (well spotted the one where it is still SAGE_LOCAL/include) by CPATH really necessary? That's the only place CPATH is used and you don't use it in module_list.py anyway.

Apart from this I am positive that it should go in while it is easy to apply. We may do some more work on this later.

[jdemeyer]

Don't use CPATH the way you are using it. Invent a new variable SAGE_INCLUDE or whatever.

CPATH is a colon-separated list of directories searched by GCC for include files.

[fbissey]

I think CPATH (and its idea) should be dropped from sage.env altogether and setup.py only be touched to normalize its use of SAGE_INC.

[jdemeyer]

Replying to fbissey:

I think CPATH (and its idea) should be dropped from sage.env altogether.

Yes exactly.

[ohanar]

Ok, updated the patch. Added the fix for cython.py suggested and reverted CPATH to SAGE_INC and it is no longer defined in env.py. I also modified module_list.py to use the new module as well.

[fbissey]

Thank you for the new patch. I will still whine a bit about setup.py. In the definition of "include_dirs" on line 54 (after patch) you didn't put SAGE_INC itself. I think the list should be

include_dirs = [SAGE_INC,
                os.path.join(SAGE_INC, 'csage'), 
 	              os.path.join(SAGE_SRC, 'sage', 'ext')]

I don't think we can count on CPATH being defined and providing SAGE_INC in all cases so I think it needs to be included here.

[ohanar]

Replying to fbissey:

I don't think we can count on CPATH being defined and providing SAGE_INC in all cases so I think it needs to be included here.

I thought the point of #13348 was so that we didn't have to add SAGE_LOCAL/include to spkgs anymore. Truthfully, I would also like to replace os.path.join(SAGE_INC, 'csage') with os.path.join(SAGE_SRC, 'c_lib', 'include'), since in some distant future, I would like the sage python library, c library, extcode, and scripts to be a single "sage" package (but obviously this conflicts with both the setup of sage and sage-on-gentoo presently).

[fbissey]

And truthfully I would like the c_lib to be handled differently, not completely sure how but differently. I was concerned that not putting SAGE_INC would break some upgrades. But upgrade from very old versions of sage is not recommended or guaranteed to work in any case. CPATH has been included in sage 5.4 that should be old enough.

Positive review then.

[ohanar]

apply to root repository

[ohanar]

Replying to fbissey:

I was concerned that not putting SAGE_INC would break some upgrades.

Good point, I think Jeroen tries to make sure that all versions back to 4.8 can still upgrade. This should still work even for those versions since the sage library implicitly depends on SAGE_ROOT_REPO, but just to be sure, I've added a tiny patch to the root repository to make this dependency explicit.

[jdemeyer]

I test upgrades starting from sage-4.5 (older versions don't work because they lack the pipestatus script).

[jdemeyer]

In sage/interfaces/expect.py, this usage of DOT_SAGE needs to be fixed:

        #If the 'SAGE_PEXPECT_LOG' environment variable is set and
        #the current logfile is None, then set the logfile to be one
        #in .sage/pexpect_logs/
        if self.__logfile is None and 'SAGE_PEXPECT_LOG' in os.environ:
            from sage.misc.all import DOT_SAGE
            logs = '%s/pexpect_logs'%DOT_SAGE
            sage_makedirs(logs)

            filename = '%s/%s-%s-%s-%s.log'%(logs, self.name(), os.getpid(), id(self), self._session_number)
            self.__logfile = open(filename, 'w')

[fbissey]

Replying to jdemeyer:

In sage/interfaces/expect.py, this usage of DOT_SAGE needs to be fixed:

        #If the 'SAGE_PEXPECT_LOG' environment variable is set and
        #the current logfile is None, then set the logfile to be one
        #in .sage/pexpect_logs/
        if self.__logfile is None and 'SAGE_PEXPECT_LOG' in os.environ:
            from sage.misc.all import DOT_SAGE
            logs = '%s/pexpect_logs'%DOT_SAGE
            sage_makedirs(logs)

            filename = '%s/%s-%s-%s-%s.log'%(logs, self.name(), os.getpid(), id(self), self._session_number)
            self.__logfile = open(filename, 'w')

That shouldn't be necessary to build or run sage and is covered by backward compatibility in sage/misc/misc.py but OK will fix it.

[fbissey]

Actually why this particular file even after the big patch there are several instances of sage variables imported from misc/misc.py. Do you want them all switched to env.py?

[jdemeyer]

Replying to fbissey:

That shouldn't be necessary to build or run sage and is covered by backward compatibility in sage/misc/misc.py

No, it's not covered since this one imports from sage.misc.all. And yes, it leads to failures (in an unrelated doctest in #12415).

[fbissey]

Ha. I missed that. In my version of this for sage-on-gentoo I made sage.misc.all backward compatible, ohanar didn't. I will check what other places rely on misc.all.

[fbissey]

Fix DOT_SAGE in expect.py

[fbissey]

That was the last spot. I added a trivial patch to take care of it.

[jdemeyer]

When building Sage, there are lots of warnings of the form

  warnings.warn(msg+' I will assume it is a system C/C++ header.')
setup.py:637: UserWarning: could not find dependency linbox/algorithms/echelon-form.h included in sage/libs/linbox/echelonform.pxd. I will assume it is a system C/C++ header.

This is a problem, those headers are not system headers, which might break dependency checking.

[ohanar]

#13031 would resolve this issue.

[fbissey]

While that ticket may really fix the problem, now I wish I hadn't said yes to your removal of SAGE_INC on the basis of CPATH. It obviously doesn't work here.

Admittely that probably means there is a bug somewhere as you would expect it to work.

[ohanar]

apply to sage library

[ohanar]

Ok, threw SAGE_INC back in, so those errors don't come up anymore.

[fbissey]

Thanks. I don't think that's essential to your git migration. More a question of minimal"beauty". We can always remove it with #13031 if it really negates the need for it.

[jhpalmieri]

If you want people to use these variables "correctly", you'd better document them somewhere. I hope you're working on a follow-up ticket for this...

[ohanar]

I made a ticket for documenting development environment variables at #14262 -- I probably won't get around to it for another week/week and a half, as I'm mainly prepping for Sage Days 47 (and then might not get a chance to work on it at the Sage Days).

[mmezzarobba]

Related: #15005.

[jhpalmieri]

Sorry to revive an old ticket, but the people involved here might be able to answer the best: what is the rationale behind the variable substitution used in sage/env.py? That is, what is the advantage to doing the string substitution "$SAGE_LOCAL" --> SAGE_ENV['SAGE_LOCAL'], rather than just using the (already defined) variable SAGE_LOCAL? Using an example from the file, why use

_add_variable_or_fallback('SAGE_ETC',        opj('$SAGE_LOCAL', 'etc'))

instead of

_add_variable_or_fallback('SAGE_ETC',        opj(SAGE_LOCAL, 'etc'))

I am asking because there is a bug in the variable substitution (which I attempted to address in #23758): the variable substitution is done by iterating over SAGE_ENV.items(), which I think has no well-defined order, yet the order in which the substitution is done can matter, for example if you have variables SAGE_LOCAL and SAGE_LOCAL_OTHER, "$SAGE_LOCAL_OTHER" matches both as a string. In #23762, the possibility has been raised of removing the variable substitution altogether, so I want to know what we would be losing if we do this.