Fixes hostpython build with macOS venv #2159

AndreMiras · 2020-04-28T22:06:29Z

Running the freshly compiled host python binary from another Python
process (e.g. sh module or os.system()) may get wrongly detected as
if it was ran directly from the venv.
This happens when pyvenv.cfg is present (e.g. venv/pyvenv.cfg).
It makes sysconfig.is_python_build() returns False instead of True
since it's using the compiled interpreter (e.g. ./build-dir/python).
https://docs.python.org/3/library/sysconfig.html#sysconfig.is_python_build
https://github.com/python/cpython/blob/v3.8.2/Lib/sysconfig.py#L127
This is because the sys._home attribute is used during the detection.
The issue was first seen in macOS venv which generates the pyvenv.cfg.
To compare both behaviours, try the following within and without a venv:

import os
os.system("./build-dir/python -E -c 'import sysconfig; print(sysconfig._sys_home)'")

One would return /usr/local/bin and the other None

Refs:

AndreMiras · 2020-04-28T22:08:50Z

pythonforandroid/build.py

@@ -764,7 +764,7 @@ def run_pymodules_install(ctx, modules, project_dir=None,
        shprint(host_python, '-m', 'venv', 'venv')

        # Prepare base environment and upgrade pip:
-        base_env = copy.copy(os.environ)
+        base_env = dict(copy.copy(os.environ))


This has nothing to do with the macOS venv fix, but it suddenly started failing (develop branch included). So I included the fix in the PR but dedicated commit

👍 but you created a separate PR right #2160? nevermind, it's fine to me!

Yes I did separate it in case this PR would take longer to be reviewed/merged.
I've now merged the other one so I'll rebase and address some of the other comments you made, thanks!

opacam · 2020-04-29T09:30:21Z

Makefile

-	cd testapps/on_device_unit_tests/ && \
-    python3 setup.py apk --sdk-dir $(ANDROID_SDK_HOME) --ndk-dir $(ANDROID_NDK_HOME) \
-    --arch=$($@_APP_ARCH)
-
 clean:


🔥 Glad that we can remove this!

opacam · 2020-04-29T09:34:49Z

pythonforandroid/recipes/hostpython3/__init__.py

@@ -35,6 +35,8 @@ class Hostpython3Recipe(Recipe):
    '''The default url to download our host python recipe. This url will
    change depending on the python version set in attribute :attr:`version`.'''

+    patches = ('patches/pyconfig_detection.patch',)


is the new patch working/needed for Python 3.7?

Good question, I've only verified for 3.8 to be honest. I can apply it to 3.8 only to be safe

opacam · 2020-04-29T09:41:06Z

pythonforandroid/recipes/python3/__init__.py

    name = 'python3'

    patches = [
+        ('patches/pyconfig_detection.patch'),
+


Do we need to patch python3 recipe as well? I understand that the problem is with hostpython and MacOs during build stage...:thinking:

Yes I also thought, but then I saw the build failing with the exact same issue so I decided to add it there as well

opacam

Awesome, sure that macOs Users will be delighted with this.

Thanks!!! 😄

rdbisme · 2020-04-29T10:04:31Z

This has been fixed also upstream, apparently: https://bugs.python.org/issue40261

[I went on trying to understand what was happening]

AndreMiras · 2020-04-29T10:07:18Z

Very nice to see @rubendibattista !
So I could have saved a day of my life by just waiting 3.8.3 to be released 😂
Then it also means this patch needs to be applied to 3.8.2 only for now. Will update

opacam · 2020-04-29T10:07:59Z

Nice, so in Python 3.8.3 this should be solved 😄

¡¡¡thanks for the info!!!

rdbisme · 2020-04-29T10:08:51Z

@AndreMiras at least for you was just one day! I spent one week after giving up... I reached that situation of crazyness where I was patching CPython directly. :)

rdbisme · 2020-04-29T10:09:22Z

@opacam and 3.7.8

rdbisme · 2020-04-29T10:14:19Z

However this is the change they're applying upstream: python/cpython@5765aca

Care to see if it solves the problem also here?

opacam · 2020-04-29T11:26:02Z

Maybe it will solve the problem, but I don't have a Mac to test...anyway...if you want to try, the following patch will modify hostpython/python recipes to use the branch you mentioned from CPython 😉

AndreMiras · 2020-04-29T12:15:16Z

Opacam the problem with this approach is it hardcodes the version and people won't be able to use different versions. Plus I'd rather not rely on something not yet release like so.
@rubendibattista I haven't tested that patch you shared. Good thing it's an upstream one so it should be properly tested. Basically I would apply only Modules/main.c change.
I'll rebase this PR and address some of the comments and then create another PR where I apply Modules/main.c patch only and let's see if that compiles

Running the freshly compiled python binary from another Python process (e.g. sh module or os.system()) may get wrongly detected as if it was ran directly from the venv. This happens when `pyvenv.cfg` is present (e.g. venv/pyvenv.cfg). It makes `sysconfig.is_python_build()` returns `False` instead of `True` since it's using the compiled interpreter (e.g. ./build-dir/python). https://docs.python.org/3/library/sysconfig.html#sysconfig.is_python_build https://github.com/python/cpython/blob/v3.8.2/Lib/sysconfig.py#L127 This is because the `sys._home` attribute is used during the detection. The issue was first seen in macOS venv which generates the `pyvenv.cfg`. To compare both behaviours, try the following within and without a venv: ```python import os os.system("./build-dir/python -E -c 'import sysconfig; print(sysconfig._sys_home)'") ``` One would return `/usr/local/bin` and the other `None` Refs: - kivy/kivy-ios#401 - kivy#2063

AndreMiras · 2020-04-29T12:36:30Z

pythonforandroid/recipes/hostpython3/__init__.py

@@ -35,6 +36,10 @@ class Hostpython3Recipe(Recipe):
    '''The default url to download our host python recipe. This url will
    change depending on the python version set in attribute :attr:`version`.'''

+    patches = (
+        ('patches/pyconfig_detection.patch', is_version_lt("3.8.3")),


Now applying to anything lower than 3.8.3. Since this issue was also impacting some recent version of the 3.7 branch I thought it would be easier to go that way than being ultra ranging inside MINOR and PATCH with a complex expression (given MAJOR.MINOR.PATCH)

rdbisme · 2020-04-29T12:42:22Z

@AndreMiras the patch on pythonw.c is not needed?

In any case I could test this. I'll do it ASAP

AndreMiras · 2020-04-29T12:48:04Z

OK the patch got rejected, it doesn't seem to apply properly I haven't investigated much, but i think my solution is good enough until 3.8.3 is released.
Let me know what you guys think

AndreMiras · 2020-04-29T12:51:58Z

@rubendibattista which patch on pythonw.c you mean the code comment? Because the only upstream fix I see is Modules/main.c the rest is unit testing and code comments, right?
Yes please give it a try and let me know

rdbisme · 2020-04-29T13:10:34Z

@AndreMiras Ok, let's go with your initial PR. I'll try to see if I can adapt the upstream patch to current versions of Python.

opacam

💯

AndreMiras · 2020-05-01T14:45:00Z

I found out why the upstream patch didn't apply. It was for the 3.7 branch 😅
The 3.8 one is python/cpython#19110
As for now the fix seems to work, but I will give the upstream patch a try on kivy-ios.
Thanks again for the heads up.
Edit: OK that doesn't fix it 😮 I think it's because the system Python is the one leaking the env down to the compiled host Python

rdbisme · 2020-05-01T15:24:14Z

🔝

🔖 v2020.06.02 * Adds missing requests sub dependencies (kivy#2221) * Bumps to Gradle 6.4.1 (kivy#2222) * Bumps to Cython==0.29.19 (kivy#2220) * Updates install and troubleshooting docs (kivy#2219) * Bumps to Ubuntu 20.04 (kivy#2218) * Attempt to improve the issue template (kivy#2217) * Add `opencv_extras` recipe (kivy#2209) * Split logic for build modes & debug symbols (kivy#2213) * Troubleshoot SSL error (kivy#2205) * Remove superfluous recipes fixes (kivy#2202) * Add tests for hostpython3 recipe (kivy#2196) * Fix for 'cannot find setuptools module' (kivy#2195) * Rename `Hostpython3Recipe` class to camel case (kivy#2194) * Fix `test_should_build` (kivy#2193) * Add initial tests for python3 recipe (kivy#2192) * PythonActivityUtil helper for unpacking data (kivy#2189) * Fixes flake8 errors post update (kivy#2191) * Share PythonUtil.java between bootstraps (kivy#2188) * Java code linting using PMD 6.23.0 (kivy#2187) * Deletes deprecated renpy Python{Activity,Service}.java (kivy#2186) * Removes java concurrency/ folder (kivy#2185) * Reuse common AssetExtract.java (kivy#2182) * Use common Hardware.java (kivy#2183) * Moves kamranzafar/ java directory to common/ (kivy#2184) * Updates release documentation (kivy#2177) * Fixes service only unittest loading (kivy#2181) * Narrows some context manager scopes (kivy#2179) * Downgrades to SDL2 2.0.9 (kivy#2180) * Bump to SDL2 2.0.10 & extract .java from SDL2 tarball (kivy#2113) * Adds pygame recipe (kivy#2164) * Adds macOS install instructions (2165) * Removed python2 support mention from README (kivy#2162) * Adding more assets (kivy#2132) * Get --add-source working for dirs in Gradle builds (kivy#2156) * Fixes python build with macOS venv (kivy#2159)

Borrowed from p4a: kivy/python-for-android#2159 Also refs upstream issue: https://bugs.python.org/issue40261 Note that the fix used upstream cannot really be used directly here because the actual system Python is impacted. Adds venv build to CI to check for regressions.

AndreMiras requested a review from opacam April 28, 2020 22:06

AndreMiras commented Apr 28, 2020

View reviewed changes

AndreMiras force-pushed the feature/fix_macos_venv_build branch 2 times, most recently from 9438b08 to b8f300f Compare April 29, 2020 06:14

opacam reviewed Apr 29, 2020

View reviewed changes

opacam previously approved these changes Apr 29, 2020

View reviewed changes

opacam mentioned this pull request Apr 29, 2020

[WIP] Add correct CFLAGS to hostpython3 and -fPIC flag to avoid relocation error for few recipes #2136

Draft

AndreMiras dismissed opacam’s stale review via 38ba1ad April 29, 2020 12:22

AndreMiras force-pushed the feature/fix_macos_venv_build branch from b8f300f to 38ba1ad Compare April 29, 2020 12:22

AndreMiras commented Apr 29, 2020

View reviewed changes

opacam approved these changes Apr 29, 2020

View reviewed changes

AndreMiras merged commit 83dc122 into kivy:develop Apr 29, 2020

AndreMiras deleted the feature/fix_macos_venv_build branch April 29, 2020 16:03

This was referenced May 1, 2020

pygame recipe #2019

Closed

Fixes building in venv kivy/kivy-ios#462

Merged

obfusk mentioned this pull request Aug 11, 2020

(host)python3: rm version check for pyconfig patch #2292

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes hostpython build with macOS venv #2159

Fixes hostpython build with macOS venv #2159

AndreMiras commented Apr 28, 2020

AndreMiras Apr 28, 2020

opacam Apr 29, 2020

AndreMiras Apr 29, 2020

opacam Apr 29, 2020

opacam Apr 29, 2020

AndreMiras Apr 29, 2020

opacam Apr 29, 2020

AndreMiras Apr 29, 2020

opacam left a comment

rdbisme commented Apr 29, 2020

AndreMiras commented Apr 29, 2020

opacam commented Apr 29, 2020

rdbisme commented Apr 29, 2020

rdbisme commented Apr 29, 2020

rdbisme commented Apr 29, 2020

opacam commented Apr 29, 2020

AndreMiras commented Apr 29, 2020

AndreMiras Apr 29, 2020

rdbisme commented Apr 29, 2020

AndreMiras commented Apr 29, 2020

AndreMiras commented Apr 29, 2020

rdbisme commented Apr 29, 2020

opacam left a comment

AndreMiras commented May 1, 2020 •

edited

Loading

rdbisme commented May 1, 2020

Fixes hostpython build with macOS venv #2159

Fixes hostpython build with macOS venv #2159

Conversation

AndreMiras commented Apr 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opacam left a comment

Choose a reason for hiding this comment

rdbisme commented Apr 29, 2020

AndreMiras commented Apr 29, 2020

opacam commented Apr 29, 2020

rdbisme commented Apr 29, 2020

rdbisme commented Apr 29, 2020

rdbisme commented Apr 29, 2020

opacam commented Apr 29, 2020

AndreMiras commented Apr 29, 2020

Choose a reason for hiding this comment

rdbisme commented Apr 29, 2020

AndreMiras commented Apr 29, 2020

AndreMiras commented Apr 29, 2020

rdbisme commented Apr 29, 2020

opacam left a comment

Choose a reason for hiding this comment

AndreMiras commented May 1, 2020 • edited Loading

rdbisme commented May 1, 2020

AndreMiras commented May 1, 2020 •

edited

Loading