Drop Python 2 support #1918
Drop Python 2 support #1918
Conversation
pythonforandroid/toolchain.py
Outdated
| @@ -100,6 +100,13 @@ def check_python_dependencies(): | |||
| toolchain_dir = dirname(__file__) | |||
| sys.path.insert(0, join(toolchain_dir, "tools", "external")) | |||
|
|
|||
| def check_python_version(): | |||
There was a problem hiding this comment.
pythonforandroid/toolchain.py:103:1: E302 expected 2 blank lines, found 1
pythonforandroid/toolchain.py
Outdated
| 'Python 3 (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.') | ||
| if sys.version_info.major < 3: | ||
| raise BuildInterruptingException(py2_text) | ||
|
|
||
| def add_boolean_option(parser, names, no_names=None, |
There was a problem hiding this comment.
pythonforandroid/toolchain.py:111:1: E302 expected 2 blank lines, found 1
|
@inclement, as you said in #1883 comment, still we have an important point to decide, Which python3 version should be the starting point? Here are my 3 cents, Despite Also it's important to mentioned this @AndreMiras comment: So, IMHO, I think that we should go for Python 3.6+ |
|
I have no real objection to going 3.6+, but I'd like to check around if there are any hidden side effects. For instance, are there platforms we want to support where 3.6+ is not available, or even just less easy to access by default? I would think probably not, but I've been surprised before. |
|
I don't think personally that these formatting strings are important enough to jump to However, is there a reason we just add an error/warning at startup about Python 2 no longer being supported, but don't also remove the python 2 recipe and all the special handling for it? I think this could lead to a massive code complexity reduction for us |
|
@Jonast What minimum version would you go for? 3.4? About removing the Python 2 recipe, I think it's nicer to do things this way because it's very simple (since the recipes and local version are totally decoupled) and it's a more user friendly step to make. We can remove the recipe at a future point. Also because I didn't think we had that much special handling for it any more, I would have pushed to remove it earlier if it had never been upgraded from the old one (which had special cases everywhere). |
|
I just updated this to be fairly complete. Currently I put 3.6 as the minimum version, but only as a current default, without other input I would have gone for 3.4. I'm sure we might use various features of the newer versions if they were guaranteed to be present, but is there anything that people are specifically wanting other than f-strings in 3.6? |
I think we have toooons of special handling for it, just think of the tox tests and so many things we aren't using in the overall codebase of p4a itself because a (host) Python 2 can't do it, like |
|
@Jonast Maybe I misunderstood, but allowing those things is the point of this PR - I'm proposing that from now on we support running python-for-android under 3.4+ only. That means we can move to Python3-only syntax in the p4a source. For the android target, I'm thinking we may as well just set a hard deadline of 2020 and add a deprecation warning about it now when Python 2 target builds are used. I think it's worth giving a bit more notice on this one since it's much more of a feature change for users, whereas running p4a itself under Python 3 is a much more trivial update. |
|
@inclement some on-device code of the |
|
@Jonast Ah, I see, I didn't realise that. I'm not that familiar with the tox config, but I think it would be possible to adjust it to only run the android module tests when run under Python 2, do you think that would be an okay intermediate solution? |
|
Yeah that should be possible, and sounds like a good start! I think it's the Edit: I just checked, and the concrete test file where this is relevant should be |
|
I've made an attempt to make the tox.ini run only the androidmodule test under Python 2. Other than this I don't have much that I'm intending to do for this to be mergeable. I haven't updated the docs to say we're Python 3 only because I actually think that's kind of redundant, but I'll add a note about Python 2 deprecation in 2020 if everyone is okay with this plan. There's still the question of minimum Python version. I've changed my mind to say we should be Python 3.4+, but I'm open to changing it again. The main advantages of going higher seem to be:
@AndreMiras @opacam any thoughts? |
|
Here're my 2 cents to advocate for 3.6. |
|
I agree it may make sense to use something that is not too super recent, but still available without major pain in our testing environment. If that is |
| from fnmatch import fnmatch | ||
| from tempfile import mkdtemp | ||
| try: | ||
|
|
||
| # This Python version workaround left for compatibility during initial version check |
There was a problem hiding this comment.
Nice to have comments about this. Best would be some unit tests. I can deal with this later
| @@ -17,6 +17,7 @@ | |||
| data_files = [] | |||
|
|
|||
|
|
|||
|
|
|||
|
Great, I'm convinced, 3.6+ it is. @AndreMiras I'll add some tests before merging, I do want to be able to ensure that everything works under Python 2 up until the version check in the main entrypoint. |
inclement
force-pushed
the
py3_only
branch
2 times, most recently
from
ad080c4
to
173ee9e
Compare
| from unittest.mock import MagicMock | ||
| except ImportError: # Python 2 | ||
| import mock | ||
| from mock import MagicMock |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
tox.ini
Outdated
| @@ -28,7 +31,7 @@ commands = flake8 pythonforandroid/ tests/ ci/ | |||
|
|
|||
| [flake8] | |||
| ignore = | |||
| E123, E124, E126, | |||
| E123, E124, E126, E127, E129 | |||
There was a problem hiding this comment.
Please not 🙏 I rather remove exceptions than adding them. Let's try to comply with the PEP8 even if it bother us
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
tox.ini
Outdated
| E124, # Closing bracket does not match visual indentation | ||
| E126, # Continuation line over-indented for hanging indent | ||
| E127, # Continuation line over-indented for visual indent | ||
| E129, # Visually indented line with same indent as next logical line |
There was a problem hiding this comment.
Nice that you're adding the description for them all 👏
My review on this was pending and I forgot to submit.
Basically I was asking not to add E127 and E129 in the exception list since we've been successfully honored it until then.
I agree at first some PE8 feels annoying, but then we get used to and it definitely helps with consistency over a collaborative project like p4a
| try: # Python 3 | ||
| from unittest import mock | ||
| except ImportError: # Python 2 | ||
| import mock |
There was a problem hiding this comment.
Minor let's be lazy here too and just:
import mock
There was a problem hiding this comment.
That's some very nice piece of work here 👏
Thanks @inclement!
Do you mind squashing in your commit merge since it's not being squashed in the PR?
We've been keeping a neat git history in lately in the recent PR, I think it's good if we keep it that way
|
I'm not convinced that non-squash merges cannot be neat, but okay for now. |
|
Thanks for the reviews! |
|
Ahah thanks for making it even though you're not convinced, I really appreciate |
|
guys, I am facing the same issue |
|
@sujithkumar05 If you don't understand some part of that error message, please ask about it on the Kivy discord or kivy-users mailing list. |
|
Can u please share the discord link . It will be a great help for me.
…On Mon, Aug 26, 2019, 2:17 AM Alexander Taylor ***@***.***> wrote:
@sujithkumar05 <https://github.com/sujithkumar05> If you don't understand
some part of that error message, please ask about it on the Kivy discord or
kivy-users mailing list.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1918?email_source=notifications&email_token=ALPOD2OTKOLZS4OEHJNQETTQGLVX3A5CNFSM4IC3KFWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5C3NNI#issuecomment-524662453>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ALPOD2NM2V3EM3PF757ZBHLQGLVX3ANCNFSM4IC3KFWA>
.
|
This should have been the original value, agreed in kivy#1918
* Added missing recommendations command and cleaned up code * Added test for print_recommendations running correctly * Updated minimum Python version to 3.6 This should have been the original value, agreed in #1918 * Added a blank line to make pylint happy * Moved f-strings to format calls to preserve py2 compat in this file * Changed travis build to use Python 3.8 * Undid travis change, for merging in a separate PR
I propose dropping support for running under Python 2 after the current release is done.
I thought about adding an override command, but actually decided there isn't much point. Anyone who really wants to run under Python 2 can edit the code.
I think we should continue supporting the python2 recipe for now, since that's a smaller change and I don't perceive that it adds a big maintenance burden (at least in its current form, it was much worse before!).