Call Cython via python -m Cython rather than system-wide binary
#1937
Conversation
There was a problem hiding this comment.
Nice, I never thought about doing it this way. It does seem more correct.
The main reason for never worrying about it (and in fact explicitly searching for any cython regardless of Python version) is that it almost never matters, for a long time I only had one of them installed. I vaguely remember some stronger reason to think it was okay but I don't remember what. I have the impression that cython2/cython3 are beginning to be more likely to behave differently though, we had some other issue about it, so even more reason to make the change.
| @@ -374,7 +365,6 @@ def prepare_build_environment(self, | |||
| if not self.ccache: | |||
| info('ccache is missing, the build will not be optimized in the ' | |||
| 'future.') | |||
| self.cython = get_cython_path() | |||
There was a problem hiding this comment.
Could we still add a check here for Cython being available, or a BuildInterruptingException if not?
There was a problem hiding this comment.
Oh, good point! Sure, that would be possible, let me have a look at it
ghost
dismissed
Dockerfile.py3
Outdated
| @@ -107,6 +107,7 @@ RUN dpkg --add-architecture i386 \ | |||
| libpangox-1.0-0:i386 libpangoxft-1.0-0:i386 libidn11:i386 \ | |||
| zip zlib1g-dev zlib1g:i386 \ | |||
| && apt -y autoremove | |||
| RUN ${RETRY} pip3 install -U Cython | |||
There was a problem hiding this comment.
Wait what? It's already there https://github.com/kivy/python-for-android/blob/v2019.07.08/Dockerfile.py3#L126
pythonforandroid/recipe.py
Outdated
| @@ -1024,8 +1024,12 @@ def cythonize_file(self, env, build_dir, filename): | |||
| del cyenv['PYTHONPATH'] | |||
| if 'PYTHONNOUSERSITE' in cyenv: | |||
| cyenv.pop('PYTHONNOUSERSITE') | |||
| cython_command = sh.Command(self.ctx.cython) | |||
| shprint(cython_command, filename, *self.cython_args, _env=cyenv) | |||
| cython_command = sh.Command( | |||
There was a problem hiding this comment.
technically this isn't a cython command, but a python command, correct?
The cython however comes later once you adds the -m cython to it few lines below
ghost
changed the title
python -m Cython rather than system-wide binarypython -m Cython rather than system-wide binary
ghost
mentioned this pull request
Dockerfile.py3
Outdated
| @@ -133,4 +135,5 @@ USER ${USER} | |||
| # install python-for-android from current branch | |||
| RUN virtualenv --python=python3 venv \ | |||
| && . venv/bin/activate \ | |||
| && pip3 install -U Cython \ | |||
There was a problem hiding this comment.
Isn't the one installed system wide enough?
https://github.com/kivy/python-for-android/blob/v2019.07.08/Dockerfile.py3#L126
There was a problem hiding this comment.
Hm true I may have interpreted that one error situation wrong, let me try it might work without that 👍
There was a problem hiding this comment.
Yup system-wide is enough, apparently 😂 well-spotted!
Oops, checked wrong build result. The system-wide is actually as it looks like mostly useless (maybe also including for any other usage?) because we launch in a virtualenv with no site-packages access, and as a result there appears to be no access to any of these outside system pacakges. The new code to call Cython of this pull request calls via python3/python2 which maps back to the virtualenv python (since venv/bin/activate changes $PATH accordingly) so it really needs to be available in the virtualenv to work.
There was a problem hiding this comment.
I understand, thanks for investigating!
Then maybe we should ditch the system one?
Edit: just saw that you already did, thanks!
There was a problem hiding this comment.
Ouch, I just saw the build is failing for Python2.
https://travis-ci.org/kivy/python-for-android/jobs/567481453
2943 RAN: /usr/bin/python2 -m Cython.Build.Cythonize ./jnius/jnius.pyx
2944
2945 STDOUT:
2946/usr/bin/python2: No module named Cython.Build
So that's properly weird and annoying. Basically it means if we want to support the "Python2 basic" apk build in the CI, we do need the system cython on python2. While only having the venv cython3. That would look kinda inconsistent in the Dockerfile, but with some comments that should make it.
So basically we want to have:
RUN pip2 install --upgrade Cython==0.28.6
And also in the venv:
&& pip3 install --upgrade Cython==0.28.6
By the way, let's take the opportunity to make them consistent.
- uppercase
Cythonbecause that's it's naming on pypi - long argument
--upgradebecause we love explicit - explicit "officially p4a tested" version
0.28.6in all places (eventually via variable)
There was a problem hiding this comment.
Nice work, thanks @Jonast 💪
Side note:
I would not have bothered maintaining the Dockerfile.py2, at some point we should drop it I think.
Basically it's used to reproduce/address host Python2 issues, but we're dropping its support
|
I just realized I never tested if this actually fixes #1885, even though I believe this is the "more correct" way to call it in any case. Does anyone have a test environment for that around? Otherwise I suppose I should set myself one up real quick and actually give that a try |
Yes same feeling here, so we should make it into the tree in any case (after the build passes of course) |
- call Cython via `python -m Cython` to avoid picking one not matching the current python version (which can happen if just calling `Cython`) - make sure Cython is present in Dockerfile.py3 and Dockerfile.py2 - make sure python 2 is available in Dockerfile.py3 - this should fix #1885
|
Good job! Feel free to merge once the build turns ✔️ |
This changes calls to Cython to
python -m Cythonto avoid picking one not matching the current python version (which can happen if just callingCython). This should fix #1885