#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 )
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
Attachments (3)
Change History (48)
comment:1 Changed 8 years ago by
- Cc jdemeyer fbissey added
- Dependencies set to #13123
comment:2 Changed 8 years ago by
comment:3 follow-ups: ↓ 4 ↓ 5 Changed 8 years ago by
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
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: ↓ 7 Changed 8 years ago by
- 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
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
Replying to ohanar:
Eventually I would like
sage.env
to be independent ofsage-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
- Cc burcin added
comment:9 Changed 8 years ago by
- Dependencies changed from #13123 to #13123, #13348
- Description modified (diff)
comment:10 Changed 8 years ago by
- 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
- 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
- Description modified (diff)
comment:13 Changed 8 years ago by
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
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
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.
comment:16 Changed 8 years ago by
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: ↓ 18 Changed 8 years ago by
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: ↓ 19 Changed 8 years ago by
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
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 justpath_to_zsolve
-- this is a little cleaner, and it removes the string arithmetic that should beos.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
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: ↓ 22 Changed 8 years ago by
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
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
- Milestone changed from sage-5.8 to sage-5.9
comment:24 Changed 8 years ago by
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: ↓ 26 Changed 8 years ago by
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
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: ↓ 28 Changed 8 years ago by
- 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.
comment:28 in reply to: ↑ 27 Changed 8 years ago by
- 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
I test upgrades starting from sage-4.5 (older versions don't work because they lack the pipestatus
script).
comment:30 follow-up: ↓ 31 Changed 8 years ago by
- 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: ↓ 33 Changed 8 years ago by
Replying to jdemeyer:
In
sage/interfaces/expect.py
, this usage ofDOT_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
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
comment:34 Changed 8 years ago by
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.
comment:35 Changed 8 years ago by
- 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.
comment:36 Changed 8 years ago by
- 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
#13031 would resolve this issue.
comment:38 Changed 8 years ago by
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.
comment:39 Changed 8 years ago by
- 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
- 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
- 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
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
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
Related: #15005.
comment:45 Changed 3 years ago by
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.
+1 to the idea.