Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-131531: android.py enhancements to support cibuildwheel#132870
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Android/android.py Outdated
| f"PREFIX={prefix}; " | ||
| f". {env_script}; " | ||
| f"export", | ||
| check=True, shell=True, capture_output=True, text=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there a way to avoidshell=True? We should try to avoid it, especially as code inpython/cpython is prominent. Also, prefer 'encoding' to 'text':
| check=True,shell=True,capture_output=True,text=True, | |
| check=True,shell=True,capture_output=True,encoding='utf-8', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In this case the whole point of the code is to run a shell script and determine which environment variables it sets, soshell=True is unavoidable. However, the script is entirely under our control – it's in the repository right next to this file.
Also, prefer 'encoding' to 'text'
Thanks, fixed.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@freakboy3742: Please review this PR, but don't actually merge it yet, as there may be some further updates depending on what happens on the cibuildwheel PR. |
!buildbot android |
bedevere-bot commentedMay 6, 2025
🤖 New build scheduled with the buildbot fleet by@mhsmith for commit9c46ab0 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F132870%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The broad strokes of this (especially the modifications to kick off an external test suite) make sense; a couple of questions inline about specific decisions/changes.
There's also the issue with the CI failures; not sure how many of those are transient issues caused by the state of main, and how many are caused by changes in this PR - at least one of the errors seems to be related to ctypes availability.
| env_script = ANDROID_DIR / "android-env.sh" | ||
| env_output = subprocess.run( | ||
| f"set -eu; " | ||
| f"export HOST={host}; " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there a reason this has become a full export?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
HOST is a somewhat standardized way of indicating cross-compilation, e.g. it's used by conda-build. So it makes sense to expose it to the build system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm also using it in the cibuildwheel PR to setsys.implementation._multiarch in the cross-compilation environment.
| # when building third-party extension modules, so pass them via the | ||
| # NODIST environment variables. | ||
| host_env = android_env(host) | ||
| for name in ["CFLAGS", "CXXFLAGS", "LDFLAGS"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why are these needed for the testbed environment? Is this script being used to drive cibuildwheel?
Or - I guess - why are they needed now to support compilation when they weren't required previously?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This section of the script is only directly used when building Python itself. This PR doesn't change the flags, it only moves the flags which contain build-time paths from the...FLAGS variables (which are exposed by sysconfig for the purpose of building packages) to the...FLAGS_NODIST variables (which are only used when building Python). This approach is recommended inPython's build documentation.
This may be unnecessary for cibuildwheel, because it contains code to rewrite any paths in the sysconfigdata. But cibuildwheel isn't the only context in which packages could be built – e.g. we may also see "native" (non-cross) compilation on Termux.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I think it's caused by this PR – I'll look into it. |
Uh oh!
There was an error while loading.Please reload this page.
#131532 added an Android release package format for the use of external tools such as cibuildwheel. This PR adds a number of features to that package to allow third-party wheels to be built and tested against it:
android.py envcommand which prints necessary environment variables (CC, CFLAGS, etc.).android.py testcommand so it can be used to run any test suite, not only Python's own tests. This involves adding the following options:--site-packagesto copy code into the app--cwdto copy files into the app's working directory-cto run a Python statement-mto run a Python moduleThe corresponding cibuildwheel PR ispypa/cibuildwheel#2349.