From 1b9c9e2901252d21f208eb86b062cb0f7bcba428 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Mon, 7 Oct 2019 12:50:12 +0530 Subject: [PATCH 1/8] Factor out running_under_virtualenv conditionals Why: This would allow for use in an updated `virtualenv_no_global` that supports PEP 405 virtual environments. --- src/pip/_internal/utils/virtualenv.py | 28 ++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/pip/_internal/utils/virtualenv.py b/src/pip/_internal/utils/virtualenv.py index 380db1c3281..a83b3e951c9 100644 --- a/src/pip/_internal/utils/virtualenv.py +++ b/src/pip/_internal/utils/virtualenv.py @@ -3,20 +3,30 @@ import sys -def running_under_virtualenv(): +def _running_under_venv(): # type: () -> bool + """Checks if sys.base_prefix and sys.prefix match. + + This handles PEP 405 compliant virtual environments. """ - Return True if we're running inside a virtualenv, False otherwise. + return sys.prefix != getattr(sys, "base_prefix", sys.prefix) + +def _running_under_regular_virtualenv(): + # type: () -> bool + """Checks if sys.real_prefix is set. + + This handles virtual environments created with pypa's virtualenv. """ - if hasattr(sys, 'real_prefix'): - # pypa/virtualenv case - return True - elif sys.prefix != getattr(sys, "base_prefix", sys.prefix): - # PEP 405 venv - return True + # pypa/virtualenv case + return hasattr(sys, 'real_prefix') + - return False +def running_under_virtualenv(): + # type: () -> bool + """Return a boolean, whether running under a virtual environment. + """ + return _running_under_venv() or _running_under_regular_virtualenv() def virtualenv_no_global(): From fd7c9b7ce4cf8c82499d53d401d995e71f108748 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Mon, 7 Oct 2019 13:39:01 +0530 Subject: [PATCH 2/8] Refactor virtualenv_no_global Why: This change makes it easier to introduce handling for PEP 405 based virtual environment's global site-package exclusion logic --- src/pip/_internal/utils/virtualenv.py | 35 ++++++++++++++++++--------- tests/unit/test_utils_virtualenv.py | 18 +++++++++----- 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/src/pip/_internal/utils/virtualenv.py b/src/pip/_internal/utils/virtualenv.py index a83b3e951c9..3bc7cd1e382 100644 --- a/src/pip/_internal/utils/virtualenv.py +++ b/src/pip/_internal/utils/virtualenv.py @@ -1,7 +1,10 @@ -import os.path +import logging +import os import site import sys +logger = logging.getLogger(__name__) + def _running_under_venv(): # type: () -> bool @@ -29,16 +32,26 @@ def running_under_virtualenv(): return _running_under_venv() or _running_under_regular_virtualenv() -def virtualenv_no_global(): +def _no_global_under_regular_virtualenv(): # type: () -> bool + """Check if "no-global-site-packages.txt" exists beside site.py + + This mirrors logic in pypa/virtualenv for determining whether system + site-packages are visible in the virtual environment. """ - Return True if in a venv and no system site packages. - """ - # this mirrors the logic in virtualenv.py for locating the - # no-global-site-packages.txt file site_mod_dir = os.path.dirname(os.path.abspath(site.__file__)) - no_global_file = os.path.join(site_mod_dir, 'no-global-site-packages.txt') - if running_under_virtualenv() and os.path.isfile(no_global_file): - return True - else: - return False + no_global_site_packages_file = os.path.join( + site_mod_dir, 'no-global-site-packages.txt', + ) + return os.path.exists(no_global_site_packages_file) + + +def virtualenv_no_global(): + # type: () -> bool + """Returns a boolean, whether running in venv with no system site-packages. + """ + + if _running_under_regular_virtualenv(): + return _no_global_under_regular_virtualenv() + + return False diff --git a/tests/unit/test_utils_virtualenv.py b/tests/unit/test_utils_virtualenv.py index 80e30404bda..98071e82d6f 100644 --- a/tests/unit/test_utils_virtualenv.py +++ b/tests/unit/test_utils_virtualenv.py @@ -32,20 +32,26 @@ def test_running_under_virtualenv( @pytest.mark.parametrize( - "running_under_virtualenv, no_global_file, expected", [ + "under_virtualenv, no_global_file, expected", [ (False, False, False), (False, True, False), (True, False, False), (True, True, True), ], ) -def test_virtualenv_no_global( - monkeypatch, tmpdir, - running_under_virtualenv, no_global_file, expected): +def test_virtualenv_no_global_with_regular_virtualenv( + monkeypatch, + tmpdir, + under_virtualenv, + no_global_file, + expected, +): monkeypatch.setattr(site, '__file__', tmpdir / 'site.py') monkeypatch.setattr( - virtualenv, 'running_under_virtualenv', - lambda: running_under_virtualenv) + virtualenv, '_running_under_regular_virtualenv', + lambda: under_virtualenv, + ) if no_global_file: (tmpdir / 'no-global-site-packages.txt').touch() + assert virtualenv.virtualenv_no_global() == expected From 1aee0eb5cbaa48b5a193b942e38a15c4cf376c05 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Mon, 7 Oct 2019 15:36:29 +0530 Subject: [PATCH 3/8] Correctly ignore system site-packages for venv Why: PEP 405 virtual environments have a different mechanism for ignoring system site-packages in virtual environments. --- src/pip/_internal/utils/virtualenv.py | 48 +++++++++++++++++ tests/unit/test_utils_virtualenv.py | 78 +++++++++++++++++++++++++++ 2 files changed, 126 insertions(+) diff --git a/src/pip/_internal/utils/virtualenv.py b/src/pip/_internal/utils/virtualenv.py index 3bc7cd1e382..1be69a96d72 100644 --- a/src/pip/_internal/utils/virtualenv.py +++ b/src/pip/_internal/utils/virtualenv.py @@ -3,6 +3,11 @@ import site import sys +from pip._internal.utils.typing import MYPY_CHECK_RUNNING + +if MYPY_CHECK_RUNNING: + from typing import Optional, List + logger = logging.getLogger(__name__) @@ -32,6 +37,46 @@ def running_under_virtualenv(): return _running_under_venv() or _running_under_regular_virtualenv() +def _get_pyvenv_cfg_lines(): + # type: () -> Optional[List[str]] + """Reads {sys.prefix}/pyvenv.cfg and returns its contents as list of lines + + Returns None, if it could not read/access the file. + """ + pyvenv_cfg_file = os.path.join(sys.prefix, 'pyvenv.cfg') + try: + with open(pyvenv_cfg_file) as f: + return f.read().splitlines() # avoids trailing newlines + except OSError: + return None + + +def _no_global_under_venv(): + # type: () -> bool + """Check `{sys.prefix}/pyvenv.cfg` for system site-packages inclusion + + PEP 405 specifies that when system site-packages are not supposed to be + visible from a virtual environment, `pyvenv.cfg` must contain the following + line: + + include-system-site-packages = false + + Additionally, log a warning if accessing the file fails. + """ + cfg_lines = _get_pyvenv_cfg_lines() + if cfg_lines is None: + # We're not in a "sane" venv, so assume there is no system + # site-packages access (since that's PEP 405's default state). + logger.warning( + "Could not access 'pyvenv.cfg' despite a virtual environment " + "being active. Assuming global site-packages is not accessible " + "in this environment." + ) + return True + + return "include-system-site-packages = false" in cfg_lines + + def _no_global_under_regular_virtualenv(): # type: () -> bool """Check if "no-global-site-packages.txt" exists beside site.py @@ -54,4 +99,7 @@ def virtualenv_no_global(): if _running_under_regular_virtualenv(): return _no_global_under_regular_virtualenv() + if _running_under_venv(): + return _no_global_under_venv() + return False diff --git a/tests/unit/test_utils_virtualenv.py b/tests/unit/test_utils_virtualenv.py index 98071e82d6f..625539d7617 100644 --- a/tests/unit/test_utils_virtualenv.py +++ b/tests/unit/test_utils_virtualenv.py @@ -1,3 +1,4 @@ +import logging import site import sys @@ -46,6 +47,8 @@ def test_virtualenv_no_global_with_regular_virtualenv( no_global_file, expected, ): + monkeypatch.setattr(virtualenv, '_running_under_venv', lambda: False) + monkeypatch.setattr(site, '__file__', tmpdir / 'site.py') monkeypatch.setattr( virtualenv, '_running_under_regular_virtualenv', @@ -55,3 +58,78 @@ def test_virtualenv_no_global_with_regular_virtualenv( (tmpdir / 'no-global-site-packages.txt').touch() assert virtualenv.virtualenv_no_global() == expected + + +@pytest.mark.parametrize( + "pyvenv_cfg_lines, under_venv, expected, expect_warning", [ + (None, False, False, False), + (None, True, True, True), # this has a warning. + ( + [ + "home = ", + "include-system-site-packages = true", + "version = ", + ], + True, + False, + False, + ), + ( + [ + "home = ", + "include-system-site-packages = false", + "version = ", + ], + True, + True, + False, + ), + ], +) +def test_virtualenv_no_global_with_pep_405_virtual_environment( + monkeypatch, + caplog, + pyvenv_cfg_lines, + under_venv, + expected, + expect_warning, +): + monkeypatch.setattr( + virtualenv, '_running_under_regular_virtualenv', lambda: False + ) + monkeypatch.setattr( + virtualenv, '_get_pyvenv_cfg_lines', lambda: pyvenv_cfg_lines + ) + monkeypatch.setattr(virtualenv, '_running_under_venv', lambda: under_venv) + + with caplog.at_level(logging.WARNING): + assert virtualenv.virtualenv_no_global() == expected + + if expect_warning: + assert caplog.records + + # Check for basic information + message = caplog.records[-1].getMessage().lower() + assert "could not access 'pyvenv.cfg'" in message + assert "assuming global site-packages is not accessible" in message + + +@pytest.mark.parametrize( + "contents, expected", [ + (None, None), + ("", []), + ("a = b\nc = d\n", ["a = b", "c = d"]), + ("a = b\nc = d", ["a = b", "c = d"]), # no trailing newlines + ] +) +def test_get_pyvenv_cfg_lines_for_pep_405_virtual_environment( + monkeypatch, + tmpdir, + contents, + expected, +): + monkeypatch.setattr(sys, 'prefix', str(tmpdir)) + if contents is not None: + tmpdir.joinpath('pyvenv.cfg').write_text(contents) + + assert virtualenv._get_pyvenv_cfg_lines() == expected From 762ffd5b5ea527c9fcd4d74e8993ca27a9a3197c Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Mon, 7 Oct 2019 15:56:20 +0530 Subject: [PATCH 4/8] :newspaper: --- news/5702.bugfix | 1 + news/7155.bugfix | 1 + 2 files changed, 2 insertions(+) create mode 100644 news/5702.bugfix create mode 100644 news/7155.bugfix diff --git a/news/5702.bugfix b/news/5702.bugfix new file mode 100644 index 00000000000..2541d745ed7 --- /dev/null +++ b/news/5702.bugfix @@ -0,0 +1 @@ +Correctly handle system site-packages, in virtual environments created with venv (PEP 405). diff --git a/news/7155.bugfix b/news/7155.bugfix new file mode 100644 index 00000000000..2541d745ed7 --- /dev/null +++ b/news/7155.bugfix @@ -0,0 +1 @@ +Correctly handle system site-packages, in virtual environments created with venv (PEP 405). From fe9ae3ba7552e230431cc39a34fa5e5c1e042b25 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Mon, 7 Oct 2019 16:00:59 +0530 Subject: [PATCH 5/8] Did someone ask why I dislike Python 2? --- src/pip/_internal/utils/virtualenv.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/pip/_internal/utils/virtualenv.py b/src/pip/_internal/utils/virtualenv.py index 1be69a96d72..8a195123bdc 100644 --- a/src/pip/_internal/utils/virtualenv.py +++ b/src/pip/_internal/utils/virtualenv.py @@ -1,3 +1,5 @@ +from __future__ import absolute_import + import logging import os import site @@ -47,7 +49,7 @@ def _get_pyvenv_cfg_lines(): try: with open(pyvenv_cfg_file) as f: return f.read().splitlines() # avoids trailing newlines - except OSError: + except IOError: return None From 5332ec57c6673609c2758e268c88df6a6454c635 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Tue, 8 Oct 2019 12:59:08 +0530 Subject: [PATCH 6/8] Apply suggestions from code review --- src/pip/_internal/utils/virtualenv.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pip/_internal/utils/virtualenv.py b/src/pip/_internal/utils/virtualenv.py index 8a195123bdc..ffc401ad104 100644 --- a/src/pip/_internal/utils/virtualenv.py +++ b/src/pip/_internal/utils/virtualenv.py @@ -8,7 +8,7 @@ from pip._internal.utils.typing import MYPY_CHECK_RUNNING if MYPY_CHECK_RUNNING: - from typing import Optional, List + from typing import List, Optional logger = logging.getLogger(__name__) @@ -34,7 +34,7 @@ def _running_under_regular_virtualenv(): def running_under_virtualenv(): # type: () -> bool - """Return a boolean, whether running under a virtual environment. + """Return True if we're running inside a virtualenv, False otherwise. """ return _running_under_venv() or _running_under_regular_virtualenv() From 57d34e0d9b41d14ae91942b97ac8d1eb1cd6d162 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Tue, 5 Nov 2019 14:54:52 +0530 Subject: [PATCH 7/8] Allow whitespace in pyvenv.cfg --- src/pip/_internal/utils/virtualenv.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/pip/_internal/utils/virtualenv.py b/src/pip/_internal/utils/virtualenv.py index ffc401ad104..d81e6ac54bb 100644 --- a/src/pip/_internal/utils/virtualenv.py +++ b/src/pip/_internal/utils/virtualenv.py @@ -2,6 +2,7 @@ import logging import os +import re import site import sys @@ -11,6 +12,9 @@ from typing import List, Optional logger = logging.getLogger(__name__) +_INCLUDE_SYSTEM_SITE_PACKAGES_REGEX = re.compile( + r"include-system-site-packages\s*=\s*(?Ptrue|false)" +) def _running_under_venv(): @@ -76,7 +80,11 @@ def _no_global_under_venv(): ) return True - return "include-system-site-packages = false" in cfg_lines + for line in cfg_lines: + match = _INCLUDE_SYSTEM_SITE_PACKAGES_REGEX.match(line) + if match is not None and match.group('value') == 'false': + return True + return False def _no_global_under_regular_virtualenv(): From 8981895b5e34de1be2a73e5fff77879c45908700 Mon Sep 17 00:00:00 2001 From: Pradyun Gedam Date: Tue, 5 Nov 2019 21:05:08 +0530 Subject: [PATCH 8/8] Skip all failing tests when using venv Our isolation logic for venv isn't correct and that is causing these tests to fail. The culprits for this are: tests/lib/venv.py::VirtualEnvironment.user_site_packages tests/lib/venv.py::VirtualEnvironment.sitecustomize Both these together are supposed to create an environment to isolate the tests. However, they were written for virtualenv and make assumptions that are not true for environments created with venv. Until we can fix VirtualEnvironment to properly isolate the test from the underlying test environment when using venv, these tests will continue to fail. This is blocking an important bugfix for users facing issues with since pip is installing packages into `--user` when run in a venv, even when `--user` isn't visible from that environment. As a temporary band-aid for this problem, I'm skipping these tests to unblock us from shipping the bugfix for the aforementioned issue. The test isolation logic should be fixed to work for venv. Once such a fix is made, this commit should be reverted. --- tests/functional/test_freeze.py | 2 ++ tests/functional/test_install.py | 2 ++ tests/functional/test_install_reqs.py | 1 + tests/functional/test_install_user.py | 9 ++++++++- tests/functional/test_install_wheel.py | 1 + tests/functional/test_list.py | 3 +++ tests/functional/test_uninstall_user.py | 1 + tests/unit/test_build_env.py | 1 + 8 files changed, 19 insertions(+), 1 deletion(-) diff --git a/tests/functional/test_freeze.py b/tests/functional/test_freeze.py index a4ff73d5c61..d13c931d0ef 100644 --- a/tests/functional/test_freeze.py +++ b/tests/functional/test_freeze.py @@ -703,6 +703,7 @@ def test_freeze_with_requirement_option_package_repeated_multi_file(script): @pytest.mark.network +@pytest.mark.incompatible_with_test_venv def test_freeze_user(script, virtualenv, data): """ Testing freeze with --user, first we have to install some stuff. @@ -733,6 +734,7 @@ def test_freeze_path(tmpdir, script, data): _check_output(result.stdout, expected) +@pytest.mark.incompatible_with_test_venv def test_freeze_path_exclude_user(tmpdir, script, data): """ Test freeze with --path and make sure packages from --user are not picked diff --git a/tests/functional/test_install.py b/tests/functional/test_install.py index 3194b165384..364893eeca6 100644 --- a/tests/functional/test_install.py +++ b/tests/functional/test_install.py @@ -124,6 +124,7 @@ def test_pep518_allows_missing_requires(script, data, common_wheels): assert result.files_created +@pytest.mark.incompatible_with_test_venv def test_pep518_with_user_pip(script, pip_src, data, common_wheels): """ Check that build dependencies are installed into the build @@ -1593,6 +1594,7 @@ def test_target_install_ignores_distutils_config_install_prefix(script): assert relative_script_base not in result.files_created +@pytest.mark.incompatible_with_test_venv def test_user_config_accepted(script): # user set in the config file is parsed as 0/1 instead of True/False. # Check that this doesn't cause a problem. diff --git a/tests/functional/test_install_reqs.py b/tests/functional/test_install_reqs.py index a653e0b2fb2..2022e1fee53 100644 --- a/tests/functional/test_install_reqs.py +++ b/tests/functional/test_install_reqs.py @@ -227,6 +227,7 @@ def test_install_local_with_subdirectory(script): result.assert_installed('version_subpkg.py', editable=False) +@pytest.mark.incompatible_with_test_venv def test_wheel_user_with_prefix_in_pydistutils_cfg( script, data, with_wheel): if os.name == 'posix': diff --git a/tests/functional/test_install_user.py b/tests/functional/test_install_user.py index 1ac644609e9..03725fc3229 100644 --- a/tests/functional/test_install_user.py +++ b/tests/functional/test_install_user.py @@ -27,6 +27,7 @@ def dist_in_site_packages(dist): class Tests_UserSite: @pytest.mark.network + @pytest.mark.incompatible_with_test_venv def test_reset_env_system_site_packages_usersite(self, script): """ Check user site works as expected. @@ -42,6 +43,7 @@ def test_reset_env_system_site_packages_usersite(self, script): @pytest.mark.network @need_svn + @pytest.mark.incompatible_with_test_venv def test_install_subversion_usersite_editable_with_distribute( self, script, tmpdir): """ @@ -55,6 +57,7 @@ def test_install_subversion_usersite_editable_with_distribute( ) result.assert_installed('INITools', use_user_site=True) + @pytest.mark.incompatible_with_test_venv def test_install_from_current_directory_into_usersite( self, script, data, with_wheel): """ @@ -75,7 +78,6 @@ def test_install_from_current_directory_into_usersite( ) assert dist_info_folder in result.files_created - @pytest.mark.incompatible_with_test_venv def test_install_user_venv_nositepkgs_fails(self, virtualenv, script, data): """ @@ -96,6 +98,7 @@ def test_install_user_venv_nositepkgs_fails(self, virtualenv, ) @pytest.mark.network + @pytest.mark.incompatible_with_test_venv def test_install_user_conflict_in_usersite(self, script): """ Test user install with conflict in usersite updates usersite. @@ -119,6 +122,7 @@ def test_install_user_conflict_in_usersite(self, script): assert not isfile(initools_v3_file), initools_v3_file @pytest.mark.network + @pytest.mark.incompatible_with_test_venv def test_install_user_conflict_in_globalsite(self, virtualenv, script): """ Test user install with conflict in global site ignores site and @@ -149,6 +153,7 @@ def test_install_user_conflict_in_globalsite(self, virtualenv, script): assert isdir(initools_folder) @pytest.mark.network + @pytest.mark.incompatible_with_test_venv def test_upgrade_user_conflict_in_globalsite(self, virtualenv, script): """ Test user install/upgrade with conflict in global site ignores site and @@ -178,6 +183,7 @@ def test_upgrade_user_conflict_in_globalsite(self, virtualenv, script): assert isdir(initools_folder) @pytest.mark.network + @pytest.mark.incompatible_with_test_venv def test_install_user_conflict_in_globalsite_and_usersite( self, virtualenv, script): """ @@ -214,6 +220,7 @@ def test_install_user_conflict_in_globalsite_and_usersite( assert isdir(initools_folder) @pytest.mark.network + @pytest.mark.incompatible_with_test_venv def test_install_user_in_global_virtualenv_with_conflict_fails( self, script): """ diff --git a/tests/functional/test_install_wheel.py b/tests/functional/test_install_wheel.py index 2dfec01eedb..e218d6e7f17 100644 --- a/tests/functional/test_install_wheel.py +++ b/tests/functional/test_install_wheel.py @@ -233,6 +233,7 @@ def test_wheel_record_lines_in_deterministic_order(script, data): assert record_lines == sorted(record_lines) +@pytest.mark.incompatible_with_test_venv def test_install_user_wheel(script, data, with_wheel): """ Test user install from wheel (that has a script) diff --git a/tests/functional/test_list.py b/tests/functional/test_list.py index a863c42c91a..53f4152c2b7 100644 --- a/tests/functional/test_list.py +++ b/tests/functional/test_list.py @@ -94,6 +94,7 @@ def test_local_columns_flag(simple_script): @pytest.mark.network +@pytest.mark.incompatible_with_test_venv def test_user_flag(script, data): """ Test the behavior of --user flag in the list command @@ -110,6 +111,7 @@ def test_user_flag(script, data): @pytest.mark.network +@pytest.mark.incompatible_with_test_venv def test_user_columns_flag(script, data): """ Test the behavior of --user --format=columns flags in the list command @@ -502,6 +504,7 @@ def test_list_path(tmpdir, script, data): assert {'name': 'simple', 'version': '2.0'} in json_result +@pytest.mark.incompatible_with_test_venv def test_list_path_exclude_user(tmpdir, script, data): """ Test list with --path and make sure packages from --user are not picked diff --git a/tests/functional/test_uninstall_user.py b/tests/functional/test_uninstall_user.py index f99f3f21c7d..58079a293a8 100644 --- a/tests/functional/test_uninstall_user.py +++ b/tests/functional/test_uninstall_user.py @@ -9,6 +9,7 @@ from tests.lib import assert_all_changes, pyversion +@pytest.mark.incompatible_with_test_venv class Tests_UninstallUserSite: @pytest.mark.network diff --git a/tests/unit/test_build_env.py b/tests/unit/test_build_env.py index 9db08a124dd..ff3b2e90cef 100644 --- a/tests/unit/test_build_env.py +++ b/tests/unit/test_build_env.py @@ -162,6 +162,7 @@ def test_build_env_overlay_prefix_has_priority(script): assert result.stdout.strip() == '2.0', str(result) +@pytest.mark.incompatible_with_test_venv def test_build_env_isolation(script): # Create dummy `pkg` wheel.