Fix setting PYTHONPATH on Python 2 not working (pypa#1673)

- ensure we do not rewrite the ``PYTHONPATH``
- ensure we respect the ``-E`` flag

Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>

Co-authored-by: Bernát Gábor <gaborjbernat@gmail.com>

docs/changelog/1673.bugfix.rst

@@ -0,0 +1 @@
+Fix ``PYTHONPATH`` being overriden on Python 2 — by :user:`jd`.

src/virtualenv/create/via_global_ref/builtin/pypy/pypy2.py

@@ -72,7 +72,7 @@ def skip_rewrite(self):
         PyPy2 built-in imports are handled by this path entry, don't overwrite to not disable it
         see: /~https://github.com/pypa/virtualenv/issues/1652
         """
-        return 'or value.endswith("lib_pypy{}__extensions__")'.format(os.sep)
+        return 'or path.endswith("lib_pypy{}__extensions__") # PyPy2 built-in import marker'.format(os.sep)
 
 
 class PyPy2Posix(PyPy2, PosixSupports):

src/virtualenv/create/via_global_ref/builtin/python2/site.py

@@ -78,40 +78,59 @@ def read_pyvenv():
 
 def rewrite_standard_library_sys_path():
     """Once this site file is loaded the standard library paths have already been set, fix them up"""
-    exe = abs_path(sys.executable)
+    exe, prefix, exec_prefix = get_exe_prefixes(base=False)
+    base_exe, base_prefix, base_exec = get_exe_prefixes(base=True)
     exe_dir = exe[: exe.rfind(sep)]
-    prefix, exec_prefix = abs_path(sys.prefix), abs_path(sys.exec_prefix)
-    base_prefix, base_exec_prefix = abs_path(sys.base_prefix), abs_path(sys.base_exec_prefix)
-    base_executable = abs_path(sys.base_executable)
-    for at, value in enumerate(sys.path):
-        value = abs_path(value)
-        # replace old sys prefix path starts with new
-        skip_rewrite = value == exe_dir  # don't fix the current executable location, notably on Windows this gets added
+    for at, path in enumerate(sys.path):
+        path = abs_path(path)  # replace old sys prefix path starts with new
+        skip_rewrite = path == exe_dir  # don't fix the current executable location, notably on Windows this gets added
         skip_rewrite = skip_rewrite  # ___SKIP_REWRITE____
         if not skip_rewrite:
-            if value.startswith(exe_dir):
-                # content inside the exe folder needs to remap to original executables folder
-                orig_exe_folder = base_executable[: base_executable.rfind(sep)]
-                value = "{}{}".format(orig_exe_folder, value[len(exe_dir) :])
-            elif value.startswith(prefix):
-                value = "{}{}".format(base_prefix, value[len(prefix) :])
-            elif value.startswith(exec_prefix):
-                value = "{}{}".format(base_exec_prefix, value[len(exec_prefix) :])
-        sys.path[at] = value
+            sys.path[at] = map_path(path, base_exe, exe_dir, exec_prefix, base_prefix, prefix, base_exec)
+
+    # the rewrite above may have changed elements from PYTHONPATH, revert these if on
+    if sys.flags.ignore_environment:
+        return
+    import os
+
+    python_paths = []
+    if "PYTHONPATH" in os.environ and os.environ["PYTHONPATH"]:
+        for path in os.environ["PYTHONPATH"].split(os.pathsep):
+            if path not in python_paths:
+                python_paths.append(path)
+    sys.path[: len(python_paths)] = python_paths
+
+
+def get_exe_prefixes(base=False):
+    return tuple(abs_path(getattr(sys, ("base_" if base else "") + i)) for i in ("executable", "prefix", "exec_prefix"))
 
 
 def abs_path(value):
-    keep = []
-    values = value.split(sep)
-    i = len(values) - 1
-    while i >= 0:
-        if values[i] == "..":
-            i -= 1
+    values, keep = value.split(sep), []
+    at = len(values) - 1
+    while at >= 0:
+        if values[at] == "..":
+            at -= 1
         else:
-            keep.append(values[i])
-        i -= 1
-    value = sep.join(keep[::-1])
-    return value
+            keep.append(values[at])
+        at -= 1
+    return sep.join(keep[::-1])
+
+
+def map_path(path, base_executable, exe_dir, exec_prefix, base_prefix, prefix, base_exec_prefix):
+    if path_starts_with(path, exe_dir):
+        # content inside the exe folder needs to remap to original executables folder
+        orig_exe_folder = base_executable[: base_executable.rfind(sep)]
+        return "{}{}".format(orig_exe_folder, path[len(exe_dir) :])
+    elif path_starts_with(path, prefix):
+        return "{}{}".format(base_prefix, path[len(prefix) :])
+    elif path_starts_with(path, exec_prefix):
+        return "{}{}".format(base_exec_prefix, path[len(exec_prefix) :])
+    return path
+
+
+def path_starts_with(directory, value):
+    return directory.startswith(value if value[-1] == sep else value + sep)
 
 
 def disable_user_site_package():

tests/unit/create/conftest.py

@@ -22,11 +22,11 @@
 
 
 # noinspection PyUnusedLocal
-def root(tmp_path_factory):
+def root(tmp_path_factory, session_app_data):
     return CURRENT.system_executable
 
 
-def venv(tmp_path_factory):
+def venv(tmp_path_factory, session_app_data):
     if CURRENT.is_venv:
         return sys.executable
     elif CURRENT.version_info.major == 3:
@@ -40,7 +40,7 @@ def venv(tmp_path_factory):
         return exe_path
 
 
-def old_virtualenv(tmp_path_factory):
+def old_virtualenv(tmp_path_factory, session_app_data):
     if CURRENT.is_old_virtualenv:
         return CURRENT.executable
     else:
@@ -77,18 +77,18 @@ def old_virtualenv(tmp_path_factory):
             process = Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
             _, __ = process.communicate()
             assert not process.returncode
-            exe_path = CURRENT.discover_exe(prefix=str(old_virtualenv_at)).original_executable
+            exe_path = CURRENT.discover_exe(session_app_data, prefix=str(old_virtualenv_at)).original_executable
             return exe_path
-        except Exception:
-            return RuntimeError("failed to create old virtualenv")
+        except Exception as exception:
+            return RuntimeError("failed to create old virtualenv %r".format(exception))
 
 
 PYTHON = {"root": root, "venv": venv, "old_virtualenv": old_virtualenv}
 
 
 @pytest.fixture(params=list(PYTHON.values()), ids=list(PYTHON.keys()), scope="session")
-def python(request, tmp_path_factory):
-    result = request.param(tmp_path_factory)
+def python(request, tmp_path_factory, session_app_data):
+    result = request.param(tmp_path_factory, session_app_data)
     if isinstance(result, Exception):
         pytest.skip("could not resolve interpreter based on {} because {}".format(request.param.__name__, result))
     if result is None:

tests/unit/create/test_creator.py

@@ -2,12 +2,14 @@
 
 import difflib
 import gc
+import json
 import logging
 import os
 import shutil
 import stat
 import subprocess
 import sys
+from collections import OrderedDict
 from itertools import product
 from stat import S_IREAD, S_IRGRP, S_IROTH
 from textwrap import dedent
@@ -19,11 +21,11 @@
 from virtualenv.create.creator import DEBUG_SCRIPT, Creator, get_env_debug_info
 from virtualenv.discovery.builtin import get_interpreter
 from virtualenv.discovery.py_info import PythonInfo
-from virtualenv.info import IS_PYPY, PY3, fs_supports_symlink
+from virtualenv.info import IS_PYPY, PY3, fs_is_case_sensitive, fs_supports_symlink
 from virtualenv.pyenv_cfg import PyEnvCfg
 from virtualenv.run import cli_run, session_via_cli
 from virtualenv.util.path import Path
-from virtualenv.util.six import ensure_text
+from virtualenv.util.six import ensure_str, ensure_text
 
 CURRENT = PythonInfo.current_system()
 
@@ -386,3 +388,59 @@ def test_create_distutils_cfg(creator, tmp_path, monkeypatch):
 
     package_folder = result.creator.platlib / "demo"  # prefix is set to the virtualenv prefix for install
     assert package_folder.exists()
+
+
+@pytest.mark.parametrize("python_path_on", [True, False], ids=["on", "off"])
+@pytest.mark.skipif(PY3, reason="we rewrite sys.path only on PY2")
+def test_python_path(monkeypatch, tmp_path, python_path_on):
+    result = cli_run([ensure_text(str(tmp_path)), "--without-pip", "--activators", ""])
+    monkeypatch.chdir(tmp_path)
+    case_sensitive = fs_is_case_sensitive()
+
+    def _get_sys_path(flag=None):
+        cmd = [str(result.creator.exe)]
+        if flag:
+            cmd.append(flag)
+        cmd.extend(["-c", "import json; import sys; print(json.dumps(sys.path))"])
+        return [i if case_sensitive else i.lower() for i in json.loads(subprocess.check_output(cmd))]
+
+    monkeypatch.delenv(str("PYTHONPATH"), raising=False)
+    base = _get_sys_path()
+
+    # note the value result.creator.interpreter.system_stdlib cannot be set, as that would disable our custom site.py
+    python_paths = [
+        str(Path(result.creator.interpreter.prefix)),
+        str(Path(result.creator.interpreter.system_stdlib) / "b"),
+        str(result.creator.purelib / "a"),
+        str(result.creator.purelib),
+        str(result.creator.bin_dir),
+        str(tmp_path / "base"),
+        str(tmp_path / "base_sep") + os.sep,
+        "name",
+        "name{}".format(os.sep),
+        str(tmp_path.parent / (ensure_text(tmp_path.name) + "_suffix")),
+        ".",
+        "..",
+        "",
+    ]
+    python_path_env = os.pathsep.join(ensure_str(i) for i in python_paths)
+    monkeypatch.setenv(str("PYTHONPATH"), python_path_env)
+
+    extra_all = _get_sys_path(None if python_path_on else "-E")
+    if python_path_on:
+        assert extra_all[0] == ""  # the cwd is always injected at start as ''
+        extra_all = extra_all[1:]
+        assert base[0] == ""
+        base = base[1:]
+
+        assert not (set(base) - set(extra_all))  # all base paths are present
+        abs_python_paths = list(OrderedDict((os.path.abspath(ensure_text(i)), None) for i in python_paths).keys())
+        abs_python_paths = [i if case_sensitive else i.lower() for i in abs_python_paths]
+
+        extra_as_python_path = extra_all[: len(abs_python_paths)]
+        assert abs_python_paths == extra_as_python_path  # python paths are there at the start
+
+        non_python_path = extra_all[len(abs_python_paths) :]
+        assert non_python_path == [i for i in base if i not in extra_as_python_path]
+    else:
+        assert base == extra_all