Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#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) 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 4 years ago by embray

  • Cc embray added

comment:2 Changed 4 years ago by vklein

  • Owner changed from (none) to vklein

comment:3 Changed 4 years ago by vklein

  • Authors changed from vklein to Vincent Klein

comment:4 Changed 4 years ago by vklein

  • Branch set to u/vklein/remove_pythonpath_environment_variable_from_sage

comment:5 Changed 4 years ago by vklein

  • Commit set to 22e470c2d6941875908f35ff3657843f1fe83f21
  • Status changed from new to needs_review

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:

22e470cRemove PYTHONPATH env variable and adapt doctest in all.py

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

comment:7 Changed 4 years ago by vdelecroix

  • 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: Changed 4 years ago by 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 ?

Last edited 4 years ago by vklein (previous) (diff)

comment:9 Changed 4 years ago by git

  • 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 4 years ago by 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!

comment:11 Changed 4 years ago by 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.

comment:12 Changed 4 years ago by vbraun

  • Branch changed from u/vklein/remove_pythonpath_environment_variable_from_sage to u/vbraun/remove_pythonpath_environment_variable_from_sage

comment:13 Changed 4 years ago by vbraun

  • Commit changed from 0834fa019fe89bca422760a565b467070b406b3a to b2d0636df6f7466c12159e235a95f8e6a1f9aa58
  • Status changed from needs_work to needs_review

New commits:

b2d0636Better check for the startup stack frames

comment:14 Changed 4 years ago by 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!?

comment:15 Changed 4 years ago by git

  • Commit changed from b2d0636df6f7466c12159e235a95f8e6a1f9aa58 to ec259b66d9f6ac91b6190c7640ea347d96630410

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

ec259b6Get rid of ellipsis in test

comment:16 Changed 4 years ago by vbraun

I don't know why generating_series is there, clearly it shouldn't. But thats not the point of this ticket.


New commits:

ec259b6Get rid of ellipsis in test

comment:17 Changed 4 years ago by vdelecroix

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

comment:18 Changed 4 years ago by chapoton

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

comment:19 Changed 4 years ago by vdelecroix

  • Authors changed from Vincent Klein to Vincent Klein, Volker Braun
  • 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 4 years ago by vbraun

  • 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 4 years ago by 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.

comment:22 Changed 4 years ago by git

  • Commit changed from ec259b66d9f6ac91b6190c7640ea347d96630410 to 3b9cad2e2459aafd6adbab55b2028efbebda8e7a

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

3b9cad2Remove PYTHONHOME, too

comment:23 Changed 4 years ago by vbraun

  • Status changed from needs_work to needs_review

comment:24 Changed 4 years ago by embray

  • Status changed from needs_review to positive_review

Fine be me.

comment:25 Changed 4 years ago by vbraun

  • 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 3 years ago by mderickx

  • 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: See TracTickets for help on using tickets.