Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use python3's venv instead of virtualenv #2152

Merged
merged 6 commits into from
Apr 25, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,7 @@ jobs:
# See also: https://github.com/travis-ci/travis-ci/issues/8589
- type -t deactivate && deactivate || true
- export PATH=/opt/python/3.7/bin:$PATH
# Install tox & virtualenv
# Note: venv/virtualenv are both used by tests/test_pythonpackage.py
- pip3.7 install -U virtualenv
# Install tox
- pip3.7 install tox>=2.0
# Install coveralls & dependencies
# Note: pyOpenSSL needed to send the coveralls reports
Expand Down
2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ RUN dpkg --add-architecture i386 \
libncurses5:i386 \
libpangox-1.0-0:i386 \
libpangoxft-1.0-0:i386 \
libssl-dev \
libstdc++6:i386 \
libtool \
openjdk-8-jdk \
Expand All @@ -77,7 +78,6 @@ RUN dpkg --add-architecture i386 \
python3-venv \
sudo \
unzip \
virtualenv \
wget \
zip \
zlib1g-dev \
Expand Down
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ ANDROID_NDK_HOME ?= $(HOME)/.android/android-ndk
all: virtualenv

$(VIRTUAL_ENV):
virtualenv --python=$(PYTHON_WITH_VERSION) $(VIRTUAL_ENV)
python3 -m venv $(VIRTUAL_ENV)
$(PIP) install Cython==0.28.6
$(PIP) install -e .

Expand Down
22 changes: 5 additions & 17 deletions pythonforandroid/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
import subprocess

from pythonforandroid.util import (
current_directory, ensure_dir, get_virtualenv_executable,
BuildInterruptingException
current_directory, ensure_dir,
BuildInterruptingException,
)
from pythonforandroid.logger import (info, warning, info_notify, info_main, shprint)
from pythonforandroid.archs import ArchARM, ArchARMv7_a, ArchAarch_64, Archx86, Archx86_64
Expand Down Expand Up @@ -357,13 +357,6 @@ def prepare_build_environment(self,

check_ndk_api(ndk_api, self.android_api)

virtualenv = get_virtualenv_executable()
if virtualenv is None:
raise IOError('Couldn\'t find a virtualenv executable, '
'you must install this to use p4a.')
self.virtualenv = virtualenv
info('Found virtualenv at {}'.format(virtualenv))

# path to some tools
self.ccache = sh.which("ccache")
if not self.ccache:
Expand Down Expand Up @@ -765,15 +758,10 @@ def run_pymodules_install(ctx, modules, project_dir=None,
info('Will process project install, if it fails then the '
'project may not be compatible for Android install.')

venv = sh.Command(ctx.virtualenv)
# Use our hostpython to create the virtualenv
host_python = sh.Command(ctx.hostpython)
with current_directory(join(ctx.build_dir)):
shprint(venv,
'--python=python{}'.format(
ctx.python_recipe.major_minor_version_string.
partition(".")[0]
),
'venv'
)
shprint(host_python, '-m', 'venv', 'venv')

# Prepare base environment and upgrade pip:
base_env = copy.copy(os.environ)
Expand Down
24 changes: 11 additions & 13 deletions pythonforandroid/pythonpackage.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,26 +33,23 @@
"""


from io import open # needed for python 2
import functools
import os
from pep517.envbuild import BuildEnvironment
from pep517.wrappers import Pep517HookCaller
import pytoml
import shutil
import subprocess
import sys
import tarfile
import tempfile
import textwrap
import time
try:
from urllib.parse import urlparse
from urllib.parse import unquote as urlunquote
except ImportError: # Python 2...
from urlparse import urlparse
from urlparse import unquote as urlunquote
import zipfile
from io import open # needed for python 2
from urllib.parse import unquote as urlunquote
from urllib.parse import urlparse

import toml
from pep517.envbuild import BuildEnvironment
from pep517.wrappers import Pep517HookCaller


def transform_dep_for_pip(dependency):
Expand Down Expand Up @@ -111,7 +108,8 @@ def extract_metainfo_files_from_package(
if is_filesystem_path(package):
shutil.copytree(
parse_as_folder_reference(package),
os.path.join(temp_folder, "package")
os.path.join(temp_folder, "package"),
ignore=shutil.ignore_patterns(".tox")
)
package = os.path.join(temp_folder, "package")

Expand Down Expand Up @@ -486,7 +484,7 @@ def _extract_metainfo_files_from_package_unsafe(

# Get build backend and requirements from pyproject.toml:
with open(os.path.join(path, 'pyproject.toml')) as f:
build_sys = pytoml.load(f)['build-system']
build_sys = toml.load(f)['build-system']
backend = build_sys["build-backend"]
build_requires.extend(build_sys["requires"])

Expand Down Expand Up @@ -630,7 +628,7 @@ def _extract_info_from_package(dependency,
) and include_build_requirements:
# Read build system from pyproject.toml file: (PEP518)
with open(os.path.join(output_folder, 'pyproject.toml')) as f:
build_sys = pytoml.load(f)['build-system']
build_sys = toml.load(f)['build-system']
if "requires" in build_sys:
requirements += build_sys["requires"]
elif include_build_requirements:
Expand Down
12 changes: 0 additions & 12 deletions pythonforandroid/util.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import contextlib
from os.path import exists, join
from os import getcwd, chdir, makedirs, walk, uname
import sh
import shutil
from fnmatch import fnmatch
from tempfile import mkdtemp
Expand Down Expand Up @@ -55,17 +54,6 @@ def ensure_dir(filename):
makedirs(filename)


def get_virtualenv_executable():
virtualenv = None
if virtualenv is None:
virtualenv = sh.which('virtualenv2')
if virtualenv is None:
virtualenv = sh.which('virtualenv-2.7')
if virtualenv is None:
virtualenv = sh.which('virtualenv')
return virtualenv


def walk_valid_filens(base_dir, invalid_dir_names, invalid_file_patterns):
"""Recursively walks all the files and directories in ``dirn``,
ignoring directories that match any pattern in ``invalid_dirns``
Expand Down
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
install_reqs = [
'appdirs', 'colorama>=0.3.3', 'jinja2', 'six',
'enum34; python_version<"3.4"', 'sh>=1.10; sys_platform!="nt"',
'pep517<0.7.0"', 'pytoml', 'virtualenv<20'
'pep517<0.7.0"', 'toml',
]
# (pep517, pytoml and virtualenv are used by pythonpackage.py)

Expand Down
42 changes: 4 additions & 38 deletions tests/test_pythonpackage_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
"""

import os
import pytest
import shutil
import sys
import subprocess
Expand Down Expand Up @@ -279,43 +278,6 @@ def test_systemwide_python(self):
else:
raise

def test_virtualenv(self):
""" Verifies that _get_system_python_executable() works correctly
if called with a python binary as found inside a virtualenv.
"""

# Get system-wide python bin seen from here first:
pybin = _get_system_python_executable()
# (this call was not a test, we really just need the path here)

test_dir = tempfile.mkdtemp()
try:
# Check that in a virtualenv, the system-wide python is returned:
subprocess.check_output([
pybin, "-m", "virtualenv",
"--python=" + str(sys.executable),
"--",
os.path.join(test_dir, "virtualenv")
])
subprocess.check_output([
os.path.join(test_dir, "virtualenv", "bin", "pip"),
"install", "-U", "pip"
])
subprocess.check_output([
os.path.join(test_dir, "virtualenv", "bin", "pip"),
"install", "-U", "pep517<0.7.0"
])
sys_python_path = self.run__get_system_python_executable(
os.path.join(test_dir, "virtualenv", "bin", "python")
)
assert os.path.normpath(sys_python_path).startswith(
os.path.normpath(pybin)
)
finally:
shutil.rmtree(test_dir)

@pytest.mark.skipif(int(sys.version.partition(".")[0]) < 3,
reason="venv is python 3 only")
def test_venv(self):
""" Verifies that _get_system_python_executable() works correctly
in a 'venv' (Python 3 only feature).
Expand All @@ -340,6 +302,10 @@ def test_venv(self):
os.path.join(test_dir, "venv", "bin", "pip"),
"install", "-U", "pep517<0.7.0"
])
subprocess.check_output([
os.path.join(test_dir, "venv", "bin", "pip"),
"install", "-U", "toml"
])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit out of scope of this PR, but I'm thinking why don't we pip install -e then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I suppose that you are speaking of pip install -e . right?

In this case...why should we? we don't need all p4a dependencies for this test, and the venv will be destroyed after is passed. Also, considering that we are speaking of one of the slowest tests of p4a, IMHO, better to try to save some time 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense 👍

sys_python_path = self.run__get_system_python_executable(
os.path.join(test_dir, "venv", "bin", "python")
)
Expand Down
40 changes: 0 additions & 40 deletions tests/test_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,46 +70,6 @@ def test_current_directory_exception(self):
):
pass

@mock.patch("pythonforandroid.util.sh.which")
def test_get_virtualenv_executable(self, mock_sh_which):
"""
Test method :meth:`~pythonforandroid.util.get_virtualenv_executable`.
In here we test:

- that all calls to `sh.which` are performed, so we expect the
first two `sh.which` calls should be None and the last one should
return the expected virtualenv (the python3 one)
- that we don't have virtualenv installed, so all calls to
`sh.which` should return None
"""
expected_venv = os.path.join(
os.path.expanduser("~"), ".local/bin/virtualenv"
)
mock_sh_which.side_effect = [None, None, expected_venv]
self.assertEqual(util.get_virtualenv_executable(), expected_venv)
mock_sh_which.assert_has_calls(
[
mock.call("virtualenv2"),
mock.call("virtualenv-2.7"),
mock.call("virtualenv"),
]
)
self.assertEqual(mock_sh_which.call_count, 3)
mock_sh_which.reset_mock()

# Now test that we don't have virtualenv installed, so all calls to
# `sh.which` should return None
mock_sh_which.side_effect = [None, None, None]
self.assertIsNone(util.get_virtualenv_executable())
self.assertEqual(mock_sh_which.call_count, 3)
mock_sh_which.assert_has_calls(
[
mock.call("virtualenv2"),
mock.call("virtualenv-2.7"),
mock.call("virtualenv"),
]
)

@mock.patch("pythonforandroid.util.walk")
def test_walk_valid_filens(self, mock_walk):
"""
Expand Down
1 change: 0 additions & 1 deletion tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ basepython = python3
[testenv]
deps =
pytest
virtualenv
py3: coveralls
backports.tempfile
# posargs will be replaced by the tox args, so you can override pytest
Expand Down