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

Drop Python 2 support #1918

Merged
merged 12 commits into from
Aug 4, 2019
2 changes: 1 addition & 1 deletion pythonforandroid/bdistapk.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def finalize_options(self):
def run(self):
self.prepare_build_dir()

from pythonforandroid.toolchain import main
from pythonforandroid.entrypoints import main
sys.argv[1] = 'apk'
main()

Expand Down
20 changes: 20 additions & 0 deletions pythonforandroid/entrypoints.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
from pythonforandroid.recommendations import check_python_version
from pythonforandroid.util import BuildInterruptingException, handle_build_exception


def main():
"""
Main entrypoint for running python-for-android as a script.
"""

try:
# Check the Python version before importing anything heavier than
# the util functions. This lets us provide a nice message about
# incompatibility rather than having the interpreter crash if it
# reaches unsupported syntax from a newer Python version.
check_python_version()

from pythonforandroid.toolchain import ToolchainCL
ToolchainCL()
except BuildInterruptingException as exc:
handle_build_exception(exc)
2 changes: 2 additions & 0 deletions pythonforandroid/logger.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
from math import log10
from collections import defaultdict
from colorama import Style as Colo_Style, Fore as Colo_Fore

# six import left for Python 2 compatibility during initial Python version check
import six

# This codecs change fixes a bug with log output, but crashes under python3
Expand Down
7 changes: 6 additions & 1 deletion pythonforandroid/recipes/python2/__init__.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from os.path import join, exists
from pythonforandroid.recipe import Recipe
from pythonforandroid.python import GuestPythonRecipe
from pythonforandroid.logger import shprint
from pythonforandroid.logger import shprint, warning
import sh


Expand Down Expand Up @@ -57,6 +57,11 @@ def prebuild_arch(self, arch):
self.apply_patch(join('patches', 'enable-openssl.patch'), arch.arch)
shprint(sh.touch, patch_mark)

def build_arch(self, arch):
warning('DEPRECATION: Support for the Python 2 recipe will be '
'removed in 2020, please upgrade to Python 3.')
super().build_arch(arch)

def set_libs_flags(self, env, arch):
env = super(Python2Recipe, self).set_libs_flags(env, arch)
if 'libffi' in self.ctx.recipe_build_order:
Expand Down
34 changes: 34 additions & 0 deletions pythonforandroid/recommendations.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
"""Simple functions for checking dependency versions."""

import sys
from distutils.version import LooseVersion
from os.path import join

from pythonforandroid.logger import info, warning
from pythonforandroid.util import BuildInterruptingException

Expand Down Expand Up @@ -182,3 +184,35 @@ def check_ndk_api(ndk_api, android_api):

if ndk_api < MIN_NDK_API:
warning(OLD_NDK_API_MESSAGE)


MIN_PYTHON_MAJOR_VERSION = 3
MIN_PYTHON_MINOR_VERSION = 4
MIN_PYTHON_VERSION = LooseVersion('{major}.{minor}'.format(major=MIN_PYTHON_MAJOR_VERSION,
minor=MIN_PYTHON_MINOR_VERSION))
PY2_ERROR_TEXT = (
'python-for-android no longer supports running under Python 2. Either upgrade to '
'Python {min_version} or higher (recommended), or revert to python-for-android 2019.07.08. '
'Note that you *can* still target Python 2 on Android by including python2 in your '
'requirements.').format(
min_version=MIN_PYTHON_VERSION)

PY_VERSION_ERROR_TEXT = (
'Your Python version {user_major}.{user_minor} is not supported by python-for-android, '
'please upgrade to {min_version} or higher.'
).format(
user_major=sys.version_info.major,
user_minor=sys.version_info.minor,
min_version=MIN_PYTHON_VERSION)


def check_python_version():
# Python 2 special cased because it's a major transition. In the
# future the major or minor versions can increment more quietly.
if sys.version_info.major == 2:
raise BuildInterruptingException(PY2_ERROR_TEXT)

if (sys.version_info.major < MIN_PYTHON_MAJOR_VERSION or
sys.version_info.minor < MIN_PYTHON_MINOR_VERSION):

raise BuildInterruptingException(PY_VERSION_ERROR_TEXT)
11 changes: 3 additions & 8 deletions pythonforandroid/toolchain.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@
from pythonforandroid.pythonpackage import get_dep_names_of_package
from pythonforandroid.recommendations import (
RECOMMENDED_NDK_API, RECOMMENDED_TARGET_API)
from pythonforandroid.util import BuildInterruptingException, handle_build_exception
from pythonforandroid.util import BuildInterruptingException
from pythonforandroid.entrypoints import main


def check_python_dependencies():
Expand Down Expand Up @@ -568,6 +569,7 @@ def add_parser(subparsers, *args, **kwargs):

args, unknown = parser.parse_known_args(sys.argv[1:])
args.unknown_args = unknown

if hasattr(args, "private") and args.private is not None:
# Pass this value on to the internal bootstrap build.py:
args.unknown_args += ["--private", args.private]
Expand Down Expand Up @@ -1187,12 +1189,5 @@ def build_status(self, _args):
print(recipe_str)


def main():
try:
ToolchainCL()
except BuildInterruptingException as exc:
handle_build_exception(exc)


if __name__ == "__main__":
main()
9 changes: 4 additions & 5 deletions pythonforandroid/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,17 @@
from os import getcwd, chdir, makedirs, walk, uname
import sh
import shutil
import sys
from fnmatch import fnmatch
from tempfile import mkdtemp
try:

# This Python version workaround left for compatibility during initial version check
Copy link
Member

Choose a reason for hiding this comment

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

Nice to have comments about this. Best would be some unit tests. I can deal with this later

try: # Python 3
from urllib.request import FancyURLopener
except ImportError:
except ImportError: # Python 2
from urllib import FancyURLopener

from pythonforandroid.logger import (logger, Err_Fore, error, info)

IS_PY3 = sys.version_info[0] >= 3


class WgetDownloader(FancyURLopener):
version = ('Wget/1.17.1')
Expand Down
8 changes: 4 additions & 4 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
data_files = []



Copy link
Member

Choose a reason for hiding this comment

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

nitpick: I hate spacing 😄

# must be a single statement since buildozer is currently parsing it, refs:
# https://github.com/kivy/buildozer/issues/722
install_reqs = [
Expand Down Expand Up @@ -94,15 +95,15 @@ def recursively_include(results, directory, patterns):
install_requires=install_reqs,
entry_points={
'console_scripts': [
'python-for-android = pythonforandroid.toolchain:main',
'p4a = pythonforandroid.toolchain:main',
'python-for-android = pythonforandroid.entrypoints:main',
'p4a = pythonforandroid.entrypoints:main',
],
'distutils.commands': [
'apk = pythonforandroid.bdistapk:BdistAPK',
],
},
classifiers = [
'Development Status :: 4 - Beta',
'Development Status :: 5 - Production/Stable',
'Intended Audience :: Developers',
'License :: OSI Approved :: MIT License',
'Operating System :: Microsoft :: Windows',
Expand All @@ -111,7 +112,6 @@ def recursively_include(results, directory, patterns):
'Operating System :: MacOS :: MacOS X',
'Operating System :: Android',
'Programming Language :: C',
'Programming Language :: Python :: 2',
'Programming Language :: Python :: 3',
'Topic :: Software Development',
'Topic :: Utilities',
Expand Down
10 changes: 8 additions & 2 deletions tests/test_androidmodule_ctypes_finder.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@

import mock
from mock import MagicMock
# This test is still expected to support Python 2, as it tests
# on-Android functionality that we still maintain
try: # Python 3+
from unittest import mock
from unittest.mock import MagicMock
except ImportError: # Python 2
import mock
from mock import MagicMock
Copy link
Member

Choose a reason for hiding this comment

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

You guys are not lazy enough.
I would just go for import mock rather than always going with the try/except.
Basically the mock package is also installed for python3 https://github.com/kivy/python-for-android/blob/v2019.07.08/tox.ini#L7
Once we ditch Python2 completely, it's also easier to just search replace import mock to from unittest import mock or something like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

It isn't installed on my desktop :p

I decided I preferred to be explicit about the Python 2 exception for the limited subset of files where we still need it. I'm willing to change it if you think it's better, but I consider this innocuous.

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't need to be on your desktop since it's in the tox.ini.
It's definitely minor/preference so if you prefer explicit that's fine for me 👍

Copy link

Choose a reason for hiding this comment

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

I would prefer keeping it in a way that works with the standard library (from unittest import mock). I personally don't mind these python 2 special branches because it reminds us that it needs to go 😄 if we change this to import mock nobody will remember to change it back, and I definitely think in the long term doing the right standard library import is better

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for sharing. I actually don't mind that much. I'm just being too lazy and I prefer one line over 4. Specially since it has to be repeated over a lot of (yet to come) tests files

import os
import shutil
import sys
Expand Down
37 changes: 37 additions & 0 deletions tests/test_entrypoints_python2.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@

# This test is a special case that we expect to run under Python 2, so
# include the necessary compatibility imports:
try: # Python 3
from unittest import mock
except ImportError: # Python 2
import mock
Copy link
Member

Choose a reason for hiding this comment

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

Minor let's be lazy here too and just:

import mock


from pythonforandroid.recommendations import PY2_ERROR_TEXT
from pythonforandroid import entrypoints


def test_main_python2():
"""Test that running under Python 2 leads to the build failing, while
running under a suitable version works fine.

Note that this test must be run *using* Python 2 to truly test
that p4a can reach the Python version check before importing some
Python-3-only syntax and crashing.
"""

# Under Python 2, we should get a normal control flow exception
# that is handled properly, not any other type of crash
with mock.patch('sys.version_info') as fake_version_info, \
mock.patch('pythonforandroid.entrypoints.handle_build_exception') as handle_build_exception:

fake_version_info.major = 2
fake_version_info.minor = 7

def check_python2_exception(exc):
"""Check that the exception message is Python 2 specific."""
assert exc.message == PY2_ERROR_TEXT
handle_build_exception.side_effect = check_python2_exception

entrypoints.main()

handle_build_exception.assert_called_once()
47 changes: 41 additions & 6 deletions tests/test_recommendations.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,13 @@
from os.path import join
from sys import version as py_version

try:
from unittest import mock
except ImportError:
# `Python 2` or lower than `Python 3.3` does not
# have the `unittest.mock` module built-in
import mock
from unittest import mock
from pythonforandroid.recommendations import (
check_ndk_api,
check_ndk_version,
check_target_api,
read_ndk_version,
check_python_version,
MAX_NDK_VERSION,
RECOMMENDED_NDK_VERSION,
RECOMMENDED_TARGET_API,
Expand All @@ -33,7 +29,12 @@
OLD_NDK_API_MESSAGE,
NEW_NDK_MESSAGE,
OLD_API_MESSAGE,
MIN_PYTHON_MAJOR_VERSION,
MIN_PYTHON_MINOR_VERSION,
PY2_ERROR_TEXT,
PY_VERSION_ERROR_TEXT,
)

from pythonforandroid.util import BuildInterruptingException

running_in_py2 = int(py_version[0]) < 3
Expand Down Expand Up @@ -202,3 +203,37 @@ def test_check_ndk_api_warning_old_ndk(self):
)
],
)

def test_check_python_version(self):
"""With any version info lower than the minimum, we should get a
BuildInterruptingException with an appropriate message.
"""
with mock.patch('sys.version_info') as fake_version_info:

# Major version is Python 2 => exception
fake_version_info.major = MIN_PYTHON_MAJOR_VERSION - 1
fake_version_info.minor = MIN_PYTHON_MINOR_VERSION
with self.assertRaises(BuildInterruptingException) as context:
check_python_version()
assert context.exception.message == PY2_ERROR_TEXT

# Major version too low => exception
# Using a float valued major version just to test the logic and avoid
# clashing with the Python 2 check
fake_version_info.major = MIN_PYTHON_MAJOR_VERSION - 0.1
fake_version_info.minor = MIN_PYTHON_MINOR_VERSION
with self.assertRaises(BuildInterruptingException) as context:
check_python_version()
assert context.exception.message == PY_VERSION_ERROR_TEXT

# Minor version too low => exception
fake_version_info.major = MIN_PYTHON_MAJOR_VERSION
fake_version_info.minor = MIN_PYTHON_MINOR_VERSION - 1
with self.assertRaises(BuildInterruptingException) as context:
check_python_version()
assert context.exception.message == PY_VERSION_ERROR_TEXT

# Version high enough => nothing interesting happens
fake_version_info.major = MIN_PYTHON_MAJOR_VERSION
fake_version_info.minor = MIN_PYTHON_MINOR_VERSION
check_python_version()
9 changes: 6 additions & 3 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,16 @@ deps =
virtualenv
py3: coveralls
backports.tempfile
# makes it possible to override pytest args, e.g.
# tox -- tests/test_graph.py
# posargs will be replaced by the tox args, so you can override pytest
# args e.g. `tox -- tests/test_graph.py`
commands = pytest {posargs:tests/}
passenv = TRAVIS TRAVIS_*
setenv =
PYTHONPATH={toxinidir}

[testenv:py27]
commands = pytest {posargs:tests/test_androidmodule_ctypes_finder.py tests/test_entrypoints_python2.py}

[testenv:py3]
# for py3 env we will get code coverage
commands =
Expand All @@ -28,7 +31,7 @@ commands = flake8 pythonforandroid/ tests/ ci/

[flake8]
ignore =
E123, E124, E126,
E123, E124, E126, E127, E129
Copy link
Member

Choose a reason for hiding this comment

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

Please not 🙏 I rather remove exceptions than adding them. Let's try to comply with the PEP8 even if it bother us

Copy link
Member Author

@inclement inclement Aug 3, 2019

Choose a reason for hiding this comment

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

I'm complying with the "a foolish consistency is the hobgoblin of little minds" part :<

I have:

with mock.patch('sys.version_info') as fake_version_info, \
     mock.patch('pythonforandroid.entrypoints.handle_build_exception') as handle_build_exception:

Flake8 would prefer misalignment:

with mock.patch('sys.version_info') as fake_version_info, \
         mock.patch('pythonforandroid.entrypoints.handle_build_exception') as handle_build_exception:

I think this is plainly less consistent and readable.

In fact, my version turns out to be exactly the form given in pep8 as an example of how with statements are an exception to the normal recommendations:

with open('/path/to/some/file/you/want/to/read') as file_1, \
     open('/path/to/some/file/being/written', 'w') as file_2:
    file_2.write(file_1.read())

How about if I make this line a #noqa?

Copy link
Member Author

Choose a reason for hiding this comment

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

Interestingly, black makes a real mess of this:

    with mock.patch("sys.version_info") as fake_version_info, mock.patch(
        "pythonforandroid.entrypoints.handle_build_exception"
    ) as handle_build_exception:

I can't be the only one who finds this a much worse choice!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for justifying it, yes it makes sense to me. I also hate the misalignment.
In fact I have two was to make it aligned while still complying with flake8. One involves blackslashes which @opacam suggested he didn't like, the other one involves parenthesis, which you suggested you didn't like 😄
I agree with your statement of foolish consistency, however in general enforcing consistency systematically brings more good than not having it.
Yes I would rather use a noqa too in this case, but I'm not sure how it behaves on multiple lines.
Yes I also think black sucks in this area, another reason I hate it. But also it would avoid us having these discussion and focus on other things, that's the reason I love it.

Copy link
Member

Choose a reason for hiding this comment

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

Well guys, I see more ways to this without relying on black... we can split lines in a lot of ways, this based on @AndreMiras solution in one of his recent PRs:

def mock_sys_info():
    return mock.patch('sys.version_info')


def mock_handle_exception():
    return mock.patch('pythonforandroid.entrypoints.handle_build_exception')


def test_main_python2():

    with mock_sys_info() as m_version, mock_handle_exception() as m_exception:

        m_version.major = 2
        m_version.minor = 7

        def check_python2_exception(exc):
            assert exc.message == PY2_ERROR_TEXT
        m_exception.side_effect = check_python2_exception

        entrypoints.main()

    m_exception.assert_called_once()

With this, you keep all the code below 80 characters, no noqa and using Context manager.

But, I would go with decorators...something like:

@mock.patch('pythonforandroid.entrypoints.handle_build_exception')
@mock.patch('sys.version_info')
def test_main_python2(fake_version_info, handle_build_exception):

    def check_python2_exception(exc):
        assert exc.message == PY2_ERROR_TEXT

    fake_version_info.major = 2
    fake_version_info.minor = 7
    handle_build_exception.side_effect = check_python2_exception

    entrypoints.main()

    handle_build_exception.assert_called_once()

Black it's great as an ortographic corrector and also to unify style of all the project even to find this kind of lines which doesn't fit as expected...then...IMHO... one must push the imagination a little further.

Play what you know and then play above that
Miles Davis

E226,
E402, E501,
W503,
Expand Down