-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
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 our terms of service and privacy 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
Android/android.py
Outdated
sysconfigdata_files = prefix.glob("lib/python*/_sysconfigdata__android_*.py") | ||
host = re.fullmatch( | ||
r"_sysconfigdata__android_(.+).py", next(sysconfigdata_files).name | ||
)[1] |
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.
sysconfigdata_files = prefix.glob("lib/python*/_sysconfigdata__android_*.py") | |
host = re.fullmatch( | |
r"_sysconfigdata__android_(.+).py", next(sysconfigdata_files).name | |
)[1] | |
sysconfigdata_files = prefix.glob("lib/python*/_sysconfigdata__android_*.py") | |
sysconfigdata_file = next(sysconfigdata_files).name | |
host = re.fullmatch(r"_sysconfigdata__android_(.+).py", sysconfigdata_file)[1] |
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.
Done, with a small change to the variable names to distinguish more clearly between Paths and strings.
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 avoid shell=True
? We should try to avoid it, especially as code in python/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, so shell=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.
key, value = match[2], match[3] | ||
if os.environ.get(key) != value: | ||
env[key] = value |
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 seems simpler, unless there's a need to minimise the size of 'env'? You could even use a dictcomp.
key, value = match[2], match[3] | |
if os.environ.get(key) != value: | |
env[key] = value | |
key, value = match[2], match[3] | |
env[key] = value |
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 code is run by the android.py env
command, which cibuildwheel uses to determine the environment variables it needs when building for Android. It outputs all variables changed by the android-env.sh script, which looks something like this:
% ./Android/android.py env aarch64-linux-android
export AR=/Users/msmith/Library/Android/sdk/ndk/27.1.12297006/toolchains/llvm/prebuilt/darwin-x86_64/bin/llvm-ar
export AS=/Users/msmith/Library/Android/sdk/ndk/27.1.12297006/toolchains/llvm/prebuilt/darwin-x86_64/bin/llvm-as
export CC=/Users/msmith/Library/Android/sdk/ndk/27.1.12297006/toolchains/llvm/prebuilt/darwin-x86_64/bin/aarch64-linux-android24-clang
export CFLAGS='-D__BIONIC_NO_PAGE_SIZE_MACRO -I/Users/msmith/git/python/cpython/cross-build/aarch64-linux-android/prefix/include'
export CPU_COUNT=10
export CXX=/Users/msmith/Library/Android/sdk/ndk/27.1.12297006/toolchains/llvm/prebuilt/darwin-x86_64/bin/aarch64-linux-android24-clang++
export CXXFLAGS='-D__BIONIC_NO_PAGE_SIZE_MACRO -I/Users/msmith/git/python/cpython/cross-build/aarch64-linux-android/prefix/include'
export HOST=aarch64-linux-android
export LD=/Users/msmith/Library/Android/sdk/ndk/27.1.12297006/toolchains/llvm/prebuilt/darwin-x86_64/bin/ld
export LDFLAGS='-Wl,--build-id=sha1 -Wl,--no-rosegment -Wl,-z,max-page-size=16384 -Wl,--no-undefined -lm -L/Users/msmith/git/python/cpython/cross-build/aarch64-linux-android/prefix/lib'
export NM=/Users/msmith/Library/Android/sdk/ndk/27.1.12297006/toolchains/llvm/prebuilt/darwin-x86_64/bin/llvm-nm
export PKG_CONFIG='pkg-config --define-prefix'
export PKG_CONFIG_LIBDIR=/Users/msmith/git/python/cpython/cross-build/aarch64-linux-android/prefix/lib/pkgconfig
export RANLIB=/Users/msmith/Library/Android/sdk/ndk/27.1.12297006/toolchains/llvm/prebuilt/darwin-x86_64/bin/llvm-ranlib
export READELF=/Users/msmith/Library/Android/sdk/ndk/27.1.12297006/toolchains/llvm/prebuilt/darwin-x86_64/bin/llvm-readelf
export SHLVL=2
export STRIP=/Users/msmith/Library/Android/sdk/ndk/27.1.12297006/toolchains/llvm/prebuilt/darwin-x86_64/bin/llvm-strip
If this included every single environment variable, it would be a lot less convenient to use it to set variables manually when debugging build problems. It would also be a lot more difficult to verify its correctness.
Co-authored-by: Adam Turner <[email protected]>
#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 env
command which prints necessary environment variables (CC, CFLAGS, etc.).android.py test
command so it can be used to run any test suite, not only Python's own tests.The corresponding cibuildwheel PR is pypa/cibuildwheel#2349.