#22608 closed defect (fixed)
Remove PYTHONPATH environment variable from sage
Reported by: | vklein | Owned by: | vklein |
---|---|---|---|
Priority: | minor | Milestone: | sage-8.0 |
Component: | misc | Keywords: | PYTHONPATH |
Cc: | vdelecroix, embray | Merged in: | |
Authors: | Vincent Klein, Volker Braun | Reviewers: | Vincent Delecroix |
Report Upstream: | N/A | Work issues: | |
Branch: | 3b9cad2 (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description
Following sage-devel PYTHONPATH breaks Python3https://groups.google.com/forum/#!topic/sage-devel/ITtF1vWIxIk
Having PYTHONPATH sets as an environment variable is reported to be incompatible with the installation of both Python2 and Python3 and with the use of gdb to debug.
We should remove the setting of the PYTHONPATH environment variable
Change History (26)
comment:1 Changed 5 years ago by
- Cc embray added
comment:2 Changed 5 years ago by
- Owner changed from (none) to vklein
comment:3 Changed 5 years ago by
comment:4 Changed 5 years ago by
- Branch set to u/vklein/remove_pythonpath_environment_variable_from_sage
comment:5 Changed 5 years ago by
- Commit set to 22e470c2d6941875908f35ff3657843f1fe83f21
- Status changed from new to needs_review
comment:6 Changed 5 years ago by
I don't fully understand the impact of this patch or why sage-env was setting PYTHONPATH
in the first place, so I'm not sure I'm the best to give a review. But I definitely support this.
comment:7 Changed 5 years ago by
- Status changed from needs_review to needs_work
I don't understand your changes to all.py
:
1) What is the reason of this modification? Were the tests failing without it? If not, I would prefer to not touch anything there. You said to "stay compatible with future Python version" but did you actually checked this?
2) The aim of os.path.join
is precisely to join several directories and files into a path like
>>> import os >>> os.path.join('my', 'path', 'to', 'my_file.py') 'my/path/to/my_file.py'
if the path would only be one string there is no need to use join
3) If you start touching this, why not modifying at the same time
wallowed = [os.path.join("lib","python","threading.py")]
comment:8 follow-up: ↓ 10 Changed 5 years ago by
1) Yes the all.py doctest fail without the modification. If PYTHONPATH is set you get the frames
<sage_path>/local/lib/python/multiprocessing/process.py
<sage_path>/local/lib/python/multiprocessing/forking.py
<sage_path>/local/lib/python/multiprocessing/process.py.
If PYTHONPATH is not set you get
<sage_path>/local/lib/python2.7/multiprocessing/process.py
<sage_path>/local/lib/python2.7/multiprocessing/forking.py
<sage_path>/local/lib/python2.7/multiprocessing/process.py instead and the old doctest failed.
2) Ok.
3) Basically because i didn't get a matching frame during my tests but that's the case for several other item's of the allowed list. Should we rewrite this doctest with this ticket ?
comment:9 Changed 5 years ago by
- Commit changed from 22e470c2d6941875908f35ff3657843f1fe83f21 to 0834fa019fe89bca422760a565b467070b406b3a
Branch pushed to git repo; I updated commit sha1. New commits:
0834fa0 | #all.py allowed.append(os.path.join("multiprocessing")) => allowed.append("multiprocessing")
|
comment:10 in reply to: ↑ 8 Changed 5 years ago by
Replying to vklein:
1) Yes the all.py doctest fail without the modification. If PYTHONPATH is set you get the frames
[...]
Then you would better say it in the ticket description!
2) Ok.
3) Basically because i didn't get a matching frame during my tests but that's the case for several other item's of the allowed list. Should we rewrite this doctest with this ticket ?
Definitely!
comment:11 Changed 5 years ago by
There are a handful of other hardcoded local/lib/python/ paths instead of local/lib/pythonx.y/ in the Sage sources; They don't hurt us yet but will sooner or later. Its always good to get rid of it and make no assumptions about where a particular python module resides.
comment:12 Changed 5 years ago by
- Branch changed from u/vklein/remove_pythonpath_environment_variable_from_sage to u/vbraun/remove_pythonpath_environment_variable_from_sage
comment:13 Changed 5 years ago by
- Commit changed from 0834fa019fe89bca422760a565b467070b406b3a to b2d0636df6f7466c12159e235a95f8e6a1f9aa58
- Status changed from needs_work to needs_review
New commits:
b2d0636 | Better check for the startup stack frames
|
comment:14 Changed 5 years ago by
I don't like the ellipsis there. It does not catch the fact that only generating_series
is there. Only that it is first. For example the following is a perfectly valid doctest
sage: print "<mod1 hello>, <mod2 hello>, <mod3 hello>" <mod1 ...>
You should also check the length of [inspect.getmodule(f) for f in frames if is_not_allowed(f)]
.
BTW, do you know why is this module listed there!?
comment:15 Changed 5 years ago by
- Commit changed from b2d0636df6f7466c12159e235a95f8e6a1f9aa58 to ec259b66d9f6ac91b6190c7640ea347d96630410
Branch pushed to git repo; I updated commit sha1. New commits:
ec259b6 | Get rid of ellipsis in test
|
comment:16 Changed 5 years ago by
I don't know why generating_series is there, clearly it shouldn't. But thats not the point of this ticket.
New commits:
ec259b6 | Get rid of ellipsis in test
|
comment:17 Changed 5 years ago by
I opened #22647 to track the bad apple...
comment:18 Changed 5 years ago by
Code looks good to me, but I feel very weakly qualified, and have no time to test.
comment:19 Changed 5 years ago by
- Milestone changed from sage-7.6 to sage-8.0
- Reviewers set to Vincent Delecroix
- Status changed from needs_review to positive_review
comment:20 Changed 5 years ago by
- Status changed from positive_review to needs_work
Breaks on OSX:
make -j8 base make[2]: warning: -jN forced in submake: disabling jobserver mode. sage-logger -p 'sage-spkg patch-2.7.5' '/Users/buildslave-sage/slave/sage_git/build/logs/pkgs/patch-2.7.5.log' [patch-2.7.5] Found local metadata for patch-2.7.5 [patch-2.7.5] ImportError: No module named site [patch-2.7.5] ************************************************************************ [patch-2.7.5] Error downloading patch-2.7.5.tar.gz [patch-2.7.5] ************************************************************************ [patch-2.7.5] Please email sage-devel (http://groups.google.com/group/sage-devel) [patch-2.7.5] explaining the problem and including the log file [patch-2.7.5] /Users/buildslave-sage/slave/sage_git/build/logs/pkgs/patch-2.7.5.log [patch-2.7.5] Describe your computer, operating system, etc. [patch-2.7.5] ************************************************************************
Shorter test
./sage -p patch Found local metadata for patch-2.7.5 ImportError: No module named site ************************************************************************ Error downloading patch-2.7.5.tar.gz ************************************************************************
comment:21 Changed 5 years ago by
In fact its nothing to do with OSX, this kills our ability to call the system python as soon as SAGE_LOCAL is created (which is way sooner in the build process than when we install our own python):
if [ -d "$SAGE_LOCAL" ]; then PYTHONHOME="$SAGE_LOCAL" export PYTHONHOME fi
resulting in
$ PYTHONHOME=/foo/bar python ImportError: No module named site
IMHO we should just unset PYTHONHOME/PYTHONPATH if they are set (and print a warning that this can't work); Then python will just default to whichever directory is compiled in. The whole PYTHONHOME/PYTHONPATH thing is probably a holdover from the past where we did not clean up paths compiled into binaries.
comment:22 Changed 5 years ago by
- Commit changed from ec259b66d9f6ac91b6190c7640ea347d96630410 to 3b9cad2e2459aafd6adbab55b2028efbebda8e7a
Branch pushed to git repo; I updated commit sha1. New commits:
3b9cad2 | Remove PYTHONHOME, too
|
comment:23 Changed 5 years ago by
- Status changed from needs_work to needs_review
comment:25 Changed 5 years ago by
- Branch changed from u/vbraun/remove_pythonpath_environment_variable_from_sage to 3b9cad2e2459aafd6adbab55b2028efbebda8e7a
- Resolution set to fixed
- Status changed from positive_review to closed
comment:26 Changed 5 years ago by
- Commit 3b9cad2e2459aafd6adbab55b2028efbebda8e7a deleted
After this ticket the use of $SAGE_PATH
broke which is still documented so I created the follow up ticket #23614
Note to reviewer : the doctest in all.py has been modified. I choose to modify allowed.append(os.path.join("lib", "python", "multiprocessing")) into allowed.append(os.path.join("multiprocessing")) rather than allowed.append(os.path.join("lib", "python2.7", "multiprocessing")) in order to stay compatible with future python version.
New commits:
Remove PYTHONPATH env variable and adapt doctest in all.py