From 543e8bb62ed8e8f91d38d486ad2d0cc0727c8ced Mon Sep 17 00:00:00 2001 From: Martin Hjelmare Date: Sat, 15 Jul 2017 16:25:02 +0200 Subject: [PATCH] Fix check for running inside venv (#8481) * Import and use the function from pip instead of defining it ourselves. * Fix tests. --- homeassistant/scripts/__init__.py | 10 ++++++---- homeassistant/setup.py | 2 +- homeassistant/util/package.py | 10 +++------- tests/test_setup.py | 12 +++++++----- tests/util/test_package.py | 26 +++++++++++++++++++------- 5 files changed, 36 insertions(+), 24 deletions(-) diff --git a/homeassistant/scripts/__init__.py b/homeassistant/scripts/__init__.py index 2324613acfb..f6934aef8f6 100644 --- a/homeassistant/scripts/__init__.py +++ b/homeassistant/scripts/__init__.py @@ -1,15 +1,17 @@ """Home Assistant command line scripts.""" import argparse import importlib +import logging import os import sys -import logging + from typing import List +from homeassistant.bootstrap import mount_local_lib_path from homeassistant.config import get_default_config_dir from homeassistant.const import CONSTRAINT_FILE -from homeassistant.util.package import install_package, is_virtual_env -from homeassistant.bootstrap import mount_local_lib_path +from homeassistant.util.package import ( + install_package, running_under_virtualenv) def run(args: List) -> int: @@ -41,7 +43,7 @@ def run(args: List) -> int: logging.basicConfig(stream=sys.stdout, level=logging.INFO) for req in getattr(script, 'REQUIREMENTS', []): - if is_virtual_env(): + if running_under_virtualenv(): returncode = install_package(req, constraints=os.path.join( os.path.dirname(__file__), os.pardir, CONSTRAINT_FILE)) else: diff --git a/homeassistant/setup.py b/homeassistant/setup.py index 3594399281f..a7083d010e6 100644 --- a/homeassistant/setup.py +++ b/homeassistant/setup.py @@ -77,7 +77,7 @@ def _async_process_requirements(hass: core.HomeAssistant, name: str, def pip_install(mod): """Install packages.""" - if pkg_util.is_virtual_env(): + if pkg_util.running_under_virtualenv(): return pkg_util.install_package( mod, constraints=os.path.join( os.path.dirname(__file__), CONSTRAINT_FILE)) diff --git a/homeassistant/util/package.py b/homeassistant/util/package.py index 4736c1acc1a..a82a50f4e02 100644 --- a/homeassistant/util/package.py +++ b/homeassistant/util/package.py @@ -2,11 +2,12 @@ import asyncio import logging import os +from subprocess import PIPE, Popen import sys import threading -from subprocess import Popen, PIPE from urllib.parse import urlparse +from pip.locations import running_under_virtualenv from typing import Optional import pkg_resources @@ -36,7 +37,7 @@ def install_package(package: str, upgrade: bool=True, if constraints is not None: args += ['--constraint', constraints] if target: - assert not is_virtual_env() + assert not running_under_virtualenv() # This only works if not running in venv args += ['--user'] env['PYTHONUSERBASE'] = os.path.abspath(target) @@ -70,11 +71,6 @@ def check_package_exists(package: str) -> bool: return any(dist in req for dist in env[req.project_name]) -def is_virtual_env() -> bool: - """Return true if environment is a virtual environment.""" - return hasattr(sys, 'real_prefix') - - def _get_user_site(deps_dir: str) -> tuple: """Get arguments and environment for subprocess used in get_user_site.""" env = os.environ.copy() diff --git a/tests/test_setup.py b/tests/test_setup.py index 742df86242f..9a0f85874ad 100644 --- a/tests/test_setup.py +++ b/tests/test_setup.py @@ -204,13 +204,14 @@ class TestSetup: assert 'comp' not in self.hass.config.components @mock.patch('homeassistant.setup.os.path.dirname') - @mock.patch('homeassistant.util.package.sys') + @mock.patch('homeassistant.util.package.running_under_virtualenv', + return_value=True) @mock.patch('homeassistant.util.package.install_package', return_value=True) def test_requirement_installed_in_venv( - self, mock_install, mock_sys, mock_dirname): + self, mock_install, mock_venv, mock_dirname): """Test requirement installed in virtual environment.""" - mock_sys.real_prefix = 'pythonpath' + mock_venv.return_value = True mock_dirname.return_value = 'ha_package_path' self.hass.config.skip_pip = False loader.set_component( @@ -222,11 +223,12 @@ class TestSetup: constraints=os.path.join('ha_package_path', CONSTRAINT_FILE)) @mock.patch('homeassistant.setup.os.path.dirname') - @mock.patch('homeassistant.util.package.sys', spec=object()) + @mock.patch('homeassistant.util.package.running_under_virtualenv', + return_value=False) @mock.patch('homeassistant.util.package.install_package', return_value=True) def test_requirement_installed_in_deps( - self, mock_install, mock_sys, mock_dirname): + self, mock_install, mock_venv, mock_dirname): """Test requirement installed in deps directory.""" mock_dirname.return_value = 'ha_package_path' self.hass.config.skip_pip = False diff --git a/tests/util/test_package.py b/tests/util/test_package.py index 7ca1315c2e1..ade374dad33 100644 --- a/tests/util/test_package.py +++ b/tests/util/test_package.py @@ -66,6 +66,14 @@ def mock_env_copy(): yield env_copy +@pytest.fixture +def mock_venv(): + """Mock homeassistant.util.package.running_under_virtualenv.""" + with patch('homeassistant.util.package.running_under_virtualenv') as mock: + mock.return_value = True + yield mock + + @asyncio.coroutine def mock_async_subprocess(): """Return an async Popen mock.""" @@ -90,7 +98,7 @@ def test_install_existing_package(mock_exists, mock_popen): assert mock_popen.return_value.communicate.call_count == 0 -def test_install(mock_sys, mock_exists, mock_popen, mock_env_copy): +def test_install(mock_sys, mock_exists, mock_popen, mock_env_copy, mock_venv): """Test an install attempt on a package that doesn't exist.""" env = mock_env_copy() assert package.install_package(TEST_NEW_REQ, False) @@ -106,7 +114,8 @@ def test_install(mock_sys, mock_exists, mock_popen, mock_env_copy): assert mock_popen.return_value.communicate.call_count == 1 -def test_install_upgrade(mock_sys, mock_exists, mock_popen, mock_env_copy): +def test_install_upgrade( + mock_sys, mock_exists, mock_popen, mock_env_copy, mock_venv): """Test an upgrade attempt on a package.""" env = mock_env_copy() assert package.install_package(TEST_NEW_REQ) @@ -122,11 +131,13 @@ def test_install_upgrade(mock_sys, mock_exists, mock_popen, mock_env_copy): assert mock_popen.return_value.communicate.call_count == 1 -def test_install_target(mock_sys, mock_exists, mock_popen, mock_env_copy): +def test_install_target( + mock_sys, mock_exists, mock_popen, mock_env_copy, mock_venv): """Test an install with a target.""" target = 'target_folder' env = mock_env_copy() env['PYTHONUSERBASE'] = os.path.abspath(target) + mock_venv.return_value = False mock_sys.platform = 'linux' args = [ mock_sys.executable, '-m', 'pip', 'install', '--quiet', @@ -142,15 +153,15 @@ def test_install_target(mock_sys, mock_exists, mock_popen, mock_env_copy): assert mock_popen.return_value.communicate.call_count == 1 -def test_install_target_venv(mock_sys, mock_exists, mock_popen, mock_env_copy): +def test_install_target_venv( + mock_sys, mock_exists, mock_popen, mock_env_copy, mock_venv): """Test an install with a target in a virtual environment.""" target = 'target_folder' - mock_sys.real_prefix = '/usr' with pytest.raises(AssertionError): package.install_package(TEST_NEW_REQ, False, target=target) -def test_install_error(caplog, mock_sys, mock_exists, mock_popen): +def test_install_error(caplog, mock_sys, mock_exists, mock_popen, mock_venv): """Test an install with a target.""" caplog.set_level(logging.WARNING) mock_popen.return_value.returncode = 1 @@ -160,7 +171,8 @@ def test_install_error(caplog, mock_sys, mock_exists, mock_popen): assert record.levelname == 'ERROR' -def test_install_constraint(mock_sys, mock_exists, mock_popen, mock_env_copy): +def test_install_constraint( + mock_sys, mock_exists, mock_popen, mock_env_copy, mock_venv): """Test install with constraint file on not installed package.""" env = mock_env_copy() constraints = 'constraints_file.txt'