Opened 8 years ago

Closed 8 years ago

Last modified 3 years ago

#13432 closed task (fixed)

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

Reported by: ohanar Owned by: jason
Priority: major Milestone: sage-5.9
Component: misc Keywords: build warnings
Cc: jdemeyer, fbissey, burcin Merged in: sage-5.9.beta0
Authors: R. Andrew Ohana Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13123, #13348 Stopgaps:

Description (last modified by 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:

Attachments (3)

trac13432_root.patch (561 bytes) - added by ohanar 8 years ago.
apply to root repository
trac_13432-fix_expect.patch (770 bytes) - added by fbissey 8 years ago.
Fix DOT_SAGE in expect.py
trac13432.patch (39.4 KB) - added by ohanar 8 years ago.
apply to sage library

Download all attachments as: .zip

Change History (48)

comment:1 Changed 8 years ago by ohanar

  • Cc jdemeyer fbissey added
  • Dependencies set to #13123

comment:2 Changed 8 years ago by jdemeyer

+1 to the idea.

comment:3 follow-ups: Changed 8 years ago by 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?

comment:4 in reply to: ↑ 3 Changed 8 years ago by 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.

comment:5 in reply to: ↑ 3 ; follow-up: Changed 8 years ago by ohanar

  • Summary changed from add sage.env and fix a inappropriate references to SAGE_ROOT to add sage.env and fix inappropriate references to SAGE_ROOT

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

comment:6 Changed 8 years ago by 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.

comment:7 in reply to: ↑ 5 Changed 8 years ago by 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.

comment:8 Changed 8 years ago by burcin

  • Cc burcin added

comment:9 Changed 8 years ago by ohanar

  • Dependencies changed from #13123 to #13123, #13348
  • Description modified (diff)

comment:10 Changed 8 years ago by ohanar

  • Description modified (diff)
  • Status changed from new to needs_review
  • Summary changed from add sage.env and fix inappropriate references to SAGE_ROOT to add sage.env and fix many inappropriate references to SAGE_ROOT

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

comment:11 Changed 8 years ago by jdemeyer

  • Summary changed from add sage.env and fix many inappropriate references to SAGE_ROOT to add sage/env.py and fix many inappropriate references to SAGE_ROOT

comment:12 Changed 8 years ago by ohanar

  • Description modified (diff)

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

comment:14 Changed 8 years ago by 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.

comment:15 Changed 8 years ago by 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.

Last edited 8 years ago by ohanar (previous) (diff)

comment:16 Changed 8 years ago by 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.

comment:17 follow-up: Changed 8 years ago by 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?

comment:18 in reply to: ↑ 17 ; follow-up: Changed 8 years ago by 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.

comment:19 in reply to: ↑ 18 Changed 8 years ago by 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.

comment:20 Changed 8 years ago by 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.

comment:21 follow-up: Changed 8 years ago by 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.

comment:22 in reply to: ↑ 21 Changed 8 years ago by jdemeyer

Replying to fbissey:

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

Yes exactly.

comment:23 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.8 to sage-5.9

comment:24 Changed 8 years ago by 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.

comment:25 follow-up: Changed 8 years ago by 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.

comment:26 in reply to: ↑ 25 Changed 8 years ago by 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).

comment:27 follow-up: Changed 8 years ago by fbissey

  • Status changed from needs_review to positive_review

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.

Changed 8 years ago by ohanar

apply to root repository

comment:28 in reply to: ↑ 27 Changed 8 years ago by ohanar

  • Description modified (diff)
  • Reviewers set to François Bissey

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.

comment:29 Changed 8 years ago by jdemeyer

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

Last edited 8 years ago by jdemeyer (previous) (diff)

comment:30 follow-up: Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

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

comment:31 in reply to: ↑ 30 ; follow-up: Changed 8 years ago by 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.

comment:32 Changed 8 years ago by 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?

comment:33 in reply to: ↑ 31 Changed 8 years ago by 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).

comment:34 Changed 8 years ago by 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.

Changed 8 years ago by fbissey

Fix DOT_SAGE in expect.py

comment:35 Changed 8 years ago by fbissey

  • Description modified (diff)
  • Status changed from needs_work to needs_review

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

Last edited 8 years ago by fbissey (previous) (diff)

comment:36 Changed 8 years ago by jdemeyer

  • Keywords build warnings added
  • Status changed from needs_review to needs_work

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.

comment:37 Changed 8 years ago by ohanar

#13031 would resolve this issue.

comment:38 Changed 8 years ago by 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.

Changed 8 years ago by ohanar

apply to sage library

comment:39 Changed 8 years ago by ohanar

  • Description modified (diff)
  • Status changed from needs_work to needs_review

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

comment:40 Changed 8 years ago by fbissey

  • Status changed from needs_review to positive_review

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.

comment:41 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.9.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:42 Changed 8 years ago by 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...

comment:43 Changed 8 years ago by 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).

comment:44 Changed 7 years ago by mmezzarobba

Related: #15005.

comment:45 Changed 3 years ago by 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.

Note: See TracTickets for help on using tickets.