Remove PYTHONPATH environment variable from sage

[vklein]

Following sage-devel PYTHONPATH breaks Python3​https://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

[vklein]

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:

​22e470c

Remove PYTHONPATH env variable and adapt doctest in all.py

[embray]

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.

[vdelecroix]

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")]

[vklein]

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 ?

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​0834fa0

#all.py allowed.append(os.path.join("multiprocessing")) => allowed.append("multiprocessing")

[vdelecroix]

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!

[vbraun]

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.

[vbraun]

New commits:

​b2d0636

Better check for the startup stack frames

[vdelecroix]

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!?

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​ec259b6

Get rid of ellipsis in test

[vbraun]

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

[vdelecroix]

I opened #22647 to track the bad apple...

[chapoton]

Code looks good to me, but I feel very weakly qualified, and have no time to test.

[vbraun]

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
************************************************************************

[vbraun]

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.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​3b9cad2

Remove PYTHONHOME, too

[embray]

Fine be me.

[mderickx]

After this ticket the use of $SAGE_PATH broke which is still documented so I created the follow up ticket #23614