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

Depends on #13123
Depends on #13348

CC: @jdemeyer @kiwifb @burcin

Component: misc

Keywords: build warnings

Author: R. Andrew Ohana

Reviewer: François Bissey

Merged: sage-5.9.beta0

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

[ohanar]

Dependencies: #13123

[jdemeyer]

comment:2

+1 to the idea.

[kiwifb]

comment:3

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]

comment:4

Replying to @kiwifb:

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]

comment:5

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 :).

[kiwifb]

comment:6

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]

comment:7

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]

Changed dependencies from #13123 to #13123, #13348

[ohanar]

comment:10

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

[kiwifb]

comment:13

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.

[kiwifb]

comment:14

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.environ['SAGE_LOCAL'] and others that made sense to me. This ticket is just for SAGE_ROOT and that's already a big chunk.

[ohanar]

comment:15

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.

[kiwifb]

comment:16

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.

[kiwifb]

comment:17

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]

comment:18

Replying to @kiwifb:

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.

[kiwifb]

comment:19

Replying to @ohanar:

Replying to @kiwifb:

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]

comment:20

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.

[kiwifb]

comment:21

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]

comment:22

Replying to @kiwifb:

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

Yes exactly.

[kiwifb]

comment:35

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

[jdemeyer]

Changed keywords from none to build warnings

[jdemeyer]

comment:36

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]

comment:37

#13031 would resolve this issue.

[kiwifb]

comment:38

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]

Attachment: trac13432.patch.gz

apply to sage library

[ohanar]

comment:39

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

[kiwifb]

comment:40

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.

[jdemeyer]

Merged: sage-5.9.beta0

[jhpalmieri]

comment:42

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]

comment:43

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).

[mezzarobba]

comment:44

Related: #15005.

[jhpalmieri]

comment:45

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.