#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, GitHub, GitLab) | Commit: | |
Dependencies: | #27041 | Stopgaps: |
Description (last modified by )
sage.all
should be importable and mostly usable even if no environment variables likeSAGE_ROOT
orSAGE_LOCAL
are set. This requires a new functionjoin
which is likeos.path.join
except thatNone
values are propagated.
- Several unneeded variables are removed.
- Get rid of the over-engineered
$VAR
replacement stuff.
- In some places,
SAGE_LIB
(installed location of Sage) can be used instead ofSAGE_SRC
.
- Determine
SAGE_LIB
not fromsite-packages
but fromsage.__file__
- 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 4 years ago by
- Type changed from PLEASE CHANGE to enhancement
comment:2 follow-up: ↓ 4 Changed 4 years ago by
comment:3 Changed 4 years ago by
- Description modified (diff)
comment:4 in reply to: ↑ 2 Changed 4 years ago by
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: ↓ 6 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
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 4 years ago by
- Component changed from scripts to porting
- Description modified (diff)
- Priority changed from trivial to major
comment:9 Changed 4 years ago by
comment:10 Changed 4 years ago by
- Description modified (diff)
- Summary changed from Make env.py use system values instead of using seeds to Cleanup sage.env
comment:11 Changed 4 years ago by
- Description modified (diff)
comment:12 follow-up: ↓ 14 Changed 4 years ago by
- 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 4 years ago by
- Description modified (diff)
comment:14 in reply to: ↑ 12 Changed 4 years ago by
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: ↓ 18 Changed 4 years ago by
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 4 years ago by
- Cc infinity0 thansen added
comment:17 Changed 4 years ago by
- Description modified (diff)
comment:18 in reply to: ↑ 15 ; follow-up: ↓ 19 Changed 4 years ago by
Replying to fbissey:
Will using
sage.__file__
change the way we useSAGE_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: ↓ 20 Changed 4 years ago by
Replying to jdemeyer:
Replying to fbissey:
Will using
sage.__file__
change the way we useSAGE_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 knowsage.__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 4 years ago by
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 4 years ago by
- Description modified (diff)
comment:22 Changed 4 years ago by
- Status changed from new to needs_review
comment:23 Changed 4 years ago by
- Branch set to u/jdemeyer/make_env_py_use_system_values_instead_of_using_seeds
comment:24 Changed 4 years ago by
- 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:
4b2dd54 | Clean up sage.env
|
comment:25 Changed 4 years ago by
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 4 years ago by
- Commit changed from 4b2dd54c91227ffe98c0c399d0cbfd64305b9d81 to b81843623428181fc79d51bdb2b1ebccdc60af79
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b818436 | Clean up sage.env
|
comment:27 Changed 4 years ago by
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 4 years ago by
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: ↓ 30 Changed 4 years ago by
+
+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
.
comment:30 in reply to: ↑ 29 ; follow-up: ↓ 33 Changed 4 years ago by
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 4 years ago by
- Commit changed from b81843623428181fc79d51bdb2b1ebccdc60af79 to f10ae3ff592931ee0ed237cf206900d2180ea813
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f10ae3f | Clean up sage.env
|
comment:32 Changed 4 years ago by
- Description modified (diff)
comment:33 in reply to: ↑ 30 Changed 4 years ago by
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 4 years ago by
- Dependencies set to #27041
Various doctest failures on the patchbot but only in deprecated functionality.
comment:35 Changed 4 years ago by
- Commit changed from f10ae3ff592931ee0ed237cf206900d2180ea813 to de3be03dcb49dc1327514f5169d75f6c1b6ee568
comment:36 Changed 4 years ago by
- Commit changed from de3be03dcb49dc1327514f5169d75f6c1b6ee568 to 3b0a0942b06750089bfae780fc07c32519f2c2aa
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
3b0a094 | Clean up sage.env
|
comment:37 Changed 4 years ago by
- 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 4 years ago by
Note that the dependency #27041 still needs review.
comment:39 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
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 4 years ago by
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 4 years ago by
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: ↓ 45 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
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 4 years ago by
- 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 4 years ago by
- 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.
comment:49 Changed 3 years ago by
Serious breakage: #27389
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?